On 05/18/2011 06:02 PM, Stephen Gallagher wrote:
On Wed, 2011-05-18 at 17:48 +0200, Jakub Hrozek wrote:
>> Patch 0003: Nack. One last minor change. I realized that
>> nss_get_etc_shells() could be vulnerable to a race-condition attack. If
>> an administrator accidentally had the wrong permissions set
>> on /etc/shells, then it would be possible for an attacker to monitor
>> that file for accesses with inotify and replace its contents between the
>> initial read for length and the second read for contents. Thus, a buffer
>> overrun could be exploited.
>>
>
> Nice catch..
>
>> I think it would be wiser for us to do buffered talloc_realloc()'s (say,
>> five at a time) and run through the getusershell() loop only once.
>> Probably putting an upper limit of, say 50 shells just to be safe.
>>
>>
>
> Done. I love working around terrible legacy interfaces.
Nack.
Please do the check for SHELL_REALLOC_MAX before actually performing the
realloc. Right now it happens to work okay because SHELL_REALLOC_MAX is
a multiple of SHELL_REALLOC_INCREMENT, but if we ever changed one or the
other, it could cause problems. Also, if we DO have more than 50 shells,
you've just allocated space unnecessarily for 55 shells.
Also, minor nitpick, could you please reduce the "Found shell" debug
level to 6 to match the level at which we print other options?
One of these days we really need to write down a formal debug level
specification. Right now it's pretty much unregulated.
attached