On Wed, Aug 10, 2011 at 17:39 +0200, Peter J. Philipp wrote:
> Hi,
>
> I'm reading through lib/libc/gen/getpwent.c and I found this right at the
> end of the file:
>
> ---
> /* See if there's any data left. If so, read in flags. */
> if (data.size > (p - (char *)data.data)) {
> bcopy(p, (char *)flagsp, sizeof(int));
> p += sizeof(int);
> } else
> *flagsp = _PASSWORD_NOUID|_PASSWORD_NOGID; /* default */
> ---
>
> If the 'if' statement is true the data.data's size can be 1 byte, 2 byte,
> 3 byte, 4 byte (or more) less than data.size. Then comes along bcopy and
> assumes that data.size is sizeof(int) (4 bytes). Wouldn't it make more
> sense to put
>
> if (data.size - (p - (char *)data.data) >= sizeof(int))
>
> so that we're careful with how much we're bcopy'ing over? AFAIK it's not
> exploitable but if there is any 'dirt' in the database less than those 4
> bytes but more to qualify the if as true then could bcopy cause a segmentation
> violation?
>
i think it's safe as long as pwd_mkdb reserves an integer for flags:
memmove(p, &flags, sizeof(int));
p += sizeof(int);
data.size = p - buf;
> Cheers,
>
> -peter
