[nas] nas: Multiple Vulnerabilities in nas 1.9.3

[Erik Auerswald]

Hi,

On 08/10/2013 05:38 PM, Hamid Zamani wrote:
> On 08/10/2013 01:51 AM, Erik Auerswald wrote:
>> See the attached patch for replacing unsafe uses of (v)sprintf and
>> strc(py|at) with the respective n-versions.
>>
>> Anyone, please test and review the patch. Are the n-versions widely
>> available? Do we care about systems not providing them? Can we use
>> configure to check availability of them?
>
> I tested the patch and updated the sources. everything looks good.

Thanks for testing!

> At this time it seems most of systems provide these functions, but
> because of system variety in nas , i think dependency checking is
> recommended.
>
> I believe that this is completely possible in `autoconf` and i think it
> should be done as a REQUIREMENT.
>
>> If the attached patch is OK there is still one issue open from your list:
>>
>> --- begin quote ---
>> ========================================================================
>> Possible Race Condition and symlink attack:
>>
>> os/connection.c:
>>
>>      tcp_dev = getenv("TCP_DEVICE"); // Need check
>>      if (tcp_dev == NULL)
>>          tcp_dev = TCP_DEVICE;
>>
>>      fd = open(tcp_dev, O_RDWR); // O_RDWR => results corrupting data
>> --- end quote ---
>>
>> The NAS clients have instances of unsafe string functions as well. That
>> should be audited... any volunteers?
>>
>
> We should check the getenv output always and can't trust on this.
>
> Erik, should we get these from env ? at least program's arg is a better
> choice. of course if i knew the code correctly.

Well, that is a bit tricky...

The code in question is used on Minix only. This seems to be a Minix 
idiom for opening a TCP connection (see 
http://www.cise.ufl.edu/~cop4600/cgi-bin/lxr/http/ident.cgi?i=tcp_device 
or http://users.sosdg.org/~qiyong/mxr/ident?i=TCP_DEVICE).

On Minix, TCP_DEVICE is #defined to "/dev/tcp" (see 
http://users.sosdg.org/~qiyong/mxr/source/include/net/netlib.h#L11). I 
do not know how to determine if a given string is a valid or invalid TCP 
device on Minix.

I am inclined to either leave this as is, or comment out the getenv() 
call and always use 'tcp_dev = TCP_DEVICE'. The latter is safe, but 
might break some usage. The former might be a risk every Minix user is 
aware of and willing to take.

Is there anybody using NAS on Minix out there? If so, please speak up!

> I've just started checking clients's codes and it seems there are some
> other flaws. i'll send them soon.

Thanks,
Erik