Patchwork Fix handling of overlength pathname in AF_UNIX sun_path

login
register
mail settings
Submitter Carlos O'Donell
Date April 18, 2012, 4:08 a.m.
Message ID <CADZpyix6DZ93f8MQf3Aa1NVV7HCFMAXVAdzRMFBY7xWHHQMPog@mail.gmail.com>
Download mbox | patch
Permalink /patch/153376/
State RFC
Delegated to: David Miller
Headers show

Comments

Carlos O'Donell - April 18, 2012, 4:08 a.m.
On Tue, Apr 17, 2012 at 10:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Michael Kerrisk <mtk.manpages@gmail.com>
> Date: Tue, 17 Apr 2012 22:44:15 +1200
>
>> 1. Changing the kernel behavior prevents userspace having
>>    to go through the sort of contortions described above
>>    in order to handle the 108-non-NUL-bytes-in-sun_path case.
>
> The problem with this logic is that it ignores every single Linux
> system that exists right now.
>
> You need to code this logic into your application unless you don't
> want it to run properly on every Linux system that actually exists.
>
> Sorry, we're not making this change.

Dave,

I don't clearly understand your position here, and perhaps that's my
own ignorance, but could you please clarify, with examples, exactly
why the change is not acceptable? I can see several valid arguments
against the change, but I don't know which argument your position
asserts.

