Re: [patch] Fix BZ #18660 -- overflow in getusershell

[Tolga Dalman <tolga dot dalman at googlemail dot com>]

Mike,

your proposal looks good to me (I read it and wrote a small test program).
In fact, I see it as a great improvement to the status quo.

Have you integrated your version and tried to add a test case as proposed earlier ?

Best regards
Tolga Dalman


On 08/19/2015 08:27 PM, Mike Frysinger wrote:
> On 19 Aug 2015 09:28, Paul Pluzhnikov wrote:
>> On Wed, Aug 19, 2015 at 7:55 AM, Mike Frysinger wrote:
>>> /* These functions are not thread safe (by design), so globals are OK.  */
>>> static FILE *shellfp;
>>> static int okshell_idx;
>>> static char *shellbuf;
>>> static size_t buf_len;
>>>
>>> char *
>>> getusershell (void)
>>> {
>>>   /* Handle the builtin case first.
>>>
>>>      NB: we do not use a global array here as the initialization needs
>>>      relocations.  These interfaces are used so rarely that this is not
>>>      justified.  Instead open code it with a switch statement.  */
>>>   switch (okshell_idx)
>>>     {
>>>     case 0:
>>>       /* Require the user call setusershell first.  */
>>>       assert (shellfp != NULL);
>>
>> I could find no man page that requires user calling setusershell first.
>> Is there a definitive description of this interface?
> 
> hmm, looks like freebsd, openbsd, darwin, and gnulib don't require this.
> i'll drop that.
> 
>>>   while (getline (&shellbuf, &buf_len, shellfp) != -1)
>>>     {
>>>       char *p;
>>>
>>>       /* Strip out any comment lines.  */
>>>       p = strchr (shellbuf, '#');
>>>       if (p)
>>>         *p = '\0';
>>>       else
>>>         {
>>>           /* Chop the trailing newline.  */
>>>           p = strchr (shellbuf, '\n');
>>
>> Current version chops on white space. Do we want to return "/foo bar"
>> here? Of course any sysadmin who puts "/foo bar" into /etc/shells gets
>> what he deserves.
> 
> the old logic is even worse than that.  it skips all leading bytes until it
> finds a slash and then returns things after that, while splitting on space.
> so a line like:
> 	space/foobar cow
> will return "/foobar".
> 
> the docs i've seen (darwin, freebsd, linux) leave it fairly vague and say
> "valid user shell".  but no one validates things, and really only document
> comments and blank lines.  i think we should just go with that.
> 
>>>       /* Only accept valid shells (which we define as starting with a '/').  */
>>>       if (shellbuf[0] == '/')
>>
>> Do we want to return "/" as a valid shell here?
> 
> the current already does :).  since we aren't checking the path in the file 
> (i.e. running stat and seeing if it's a +x file), i don't think we want to
> treat / specially.  afterall, "/bogus" or "/bin" or "/sbin/" will still get
> returned.
> 
>>> void
>>> setusershell (void)
>>> {
>>>   /* We could rewind shellfp here, but we get smaller code this way.
>>>      Keep in mind this is not a performance sensitive API.  */
>>>   endusershell ();
>>>
>>>   shellfp = fopen (_LOCAL_PATH_SHELLS, "rce");
>>>   if (shellfp == NULL)
>>>     okshell_idx = 1;
>>
>> I think you missed a part here:
>>
>>   else
>>     okshell_idx = 4;
> 
> nope ... current code is correct.  see how the value of 0 is treated (which is 
> also the default since it's a bss var).
> -mike
>