Re: [bug #29755] gdomap information disclosure vulnerabilities

[Dan Rosenberg]

Fair enough, restricting configuration files to specific users
restricts functionality, so it's not a good solution.  But in
situations like this, it's better to be safe than sorry - even if
reporting line numbers seems harmless, it's hard to predict situations
in which even this may be useful information that an unprivileged user
shouldn't have the ability to acquire.  In fact, with the way this
code is written, a user may confirm the existence of files in
directories he does not have permission to access (because the fopen()
will succeed if the file exists, and fail if it doesn't), which your
proposed fix does not address.

There are other potential issues as well.  For example, in the
load_iface() function, the provided file is opened, then the number of
lines is counted, and memory is malloc'd as follows:

  addr = (struct in_addr*)malloc((num_iface+1)*IASIZE);
  mask = (struct in_addr*)malloc((num_iface+1)*IASIZE);
  bcok = (unsigned char*)malloc((num_iface+1)*sizeof(char));
  bcst = (struct in_addr*)malloc((num_iface+1)*IASIZE);

If an attacker provides a file (or socket) with a very large number of
lines such that (num_iface + 1) * IASIZE overflows and wraps around to
0, the subsequent code that uses this memory under the assumption that
it's big enough to contain num_iface + 1 structs may result in a
potentially exploitable heap overflow.

I think the easiest way to handle all of these issues is to remove the
setuid bit altogether, at least until a more thorough security review
is done.  I can't really see a reason why unprivileged users should be
able to launch this daemon anyway.

Thanks,
Dan

On Wed, May 5, 2010 at 11:56 AM, Richard Frith-Macdonald <rfm@gnu.org> wrote:
>
> On 5 May 2010, at 16:04, Dan Rosenberg wrote:
>
>> I'm still a bit unsure about this fix.  I think that it should either
>> be made explicit that gdomap is never intended to be installed setuid
>> (and as a result, is never installed that way), or it should be fixed
>> so that it's completely safe to run setuid - choosing somewhere in
>> between leaves some users open to vulnerability.
>
> I think the current state is that it's safe to run setuid to root.
> But ... that doesn't mean we should start recommending having it setuid.
>
>> I haven't seen the actual code of the fix, but it sounds as if it's a
>> good start but incomplete.  As Fred mentioned, unprivileged users
>> should not be able to open and parse other users' files at all, even
>> if the error information returned is limited.
>
> I'm not sure about that ... when there's no security issue in doing so, it 
> seems reasonable to be able to have different users share common 
> configuration information (after all, that's what group permissions are for). 
>  Forcing all users to have their own separate config files would rather 
> defeat the point.
>
> It's hard to see how the current code (reporting problem line number) could 
> provide useful information to a cracker, but I suppose we could simply report 
> nothing at all.