This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542)


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]