This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542)
- From: Paul Eggert <eggert at cs dot ucla dot edu>
- To: Martin Sebor <msebor at gmail dot com>, Carlos O'Donell <carlos at redhat dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Wed, 7 Feb 2018 11:52:56 -0800
- Subject: Re: [PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542)
- Authentication-results: sourceware.org; auth=none
- References: <f6a8c32f-f524-9ebb-03bc-4484f8a80a16@gmail.com> <2ab77d63-7f68-1c60-40d2-71a9bef5f21f@cs.ucla.edu> <20171204015531.GA8729@altlinux.org> <9588ad22-bb91-5364-2217-4813857ee529@cs.ucla.edu> <20180206210601.GA23713@altlinux.org> <11a4c422-d919-df63-8328-a7e17fef8ae8@cs.ucla.edu> <20180207105350.GA787@altlinux.org>
On 02/07/2018 02:53 AM, Dmitry V. Levin wrote:
I don't see why a valid code has to be maimed to cater for broken code.
The problem here is that there is a dispute over what is "valid" and
what is "broken". It's only a minor disagreement and shouldn't affect
typical usage, but it is a disagreement. A good course here is to follow
the usual advice about being strict about what you generate and generous
about what you accept. We don't want to penalize code that is taking
such an approach, in an attempt to slightly simplify code that is
zealously enforcing one side of the dispute.
I hope you don't really suggest people to apply changes like this > to their valid code:
No, because that particular code isn't valid and should be fixed anyway,
regardless of whether one assumes sun_path uses strncpy format. The
problem is that the code mistakenly assumes that 'socket' always creates
a file whose name is given by av[1] and then goes off and unlinks av[1]
to make room for the new file, but this is incorrect when av[1]'s length
exceeds sizeof(addr.sun_path). That is, this code:
assert(strlen(av[1]) > 0);
strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
len = offsetof(struct sockaddr_un, sun_path) + strlen(av[1]) + 1;
if (len > sizeof(addr))
len = sizeof(addr);
unlink(av[1]);
should be replaced by something like this code:
assert(0 < strlen(av[1]) && strlen(av[1]) < sizeof(addr.sun_path));
strcpy(addr.sun_path, av[1]);
len = offsetof(struct sockaddr_un, sun_path) + strlen(av[1]) + 1;
unlink(av[1]);
and once you do that, the problem goes away regardless of whether
sun_path uses strncpy format.
In short, this particular test case should be changed to be strict about
what it generates, and glibc should support this sort of usage instead
of introducing artificial roadblocks against it.