One might assert that careless userspace applications exist that pass
`sizeof(struct sockaddr_un)' (or worse) as the 3rd argument to bind
instead of SUN_LEN(my_sock). The logic in the patch doesn't account
for this, and can't really, and would therefore unacceptably break
existing applications by trying to assert the location of a \0 where
one doesn't exist. The kernel must therefore continue to
null-terminate at the specified length, possibly the 109th character,
and use strlen to capture the true length of the path. The kernel
knows that in the worst case a non-null terminated path might contain
some garbage, but that's the users fault, and the logic to prevent
this must exist in the application not the kernel.

The counter argument to all of this is that it's a QoI issue, and that
the kernel shouldn't accept accidentally non-null terminated paths,
and should instead return EINVAL for them. Not to mention that it's
difficult for userspace to easily catch this error in glibc which
would need to inspect the sockaddr, duplicating kernel code.

Why is it valid for the user path to have no null terminator?

Why not have:


---

Does that make sense?

Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 18, 2012, 4:16 a.m.
From: "Carlos O'Donell" <carlos@systemhalted.org>
Date: Wed, 18 Apr 2012 00:08:47 -0400

> I don't clearly understand your position here, and perhaps that's my
> own ignorance, but could you please clarify, with examples, exactly
> why the change is not acceptable?

My position is that since millions upon millions of Linux systems, in
fact every single Linux system, exists right now with the current
behavior we are not helping application writers at all by changing
behavior now after it's been this way for nearly 20 years.

Because if an application writer wants his code to work on systems
that actually exist he has to accomodate the non-NULL termination
situation if he wants to inspect or print out an AF_UNIX path.

Because every system in existence right now allows the non-NULL
terminated AF_UNIX paths, therefore it's possible on every system
in existence right now.

Catch my drift?

The very thing the patch claims to help, it doesn't.  We install this
kernel patch now and then tell application writers that they can just
assume all AF_UNIX paths are NULL terminated when they want to print
it out, because such code will not actually be guarenteed to work on
all deployed Linux machines out there.

You cannot just ignore 20 years of precedence and say "oh let's change
this in the kernel now, and that way application writers don't have to
worry about that lack of NULL termination any more."  It simply
doesn't work like that.

All of this talk about whether applications actually create non-NULL
terminated AF_UNIX paths don't even factor into the conversation.

So the value proposition for this patch simply does not exist.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - April 18, 2012, 8:17 a.m.
> 
> Why not have:
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index d510353..f9f77a7 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un
> *sunaddr, int len, unsigned *hashp)
>                  */
>                 ((char *)sunaddr)[len] = 0;
>                 len = strlen(sunaddr->sun_path)+1+sizeof(short);
> +               /* No null terminator was found in the path. */
> +               if (len > sizeof(*sunaddr))
> +                       return -EINVAL;
>                 return len;

That could generate a kernel page fault!
(Depending on what follows (or rather doesn't follow!) sun_path.)
You'd need to use memchr() not strlen().

	David


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Carlos O'Donell - April 18, 2012, 12:57 p.m.
On Wed, Apr 18, 2012 at 12:16 AM, David Miller <davem@davemloft.net> wrote:
> From: "Carlos O'Donell" <carlos@systemhalted.org>
> Date: Wed, 18 Apr 2012 00:08:47 -0400
>
>> I don't clearly understand your position here, and perhaps that's my
>> own ignorance, but could you please clarify, with examples, exactly
>> why the change is not acceptable?
>
> My position is that since millions upon millions of Linux systems, in
> fact every single Linux system, exists right now with the current
> behavior we are not helping application writers at all by changing
> behavior now after it's been this way for nearly 20 years.
>
> Because if an application writer wants his code to work on systems
> that actually exist he has to accomodate the non-NULL termination
> situation if he wants to inspect or print out an AF_UNIX path.
>
> Because every system in existence right now allows the non-NULL
> terminated AF_UNIX paths, therefore it's possible on every system
> in existence right now.
>
> Catch my drift?
>
> The very thing the patch claims to help, it doesn't.  We install this
> kernel patch now and then tell application writers that they can just
> assume all AF_UNIX paths are NULL terminated when they want to print
> it out, because such code will not actually be guarenteed to work on
> all deployed Linux machines out there.
>
> You cannot just ignore 20 years of precedence and say "oh let's change
> this in the kernel now, and that way application writers don't have to
> worry about that lack of NULL termination any more."  It simply
> doesn't work like that.
>
> All of this talk about whether applications actually create non-NULL
> terminated AF_UNIX paths don't even factor into the conversation.
>
> So the value proposition for this patch simply does not exist.

Thank you, this is the kind of position statement I can point to if I
ever get asked about this again.

In summary your opinion is that the API has and always will allow up
to 108 chars to be used in sun_path?

In which case I will talk to the Austin group to get a good example
added to POSIX showing safe usage.

Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thadeu Lima de Souza Cascardo - April 18, 2012, 1:13 p.m.
On Wed, Apr 18, 2012 at 09:17:26AM +0100, David Laight wrote:
> 
> > 
> > Why not have:
> > 
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index d510353..f9f77a7 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un
> > *sunaddr, int len, unsigned *hashp)
> >                  */
> >                 ((char *)sunaddr)[len] = 0;
> >                 len = strlen(sunaddr->sun_path)+1+sizeof(short);
> > +               /* No null terminator was found in the path. */
> > +               if (len > sizeof(*sunaddr))
> > +                       return -EINVAL;
> >                 return len;
> 
> That could generate a kernel page fault!
> (Depending on what follows (or rather doesn't follow!) sun_path.)
> You'd need to use memchr() not strlen().
> 
> 	David
> 

Hi, David.

What follows is a 0 byte, because it's set that way in the line before
strlen. Note that len is tested for sizeof(*sunaddr), and there is a
huge comment about that extra byte that was omitted.

The whole function is at net/unix/af_unix.c:203.

Regards,
Cascardo.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 18, 2012, 5:31 p.m.
From: "Carlos O'Donell" <carlos@systemhalted.org>
Date: Wed, 18 Apr 2012 08:57:58 -0400

> In summary your opinion is that the API has and always will allow up
> to 108 chars to be used in sun_path?

Yes.

> In which case I will talk to the Austin group to get a good example
> added to POSIX showing safe usage.

Why would you add language to POSIX for Linux specific behavior?
Just curious :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Carlos O'Donell - April 18, 2012, 6:48 p.m.
On Wed, Apr 18, 2012 at 1:31 PM, David Miller <davem@davemloft.net> wrote:
> From: "Carlos O'Donell" <carlos@systemhalted.org>
> Date: Wed, 18 Apr 2012 08:57:58 -0400
>
>> In summary your opinion is that the API has and always will allow up
>> to 108 chars to be used in sun_path?
>
> Yes.
>
>> In which case I will talk to the Austin group to get a good example
>> added to POSIX showing safe usage.
>
> Why would you add language to POSIX for Linux specific behavior?
> Just curious :-)

Why not? Do you ever feel crazy when people question what you think is
perfectly reasonable? ;-)

POSIX doesn't exist in a vacuum, we need to harmonize reality with the
standard. If an implementation exists where sun_path has no
null-terminator then it is useful to have POSIX clarify that
null-termination is implementation defined behaviour, just like it
says that sun_path's length undefined. Under "Application Usage" or
"Examples" it's valid to talk about specific implementations.

See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html,
where it talks about BSD in the "Application Usage." It's about time
we some "Linux this" and "Linux that" in there.

Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 18, 2012, 7:23 p.m.
From: "Carlos O'Donell" <carlos@systemhalted.org>
Date: Wed, 18 Apr 2012 14:48:43 -0400

> POSIX doesn't exist in a vacuum, we need to harmonize reality with the
> standard. If an implementation exists where sun_path has no
> null-terminator then it is useful to have POSIX clarify that
> null-termination is implementation defined behaviour, just like it
> says that sun_path's length undefined. Under "Application Usage" or
> "Examples" it's valid to talk about specific implementations.
> 
> See: http://pubs.opengroup.org/onlinepubs/007904975/basedefs/sys/un.h.html,
> where it talks about BSD in the "Application Usage." It's about time
> we some "Linux this" and "Linux that" in there.

Ok, thanks for explaining.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Kerrisk - April 18, 2012, 10:50 p.m.
On Wed, Apr 18, 2012 at 4:16 PM, David Miller <davem@davemloft.net> wrote:
> From: "Carlos O'Donell" <carlos@systemhalted.org>
> Date: Wed, 18 Apr 2012 00:08:47 -0400
>
>> I don't clearly understand your position here, and perhaps that's my
>> own ignorance, but could you please clarify, with examples, exactly
>> why the change is not acceptable?
>
> My position is that since millions upon millions of Linux systems, in
> fact every single Linux system, exists right now with the current
> behavior we are not helping application writers at all by changing
> behavior now after it's been this way for nearly 20 years.
>
> Because if an application writer wants his code to work on systems
> that actually exist he has to accomodate the non-NULL termination
> situation if he wants to inspect or print out an AF_UNIX path.
>
> Because every system in existence right now allows the non-NULL
> terminated AF_UNIX paths, therefore it's possible on every system
> in existence right now.
>
> Catch my drift?
>
> The very thing the patch claims to help, it doesn't.  We install this
> kernel patch now and then tell application writers that they can just
> assume all AF_UNIX paths are NULL terminated when they want to print
> it out, because such code will not actually be guarenteed to work on
> all deployed Linux machines out there.

Hang on a moment. I did not suggest that we can just tell users they
can forget about the past. Obviously, users will need to program to
past kernel behavior here for a good long time yet. (As Alan says
elsewhere in the thread "they'll be defensively coding for
the existing API for another ten years for enterprise distros
anyway".) However, this is about longer-term improvement of the
quality of implementation; in X years (choose your X) time, a lot of
new application may not need to care about the old broken behavior.
See some related examples below.

And you skipped past my other two points. Even if my understanding
about POSIX mandates is correct, I can understand how we might ignore
that point. But the last one is still germane:

[[
3. Considering these two sets:

  (a) [applications that rely on the assumption that there
       is a null terminator inside sizeof(sun_path) bytes]
  (b) [applications that would break if the kernel behavior changed]

  I suspect that set (a) is rather larger than set (b)--or, more
  likely still, applications ensure they go for the lowest common
  denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
  bytes, and so avoid this issue completely.
]]

There may well be potential breakages out there in set (a), and
improving the QOI would help them. (To put things in terms of Alan's
response: I suspect that there may well be existing applications that
are *not* defensively handling the existing API).

Taking the logic you've posed (my reading: "we shouldn't fix old
brokenness because applications will still need to code to the
brokenness") to the extreme, we'd *never* fix old pieces of
brokenness. However, we certainly have precedents for doing exactly
that:

After nearly 15 years of brokenness (stretching back to the first
kernels), commit 69be8f189653cd81aae5a74e26615b12871bb72e fixed this
(sigaction(2)):

   BUGS
       In kernels up to and including 2.6.13, specifying SA_NODEFER in
       sa_flags  prevents  not  only  the  delivered signal from being
       masked during execution of the handler, but  also  the  signals
       specified in sa_mask.  This bug was fixed in kernel 2.6.14.

Similarly, after brokenness that had run through the entire preceding
2.4.x kernel series, Linux 2.6.4 fixed this:

    BUGS
       In kernel 2.4 (and earlier) there is some  strangeness  in  the
       handling  of  X_OK  tests  for superuser.  If all categories of
       execute permission are disabled for a nondirectory  file,  then
       the  only  access() test that returns -1 is when mode is speci‐
       fied as just X_OK; if R_OK or W_OK is also specified  in  mode,
       then  access() returns 0 for such files.  Early 2.6 kernels (up
       to and including 2.6.3) also behaved in the same way as  kernel
       2.4.

(A little background here:
http://thread.gmane.org/gmane.linux.kernel/158814, and the fix
eventually went in with
http://thread.gmane.org/gmane.linux.kernel/178719)

> You cannot just ignore 20 years of precedence and say "oh let's change
> this in the kernel now, and that way application writers don't have to
> worry about that lack of NULL termination any more."  It simply
> doesn't work like that.

As should be clear from the above, I agree. But still, I don't think
the logic "it's broken, and even if we fix it, users will still have
to code to the old brokenness" is a sufficient argument against
improving the QOI long-term.

> All of this talk about whether applications actually create non-NULL
> terminated AF_UNIX paths don't even factor into the conversation.
>
> So the value proposition for this patch simply does not exist.

Of course, it's your call in the end, but I don't think things are as
cut-and-dried as your response suggests.

Cheers,

Michael
David Miller - April 18, 2012, 11:31 p.m.
From: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Date: Thu, 19 Apr 2012 10:50:40 +1200

> However, this is about longer-term improvement of the quality of
> implementation; in X years (choose your X) time, a lot of new
> application may not need to care about the old broken behavior.

There is really no value to this, the AF_UNIX NULL termination issue
is significantly different from the signal examples you mention.

If we're going to, like Carlos will, make mention in POSIX documents
that one must account for possible lack of NULL termination, there
is absolutely ZERO value in changing things because we are telling
application writers the state of reality which is that they have
to allot for this.

Please drop this issue, the discussion was over a long time ago, thank
you very much.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - April 19, 2012, 10:19 a.m.
> 3. Considering these two sets:
> 
>   (a) [applications that rely on the assumption that there
>        is a null terminator inside sizeof(sun_path) bytes]
>   (b) [applications that would break if the kernel behavior changed]
> 
>   I suspect that set (a) is rather larger than set (b)--or, more
>   likely still, applications ensure they go for the lowest common
>   denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
>   bytes, and so avoid this issue completely.

Or another way of putting it

3(a)	Sloppy coding that may have lots of other bugs

3(b)	Interfaces and code we promised not to break.



Alan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Kerrisk - April 19, 2012, 10:33 a.m.
On Thu, Apr 19, 2012 at 10:19 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> 3. Considering these two sets:
>>
>>   (a) [applications that rely on the assumption that there
>>        is a null terminator inside sizeof(sun_path) bytes]
>>   (b) [applications that would break if the kernel behavior changed]
>>
>>   I suspect that set (a) is rather larger than set (b)--or, more
>>   likely still, applications ensure they go for the lowest common
>>   denominator limit of 92 (HP-UX) or 104 (historical BSD limit)
>>   bytes, and so avoid this issue completely.
>
> Or another way of putting it
>
> 3(a)    Sloppy coding that may have lots of other bugs
>
> 3(b)    Interfaces and code we promised not to break.

Yes, it's another way of putting it. (Though regarding 3(b), part of
the problem is that there never was a clearly specified contract.)

Anyway, I've dug deeper, looking at hat happens on other platforms.
It's a mess: the BSDs don't even guarantee  that sun_path is
null_terminated. So, here's how one has to portably deal with the
variations:

addrlen = sizeof(struct sockaddr_un);
cfd = accept(lfd, &addr, &addlen);

printf("%.*s", addrlen - offsetof(struct sockaddr_un, sun_path), addr.sun_path);

That's pretty hideous!

Thanks,

Michael
Jan Engelhardt - April 19, 2012, 12:11 p.m.
On Thursday 2012-04-19 12:33, Michael Kerrisk (man-pages) wrote:
>
>Anyway, I've dug deeper, looking at hat happens on other platforms.
>It's a mess: the BSDs don't even guarantee  that sun_path is
>null_terminated. So, here's how one has to portably deal with the
>variations:
>
>addrlen = sizeof(struct sockaddr_un);
>cfd = accept(lfd, &addr, &addlen);
>
>printf("%.*s", addrlen - offsetof(struct sockaddr_un, sun_path), addr.sun_path);

What operating system made you write that?


I just ask for fun and the record. Solaris's dirent is also
constructed such that using sizeof(...) is useless.

typedef struct dirent {
        ino_t           d_ino;          /* "inode number" of entry */
        off_t           d_off;          /* offset of disk directory entry */
        unsigned short  d_reclen;       /* length of this record */
        char            d_name[1];      /* name of file */
} dirent_t;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index d510353..f9f77a7 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -216,6 +216,9 @@  static int unix_mkname(struct sockaddr_un
*sunaddr, int len, unsigned *hashp)
                 */
                ((char *)sunaddr)[len] = 0;
                len = strlen(sunaddr->sun_path)+1+sizeof(short);
+               /* No null terminator was found in the path. */
+               if (len > sizeof(*sunaddr))
+                       return -EINVAL;
                return len;
        }