From d13a7a6f100576b1e30dc044b2e0c4cbcb6196f6 Mon Sep 17 00:00:00 2001 From: Arjun Shankar Date: Tue, 2 Aug 2022 11:10:25 +0200 Subject: socket: Check lengths before advancing pointer in CMSG_NXTHDR The inline and library functions that the CMSG_NXTHDR macro may expand to increment the pointer to the header before checking the stride of the increment against available space. Since C only allows incrementing pointers to one past the end of an array, the increment must be done after a length check. This commit fixes that and includes a regression test for CMSG_FIRSTHDR and CMSG_NXTHDR. The Linux, Hurd, and generic headers are all changed. Tested on Linux on armv7hl, i686, x86_64, aarch64, ppc64le, and s390x. [BZ #28846] Reviewed-by: Siddhesh Poyarekar (cherry picked from commit 9c443ac4559a47ed99859bd80d14dc4b6dd220a1) --- sysdeps/unix/sysv/linux/bits/socket.h | 40 +++++++++++++++++++++++++++++------ sysdeps/unix/sysv/linux/cmsg_nxthdr.c | 36 ++++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 15 deletions(-) (limited to 'sysdeps/unix') diff --git a/sysdeps/unix/sysv/linux/bits/socket.h b/sysdeps/unix/sysv/linux/bits/socket.h index 4f1f810ea1..539b8d7716 100644 --- a/sysdeps/unix/sysv/linux/bits/socket.h +++ b/sysdeps/unix/sysv/linux/bits/socket.h @@ -307,6 +307,12 @@ struct cmsghdr + CMSG_ALIGN (sizeof (struct cmsghdr))) #define CMSG_LEN(len) (CMSG_ALIGN (sizeof (struct cmsghdr)) + (len)) +/* Given a length, return the additional padding necessary such that + len + __CMSG_PADDING(len) == CMSG_ALIGN (len). */ +#define __CMSG_PADDING(len) ((sizeof (size_t) \ + - ((len) & (sizeof (size_t) - 1))) \ + & (sizeof (size_t) - 1)) + extern struct cmsghdr *__cmsg_nxthdr (struct msghdr *__mhdr, struct cmsghdr *__cmsg) __THROW; #ifdef __USE_EXTERN_INLINES @@ -316,18 +322,38 @@ extern struct cmsghdr *__cmsg_nxthdr (struct msghdr *__mhdr, _EXTERN_INLINE struct cmsghdr * __NTH (__cmsg_nxthdr (struct msghdr *__mhdr, struct cmsghdr *__cmsg)) { + /* We may safely assume that __cmsg lies between __mhdr->msg_control and + __mhdr->msg_controllen because the user is required to obtain the first + cmsg via CMSG_FIRSTHDR, set its length, then obtain subsequent cmsgs + via CMSG_NXTHDR, setting lengths along the way. However, we don't yet + trust the value of __cmsg->cmsg_len and therefore do not use it in any + pointer arithmetic until we check its value. */ + + unsigned char * __msg_control_ptr = (unsigned char *) __mhdr->msg_control; + unsigned char * __cmsg_ptr = (unsigned char *) __cmsg; + + size_t __size_needed = sizeof (struct cmsghdr) + + __CMSG_PADDING (__cmsg->cmsg_len); + + /* The current header is malformed, too small to be a full header. */ if ((size_t) __cmsg->cmsg_len < sizeof (struct cmsghdr)) - /* The kernel header does this so there may be a reason. */ return (struct cmsghdr *) 0; + /* There isn't enough space between __cmsg and the end of the buffer to + hold the current cmsg *and* the next one. */ + if (((size_t) + (__msg_control_ptr + __mhdr->msg_controllen - __cmsg_ptr) + < __size_needed) + || ((size_t) + (__msg_control_ptr + __mhdr->msg_controllen - __cmsg_ptr + - __size_needed) + < __cmsg->cmsg_len)) + + return (struct cmsghdr *) 0; + + /* Now, we trust cmsg_len and can use it to find the next header. */ __cmsg = (struct cmsghdr *) ((unsigned char *) __cmsg + CMSG_ALIGN (__cmsg->cmsg_len)); - if ((unsigned char *) (__cmsg + 1) > ((unsigned char *) __mhdr->msg_control - + __mhdr->msg_controllen) - || ((unsigned char *) __cmsg + CMSG_ALIGN (__cmsg->cmsg_len) - > ((unsigned char *) __mhdr->msg_control + __mhdr->msg_controllen))) - /* No more entries. */ - return (struct cmsghdr *) 0; return __cmsg; } #endif /* Use `extern inline'. */ diff --git a/sysdeps/unix/sysv/linux/cmsg_nxthdr.c b/sysdeps/unix/sysv/linux/cmsg_nxthdr.c index 15b7a3a925..24f72b797a 100644 --- a/sysdeps/unix/sysv/linux/cmsg_nxthdr.c +++ b/sysdeps/unix/sysv/linux/cmsg_nxthdr.c @@ -23,18 +23,38 @@ struct cmsghdr * __cmsg_nxthdr (struct msghdr *mhdr, struct cmsghdr *cmsg) { + /* We may safely assume that cmsg lies between mhdr->msg_control and + mhdr->msg_controllen because the user is required to obtain the first + cmsg via CMSG_FIRSTHDR, set its length, then obtain subsequent cmsgs + via CMSG_NXTHDR, setting lengths along the way. However, we don't yet + trust the value of cmsg->cmsg_len and therefore do not use it in any + pointer arithmetic until we check its value. */ + + unsigned char * msg_control_ptr = (unsigned char *) mhdr->msg_control; + unsigned char * cmsg_ptr = (unsigned char *) cmsg; + + size_t size_needed = sizeof (struct cmsghdr) + + __CMSG_PADDING (cmsg->cmsg_len); + + /* The current header is malformed, too small to be a full header. */ if ((size_t) cmsg->cmsg_len < sizeof (struct cmsghdr)) - /* The kernel header does this so there may be a reason. */ - return NULL; + return (struct cmsghdr *) 0; + + /* There isn't enough space between cmsg and the end of the buffer to + hold the current cmsg *and* the next one. */ + if (((size_t) + (msg_control_ptr + mhdr->msg_controllen - cmsg_ptr) + < size_needed) + || ((size_t) + (msg_control_ptr + mhdr->msg_controllen - cmsg_ptr + - size_needed) + < cmsg->cmsg_len)) + + return (struct cmsghdr *) 0; + /* Now, we trust cmsg_len and can use it to find the next header. */ cmsg = (struct cmsghdr *) ((unsigned char *) cmsg + CMSG_ALIGN (cmsg->cmsg_len)); - if ((unsigned char *) (cmsg + 1) > ((unsigned char *) mhdr->msg_control - + mhdr->msg_controllen) - || ((unsigned char *) cmsg + CMSG_ALIGN (cmsg->cmsg_len) - > ((unsigned char *) mhdr->msg_control + mhdr->msg_controllen))) - /* No more entries. */ - return NULL; return cmsg; } libc_hidden_def (__cmsg_nxthdr) -- cgit 1.4.1