ldpd: buffer underflow, thread safety (PVS-Studio)

This commit fixes two issues:
- memcpy() using containers of different sizes when using addr2sa(), mixing
  'struct sockaddr_storage' and 'union sockunion'.
- addr2sa() function not being thread safe (using a local static variable as
  container.

Signed-off-by: F. Aragon <paco@voltanet.io>

diff --git a/ldpd/ldpd.h b/ldpd/ldpd.h
index ed0a5407b2bf476fa6ecc42530cabf96d79e885a..91135055817fa1cb8b477383b554bfe2ef1c1888 100644 (file)
--- a/ldpd/ldpd.h
+++ b/ldpd/ldpd.h
@@ -691,7 +691,8 @@ void                 embedscope(struct sockaddr_in6 *);
 void            recoverscope(struct sockaddr_in6 *);
 void            addscope(struct sockaddr_in6 *, uint32_t);
 void            clearscope(struct in6_addr *);
-struct sockaddr        *addr2sa(int af, union ldpd_addr *, uint16_t);
+void            addr2sa(int af, const union ldpd_addr *, uint16_t,
+                   union sockunion *su);
 void            sa2addr(struct sockaddr *, int *, union ldpd_addr *,
                    in_port_t *);
 socklen_t       sockaddr_len(struct sockaddr *);

diff --git a/ldpd/neighbor.c b/ldpd/neighbor.c
index 1c3f650dff3baede6f9fb730b862d3603106f1ad..78a6131ca404f6e2f15a00c9cca73ee01988cbec 100644 (file)
--- a/ldpd/neighbor.c
+++ b/ldpd/neighbor.c
@@ -584,8 +584,8 @@ nbr_connect_cb(struct thread *thread)
 int
 nbr_establish_connection(struct nbr *nbr)
 {
-       struct sockaddr_storage  local_sa;
-       struct sockaddr_storage  remote_sa;
+       union sockunion          local_su;
+       union sockunion          remote_su;
        struct adj              *adj;
        struct nbr_params       *nbrp;
 #ifdef __OpenBSD__
@@ -619,16 +619,14 @@ nbr_establish_connection(struct nbr *nbr)
 #endif
        }
 
-       memcpy(&local_sa, addr2sa(nbr->af, &nbr->laddr, 0), sizeof(local_sa));
-       memcpy(&remote_sa, addr2sa(nbr->af, &nbr->raddr, LDP_PORT),
-           sizeof(local_sa));
+       addr2sa(nbr->af, &nbr->laddr, 0, &local_su);
+       addr2sa(nbr->af, &nbr->raddr, LDP_PORT, &remote_su);
        if (nbr->af == AF_INET6 && nbr->raddr_scope)
-               addscope((struct sockaddr_in6 *)&remote_sa, nbr->raddr_scope);
+               addscope(&remote_su.sin6, nbr->raddr_scope);
 
-       if (bind(nbr->fd, (struct sockaddr *)&local_sa,
-           sockaddr_len((struct sockaddr *)&local_sa)) == -1) {
+       if (bind(nbr->fd, &local_su.sa, sockaddr_len(&local_su.sa)) == -1) {
                log_warn("%s: error while binding socket to %s", __func__,
-                   log_sockaddr((struct sockaddr *)&local_sa));
+                        log_sockaddr(&local_su.sa));
                close(nbr->fd);
                return (-1);
        }
@@ -646,15 +644,15 @@ nbr_establish_connection(struct nbr *nbr)
                send_hello(adj->source.type, adj->source.link.ia,
                    adj->source.target);
 
-       if (connect(nbr->fd, (struct sockaddr *)&remote_sa,
-           sockaddr_len((struct sockaddr *)&remote_sa)) == -1) {
+       if (connect(nbr->fd, &remote_su.sa, sockaddr_len(&remote_su.sa))
+           == -1) {
                if (errno == EINPROGRESS) {
                        thread_add_write(master, nbr_connect_cb, nbr, nbr->fd,
                                         &nbr->ev_connect);
                        return (0);
                }
                log_warn("%s: error while connecting to %s", __func__,
-                   log_sockaddr((struct sockaddr *)&remote_sa));
+                        log_sockaddr(&remote_su.sa));
                close(nbr->fd);
                return (-1);
        }

diff --git a/ldpd/packet.c b/ldpd/packet.c
index b0f9c5eb14f8abadd7dcbf8129f6647610ff061d..8ca90841de82e57ce7ead41e2c6dc747016853ad 100644 (file)
--- a/ldpd/packet.c
+++ b/ldpd/packet.c
@@ -70,7 +70,7 @@ int
 send_packet(int fd, int af, union ldpd_addr *dst, struct iface_af *ia,
     void *pkt, size_t len)
 {
-       struct sockaddr         *sa;
+       union sockunion su;
 
        switch (af) {
        case AF_INET:
@@ -97,10 +97,10 @@ send_packet(int fd, int af, union ldpd_addr *dst, struct iface_af *ia,
                fatalx("send_packet: unknown af");
        }
 
-       sa = addr2sa(af, dst, LDP_PORT);
-       if (sendto(fd, pkt, len, 0, sa, sockaddr_len(sa)) == -1) {
+       addr2sa(af, dst, LDP_PORT, &su);
+       if (sendto(fd, pkt, len, 0, &su.sa, sockaddr_len(&su.sa)) == -1) {
                log_warn("%s: error sending packet to %s", __func__,
-                   log_sockaddr(sa));
+                        log_sockaddr(&su.sa));
                return (-1);
        }
 

diff --git a/ldpd/pfkey.c b/ldpd/pfkey.c
index a1a79dabfd359291611a7c776494da86c14af4b0..906737217c0e0513a7279b6658f9f9dde1e2ca22 100644 (file)
--- a/ldpd/pfkey.c
+++ b/ldpd/pfkey.c
@@ -64,17 +64,17 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
        ssize_t                 n;
        int                     len = 0;
        int                     iov_cnt;
-       struct sockaddr_storage ssrc, sdst, smask, dmask;
-       struct sockaddr         *saptr;
+       struct sockaddr_storage smask, dmask;
+       union sockunion         su_src, su_dst;
 
        if (!pid)
                pid = getpid();
 
        /* we need clean sockaddr... no ports set */
-       memset(&ssrc, 0, sizeof(ssrc));
        memset(&smask, 0, sizeof(smask));
-       if ((saptr = addr2sa(af, src, 0)))
-               memcpy(&ssrc, saptr, sizeof(ssrc));
+
+       addr2sa(af, src, 0, &su_src);
+
        switch (af) {
        case AF_INET:
                memset(&((struct sockaddr_in *)&smask)->sin_addr, 0xff, 32/8);
@@ -86,13 +86,13 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
        default:
                return (-1);
        }
-       smask.ss_family = ssrc.ss_family;
-       smask.ss_len = ssrc.ss_len;
+       smask.ss_family = su_src.sa.sa_family;
+       smask.ss_len = sockaddr_len(&su_src.sa);
 
-       memset(&sdst, 0, sizeof(sdst));
        memset(&dmask, 0, sizeof(dmask));
-       if ((saptr = addr2sa(af, dst, 0)))
-               memcpy(&sdst, saptr, sizeof(sdst));
+
+       addr2sa(af, dst, 0, &su_dst);
+
        switch (af) {
        case AF_INET:
                memset(&((struct sockaddr_in *)&dmask)->sin_addr, 0xff, 32/8);
@@ -104,8 +104,8 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
        default:
                return (-1);
        }
-       dmask.ss_family = sdst.ss_family;
-       dmask.ss_len = sdst.ss_len;
+       dmask.ss_family = su_dst.sa.sa_family;
+       dmask.ss_len = sockaddr_len(&su_dst.sa);
 
        memset(&smsg, 0, sizeof(smsg));
        smsg.sadb_msg_version = PF_KEY_V2;
@@ -138,11 +138,13 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 
        memset(&sa_src, 0, sizeof(sa_src));
        sa_src.sadb_address_exttype = SADB_EXT_ADDRESS_SRC;
-       sa_src.sadb_address_len = (sizeof(sa_src) + ROUNDUP(ssrc.ss_len)) / 8;
+       sa_src.sadb_address_len =
+               (sizeof(sa_src) + ROUNDUP(sockaddr_len(&su_src.sa))) / 8;
 
        memset(&sa_dst, 0, sizeof(sa_dst));
        sa_dst.sadb_address_exttype = SADB_EXT_ADDRESS_DST;
-       sa_dst.sadb_address_len = (sizeof(sa_dst) + ROUNDUP(sdst.ss_len)) / 8;
+       sa_dst.sadb_address_len =
+               (sizeof(sa_dst) + ROUNDUP(sockaddr_len(&su_dst.sa))) / 8;
 
        sa.sadb_sa_auth = aalg;
        sa.sadb_sa_encrypt = SADB_X_EALG_AES; /* XXX */
@@ -195,8 +197,8 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
        iov[iov_cnt].iov_base = &sa_dst;
        iov[iov_cnt].iov_len = sizeof(sa_dst);
        iov_cnt++;
-       iov[iov_cnt].iov_base = &sdst;
-       iov[iov_cnt].iov_len = ROUNDUP(sdst.ss_len);
+       iov[iov_cnt].iov_base = &su_dst;
+       iov[iov_cnt].iov_len = ROUNDUP(sockaddr_len(&su_dst.sa));
        smsg.sadb_msg_len += sa_dst.sadb_address_len;
        iov_cnt++;
 
@@ -204,8 +206,8 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
        iov[iov_cnt].iov_base = &sa_src;
        iov[iov_cnt].iov_len = sizeof(sa_src);
        iov_cnt++;
-       iov[iov_cnt].iov_base = &ssrc;
-       iov[iov_cnt].iov_len = ROUNDUP(ssrc.ss_len);
+       iov[iov_cnt].iov_base = &su_src;
+       iov[iov_cnt].iov_len = ROUNDUP(sockaddr_len(&su_src.sa));
        smsg.sadb_msg_len += sa_src.sadb_address_len;
        iov_cnt++;
 

diff --git a/ldpd/socket.c b/ldpd/socket.c
index eaea9973a015d237b40b6c84b80f96fe15a2e9e9..aefa3461a8781a42be5b3182f4ee155dc71452b9 100644 (file)
--- a/ldpd/socket.c
+++ b/ldpd/socket.c
@@ -37,7 +37,7 @@ ldp_create_socket(int af, enum socket_type type)
 {
        int                      fd, domain, proto;
        union ldpd_addr          addr;
-       struct sockaddr_storage  local_sa;
+       union sockunion          local_su;
 #ifdef __OpenBSD__
        int                      opt;
 #endif
@@ -70,14 +70,12 @@ ldp_create_socket(int af, enum socket_type type)
        case LDP_SOCKET_DISC:
                /* listen on all addresses */
                memset(&addr, 0, sizeof(addr));
-               memcpy(&local_sa, addr2sa(af, &addr, LDP_PORT),
-                   sizeof(local_sa));
+               addr2sa(af, &addr, LDP_PORT, &local_su);
                break;
        case LDP_SOCKET_EDISC:
        case LDP_SOCKET_SESSION:
                addr = (ldp_af_conf_get(ldpd_conf, af))->trans_addr;
-               memcpy(&local_sa, addr2sa(af, &addr, LDP_PORT),
-                   sizeof(local_sa));
+               addr2sa(af, &addr, LDP_PORT, &local_su);
                /* ignore any possible error */
                sock_set_bindany(fd, 1);
                break;
@@ -90,8 +88,7 @@ ldp_create_socket(int af, enum socket_type type)
                close(fd);
                return (-1);
        }
-       if (bind(fd, (struct sockaddr *)&local_sa,
-           sockaddr_len((struct sockaddr *)&local_sa)) == -1) {
+       if (bind(fd, &local_su.sa, sockaddr_len(&local_su.sa)) == -1) {
                save_errno = errno;
                if (ldpd_privs.change(ZPRIVS_LOWER))
                        log_warn("%s: could not lower privs", __func__);
@@ -307,7 +304,7 @@ sock_set_md5sig(int fd, int af, union ldpd_addr *addr, const char *password)
        if (fd == -1)
                return (0);
 #if HAVE_DECL_TCP_MD5SIG
-       memcpy(&su, addr2sa(af, addr, 0), sizeof(su));
+       addr2sa(af, addr, 0, &su);
 
        if (ldpe_privs.change(ZPRIVS_RAISE)) {
                log_warn("%s: could not raise privs", __func__);

diff --git a/ldpd/util.c b/ldpd/util.c
index e735263f5fdcd316cb575ce10756286ccf206edd..12f9cb0ccf94d580d5b1e259c5167be2d52564c8 100644 (file)
--- a/ldpd/util.c
+++ b/ldpd/util.c
@@ -305,14 +305,13 @@ clearscope(struct in6_addr *in6)
        }
 }
 
-struct sockaddr *
-addr2sa(int af, union ldpd_addr *addr, uint16_t port)
+void
+addr2sa(int af, const union ldpd_addr *addr, uint16_t port, union sockunion *su)
 {
-       static struct sockaddr_storage   ss;
-       struct sockaddr_in              *sa_in = (struct sockaddr_in *)&ss;
-       struct sockaddr_in6             *sa_in6 = (struct sockaddr_in6 *)&ss;
+       struct sockaddr_in              *sa_in = &su->sin;
+       struct sockaddr_in6             *sa_in6 = &su->sin6;
 
-       memset(&ss, 0, sizeof(ss));
+       memset(su, 0, sizeof(*su));
        switch (af) {
        case AF_INET:
                sa_in->sin_family = AF_INET;
@@ -333,8 +332,6 @@ addr2sa(int af, union ldpd_addr *addr, uint16_t port)
        default:
                fatalx("addr2sa: unknown af");
        }
-
-       return ((struct sockaddr *)&ss);
 }
 
 void