[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#514917: libc6-dev: CMSG_NXTHDR can't be used to construct new control messages, due to its payload length validation



Package: libc6-dev
Version: 2.7-18
Severity: normal

The macros CMSG_FIRSTHDR and CMSG_NXTHDR (defined in in
/usr/include/bits/bits/socket.h) use inconsistent validation of the
length of the next control message.

CMSG_FIRSTHDR just checks that the msg_controllen is large enough for
a cmsghdr to fit. CMSG_NXTHDR also checks that the *payload* of the cmsg
fits. Compare

  #define CMSG_FIRSTHDR(mhdr) \
    ((size_t) (mhdr)->msg_controllen >= sizeof (struct cmsghdr)		      \
     ? (struct cmsghdr *) (mhdr)->msg_control : (struct cmsghdr *) NULL)

to

  #define CMSG_NXTHDR(mhdr, cmsg) __cmsg_nxthdr (mhdr, cmsg)

  _EXTERN_INLINE struct cmsghdr *
  __NTH (__cmsg_nxthdr (struct msghdr *__mhdr, struct cmsghdr *__cmsg))
  {
    if ((size_t) __cmsg->cmsg_len < sizeof (struct cmsghdr))
      /* The kernel header does this so there may be a reason.  */
      return 0;

    __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 0;
    return __cmsg;
  }

When parsing a msghdr from recvmsg, it makes sense to check that the
payload length fits, and return NULL if the messages appears to be
truncated. From this point of view, it would make sense to add a
similar check to CMSG_FIRSTHDR.

However, when preparing a msghdr for sendmsg, consisting of several
control messages, this length check makes these macros almost
unusable. Consider code like

  struct msghdr hdr;
  struct cmsghdr *cmsg;
  [...]
  hdr.msg_controllen = CMSG_SPACE(sizeof(struct ucred)) + CMSG_SPACE(sizeof(int));
  hdr.msg_control = alloca(hdr.msg_controllen);

  cmsg = CMSG_FIRSTHDR(&hdr);
  [...]
  cmsg->cmsg_len = CMSG_LEN(sizeof(struct ucred));

  cmsg = CMSG_NXTHDR(&hdr, cmsg);
  [...]
  cmsg->cmsg_len = CMSG_LEN(sizeof(int));

As far as I understand, this is the proper way to fill out a msghdr
structure with two control messages. However, the call to CMSG_NXTHDR
implies a length check that accesses __cmsg->cmsg_len (line marked ->
in the definition). This field is not yet initialized, and if it
happens to contain a large value, CMSG_NXTHDR returns NULL, not the
address of the space allocated for the second control message. So then
the second control message, which I carefully allocated space for,
can't be filled out.

CMSG_FIRSTHDR does not check the cmsg_len field of the first control
message, and this is documented in the sense that the example code for
sending an array of fds in the linux cmsg(3) man page (as well as BSD
manpages) depends on this behavior: If CMSG_FIRSTHDR used a stricter
check as in CMSG_NXTHDR, then this example code would fail in exactly
the same way as my example, due to an access of an initialized
cmsg_len field. So why should CMSG_NXTHDR use a stricter length check?

I don't know how other C libraries do this, or what relevant standards
say. The easiest fix is to just remove the check from CMSG_NXTHDR.

As a workaround, I think I could memset the entire control area to
zero, so that all length fields are zero, no matter where they're
actually located when the control message is layed out, but that still
leaves CMSG_FIRSTHDR and CMSG_NXTHDR with inconsistent behavior.

To me it would make more sense to have separate macros for reading and
writing:

  CMSG_FIRSTHDR(mhdr)
  CMSG_NXTHDR(mhdr, cmsg)

could check the length, and would be used for processing msghdr from
recvmesg. Then have two new macros,

  CMSG_PUT_FIRSTHDR(mhdr, len)
  CMSG_PUT_NXTHDR(mhdr, cmsg, len)

that takes the length of the next control message as an additional
argument, uses that to check that the control message fits within
msg_controllen, and they can also initialize cmsg_len while they're at
it. But maybe such an API change is totally unrealistic.

Regards,
/Niels Möller

-- System Information:
Debian Release: 5.0
  APT prefers testing
  APT policy: (500, 'testing')
Architecture: i386 (i686)

Kernel: Linux 2.6.26-1-686 (SMP w/1 CPU core)
Locale: LANG=C, LC_CTYPE=sv_SE.ISO-8859-1 (charmap=ISO-8859-1)
Shell: /bin/sh linked to /bin/bash

Versions of packages libc6-dev depends on:
ii  libc6                         2.7-18     GNU C Library: Shared libraries
ii  linux-libc-dev                2.6.26-13  Linux support headers for userspac

Versions of packages libc6-dev recommends:
ii  gcc [c-compiler]             4:4.3.2-2   The GNU C compiler
ii  gcc-2.95 [c-compiler]        1:2.95.4-27 The GNU C compiler
ii  gcc-3.0 [c-compiler]         1:3.0.4-7   The GNU C compiler.
ii  gcc-3.2 [c-compiler]         1:3.2.3-9   The GNU C compiler
ii  gcc-3.3 [c-compiler]         1:3.3.6-15  The GNU C compiler
ii  gcc-4.1 [c-compiler]         4.1.2-25    The GNU C compiler
ii  gcc-4.3 [c-compiler]         4.3.2-1.1   The GNU C compiler

Versions of packages libc6-dev suggests:
ii  glibc-doc                     2.7-18     GNU C Library: Documentation
ii  manpages-dev                  3.05-1     Manual pages about using GNU/Linux

-- no debconf information



Reply to: