7590 sendmsg on AF_UNIX socket fails after process drops privileges

Review Request #269 — Created Nov. 18, 2016 and submitted

gwr
illumos-gate
7590
general

7590 sendmsg on AF_UNIX socket fails after process drops privileges

We should not assume we can call so_ux_addr_xlate() in sendmsg,
because we may no longer have the privileges needed to lookup
the AF_UNIX name again. The problem here was with "connected"
datagram sockets, where we already know the peer address in
the internal form needed for sending. Just use it.

The test program attached to the issue.
Added some tests for this to $SRC/tests.
Ran this on a system using AF_UNIX stuff (X11)

This build clean, and tests work fine.
I'd like to RTI it. Can I get some "ship it"s?

  • 0
  • 0
  • 14
  • 4
  • 18
Description From Last Updated
rm
  1. I think this is a good start and conceptually makes sense as the way to tackle the issue of permissions and dropping privileges.
    1. Thanks for taking a look. I'll update the RB, but there are a few questions for you...

    2. Have I taken care of all the issues you raised?
      Would like to "move this along"...

    3. Hi Gordon, I have a few outstanding questions on some of the test bits, but otherwise the main parts here look good. Thanks.

  2. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    Are the errors that this is checking no longer relevant?
    1. I should have updated the prior comment too.
      Now that we're using the internal address here
      and not translating, that comment is stale.

      The error check was for the possibility the
      the translation from external to internal
      form might fail. No longer relevant.

      Can I just delete the comment above?
      2854-2858, or are you looking for more?

    2. Was mostly just looking to understand what had been going on.
  3. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    Can you expand this comment to note that we are only dealing with connected sockets due to earlier checks? This threw me off for a while.
  4. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    If we have a call to connect during this time, how do we guarantee that the contents here won't change? How do we make sure we don't grab data that's changing? It seems like we don't always hold the so_lock during all of these operations.
    1. It's the norm in this code to use these AF_UNIX socket addresses
      even while they may be "changing under foot". i.e. see comments
      (i.e. at socktpi.c line 3711) about the source address changing.

      If the destination address changes under foot, the worst that
      happens is we attempt sending the datagram to some grabled
      destination that doesn't exist, so it ends up getting dropped.
      That would only happen with a concurrent connect and sendmsg
      on the same socket, so I'm not worried about it.

    2. OK, I see. I guess this is one of those things that's up to the user, unfortunately. Regardless of what'd make the most sense, it doesn't make sense to tie into this for now.

  5. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    It looks like it's only true for TCP and UDP, not for all IPv4 and IPv6.
    1. I was just trying to clarify that AF_UNIX does not go down that path.
      Should I delete the comment? or say "Not for AF_UNIX"?

    2. The change is good, thanks.

  6. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    This comment doesn't help. Can you elaborate somewhere on the semantics of this flag?
    1. Yeah, I'll take a stab at better comments on that.
      How about in the comment above sotpi_sendmsg?

    2. Thanks, what you have now is useful.

  7. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    If anything has set this flag, shouldn't that be an error? Why do we just mask it out and ignore garbage (likely from userland)?
    1. I thought of adding flags validation here (i.e. return EINVAL on bogus flags) but the code didn't do that before, so it seemed "off topic" to change that as part of this work.
      If we're contining to ignore invalid flags, we need to clear this "internal-only" one.

    2. Hmm. Fair enough, I think that makes sense, unfortunately. With our luck, stricter checking would now break a lot of stuff.
  8. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    It would help if you explain in the comment why we don't need to do the translation at this point.
    1. Yeah, I'll put a longer explanation in the block above this funciton,
      and perhaps a reference to it a these lines.

  9. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)
     
     
    cstyle nit: space between cast.
    1. Huh. I ran cstyle...

  10. usr/src/uts/common/sys/socket.h (Diff revision 1)
     
     
    It may be worth adding here that callers can't set this, at least as described here. It doesn't seem correct to say that this is for kernel sockets only. As kernel sockets don't set this, rather it's being used internally by the framework.
    1. Yeah... all of these are "internal use only" (as opposed to being specifically for ksockets).
      You OK with the comment changing to something like "Only used internally in the kernel"?

    2. Yeah, that's fine. Thanks.

  11. 
      
gwr
gwr
yyang
  1. 
      
  2. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 3)
     
     
     

    name is defined as struct sockaddr *, so it's just confusing to assign internal form address to name and pass it down to sosend_dgram() and sosend_dgramcmsg(). How about just setting MSG_SENDTO_NOXLATE flag here? name is still assigned sti->sti_faddr_sa, and then inside sosend_dgram() and sosend_dgramcmsg(), if flag MSG_SENDTO_NOXLATE is set, get internal form address from sti->sti_ux_faddr?

    I've tested this change using Samba 4.4.7 and it worked well. By the way, I was the bug reporter for the following issue:

    https://bugzilla.samba.org/show_bug.cgi?id=12429

    1. I see what you mean, but here the passed in name==NULL, and the functions sosend_dgram(), sosend_dgramcmsg() assert name!=NULL and I didn't really want to change that. That's why I chose to largely "borrow" the logic around the sti->>sti_faddr_noxlate flag.

    2. Sorry for not stating clearly, I meant changing that block like the following:

              if (so->so_family == AF_UNIX) {
                  flags |= MSG_SENDTO_NOXLATE;
              }
              ASSERT(sti->sti_faddr_sa);
              name = sti->sti_faddr_sa;
              namelen = (t_uscalar_t)sti->sti_faddr_len;
      
    3. Oh, I see. In the AF_UNIX case, sti_faddr_sa needs to be the "external" name, so that getpeername works correctly.
      I don't think we can easily put the internal form there, which is why this uses sti_ux_faddr instead.

    4. I am not seeing why it won't work:

      @@ -3702,6 +3704,15 @@ sosend_dgramcmsg(struct sonode so, struct sockaddr name, socklen_t namelen,
      addrlen = namelen;
      src = NULL;
      srclen = 0;
      + } else if (flags & MSG_SENDTO_NOXLATE) {
      + /
      + * Have an internal form dest. address.
      + * Pass the source address as usual.
      +
      /
      + addr = &sti->sti_ux_faddr;
      + addrlen = sizeof (sti->sti_ux_faddr);
      + src = sti->sti_laddr_sa;
      + srclen = (socklen_t)sti->sti_laddr_len;
      } else {
      /
      * Pass the sockaddr_un source address as an option
      @@ -4022,6 +4048,15 @@ sosend_dgram(struct sonode
      so, struct sockaddr name, socklen_t namelen,
      addrlen = namelen;
      src = NULL;
      srclen = 0;
      + } else if (flags & MSG_SENDTO_NOXLATE) {
      + /

      + * Have an internal form dest. address.
      + * Pass the source address as usual.
      + /
      + addr = &sti->sti_ux_faddr;
      + addrlen = sizeof (sti->sti_ux_faddr);
      + src = sti->sti_laddr_sa;
      + srclen = (socklen_t)sti->sti_laddr_len;
      } else {
      /

      * Pass the sockaddr_un source address as an option
      Thanks.

    5. I'm finding it difficult to understand your proposed modification. Could you perhaps send me a complete patch?
      Also, my initial impression is that the change looks equivalent. What makes it better?
      Thanks.

    6. Hi Gordon,

      Sorry for the confusion. I've sent you the complete patch. You're right, technically what I proposed is equivalent to yours, but it will make the code less confusing.

      Thanks,

      --Youzhong

    7. With your patch I now see what you're getting at.
      Please see the comment I added at socktpi.c line 3713

  3. 
      
gwr
  1. 
      
  2. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 3)
     
     

    Youzhong Yang suggested an alternative here:

        addr = &sti->sti_ux_faddr;
        addrlen = sizeof (sti->sti_ux_faddr);
    

    That would be equivalent, but it would mean that the MSG_SENDTO_NOXLATE would have the additional semantics of ignoring the passed name. I don't think I like that better than passing in name set to the same value, which follows that pattern established with the sti_faddr_noxlate flag.

  3. 
      
gwr
gwr
gwr
rm
  1. 
      
  2. Why is this commented out? Should we have all of these in gate or not? Or is there something special about them that causes it not to be able to operate?
    1. That was an oversight. Will fix. Thanks.

  3. Something tells me this line isn't right. ;)
    1. Well, I copied the file from ../poll/Makefile
      (if I recall correctly:)
      When we copy things, we're supposed to keep the top matter...
      Is there something else you'd like to see there?

    2. OK, well, if you did just copy and not write it, then I guess so.

  4. Would we expect listen to succeed multiple times? I'm not sure I understand why this is a while loop.
    1. That's just (in general) what a listener should do.
      Yes, in this test there will be only one connect.
      Any reason to do this differently?

    2. Generally, I would expect a listener to call listen once, but to call accept once per connection in a loop. However, digging into the source a bit more, it looks like we'll let you call listen() multiple times in a row to potentially change the backlog value. It's weird and not something you find in most network code, but not something worth arguing over and it looks like it's not going to break anything.
      
      I would just say that if it's important for the test to call listen() multiple times for some of the privilege checking, you probably want to make sure that's commented, because that's the kind of thing most folks would be confused by.
    3. D'oh! I looked at that multiple times and my brain saw an "accept" loop, not a "listen" loop.
      Sorry. Yeah, I'll fix that.

  5. Missing new line between return value and type? This is true of multiple functions here.
    1. Huh. I thought I c-styled all of this.
      I'll check again. Thanks.

  6. 
      
gwr
gwr
gwr
gwr
tsoome
  1. Ship It!
    1. Reading this does make a sense, but my experience about this area is quite low so the "weight" of this "shipit!" is probably not that high;)

  2. 
      
yuripv
  1. 
      
  2. Move these to Makefile?

  3. Make this a proper comment and don't silence cstyle? :-) also s/Illumos/illumos/.

    1. Well... as this is someone else's work, I was trying to
      "keep a light touch" on it. Does this bother you a lot?

    2. I decided to go ahead and cleanup that comment.
      A lot of it no longer really applied to our use here.

  4. What is this about? If we aren't going to use these fields, just remove them; if they are showing the problem, remove the #if 0, so that problem is visible (even if it makes the test case fail).

    1. Ditto. It's really documentation about this particular
      use of the send vs sendto interfaces.

      Can you suggest a way to preserve the "documenting"
      aspect of this while fixing whatever is offensive here?

      If I change that to a plain old comment, would you
      find that satisfactory?

  5. usr/src/uts/common/fs/sockfs/socktpi.h (Diff revision 8)
     
     

    s/is hold/helds/?

  6. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 8)
     
     

    efficient

  7. usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 8)
     
     

    subtleties

  8. 
      
gwr
yuripv
  1. Looks good as far as I understand it.

  2. 
      
gwr
rm
  1. Ship It!
  2. 
      
tsoome
  1. Ship It!
  2. 
      
gwr
Review request changed

Status: Closed (submitted)

Loading...