Re: [bug #29755] gdomap information disclosure vulnerabilities

[Richard Frith-Macdonald]

On 5 May 2010, at 18:52, Dan Rosenberg wrote:

> 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.

Yep ... I changed it to simply report that a problem occurred ... which doesn't 
even provide information about whether the file exists or not.

> 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.

Thanks ...thats fixed simply by limiting the number of items read to a max of 
1000.

> 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.

Agreed, they normally shouldn't ... distributions are supposed to start the 
program at boot time and not install it setuid... the setuid functionality is 
for development systems, and to be honest I'm not sure how much that's needed 
any more.  Anyway, it's absolutely certain that distributions should not need 
to have it setuid because they should have started the daemon, removing the 
need for unprivileged users to start it.