[nas] nas: Multiple Vulnerabilities in nas 1.9.3

[Erik Auerswald]

Hi,

On 08/13/2013 08:45 PM, Jon Trulson wrote:
> On Tue, 13 Aug 2013, Erik Auerswald wrote:
>> On Tue, Aug 13, 2013 at 04:07:33PM +0200, Erik Auerswald wrote:
>>> On Mon, Aug 12, 2013 at 11:28:50AM -0600, Jon Trulson wrote:
>>>> On Mon, 12 Aug 2013, Erik Auerswald wrote:
>>>>> On Sun, Aug 11, 2013 at 10:09:13AM +0430, Hamid Zamani wrote:
>>>>>> On 08/10/2013 10:08 PM, Erik Auerswald wrote:
>>>>>>> I do not know how to determine if a given string is a valid or
>>>>>>> invalid TCP
>>>>>>> device on Minix.
>>>>>>
>>>>>> on Minix i checked the /dev/tcp :
> [...]
>>>>
>>>> Could you just check for a number instead?  Ie: Instead of TCP_DEVICE,
>>>> use something like TCP_DEVNUM, which if set, is expected to be some
>>>> (smallish) integer that can be tacked onto /dev/tcp in the code...?
>>>
>>> Do you have any references to TCP_DEVNUM? Quick googling did not turn
>>> up anything. I suppose that would be new, special environment variable
>>> just for nasd on Minix.
>>>
>
> No, it was something I just made up - I should have read that page in
> more detail - TCP_DEVICE is actually needed.  As you can see, I don't
> use Minix :)

Neither do I...

>>> I do not like to change from the usual TCP_DEVICE (see
>>> http://www.minix3.org/manpages/html4/ip.html) to something
>>> special just for NAS. I do not know if anybody at all uses NAS on
>>> Minix and
>>> I don't even compile the code in question (because of #ifdef).
>>>
>>> My research so far hints at TCP/IP devices having major 7
>>> (http://www.minix-vmd.org/pub/Minix-vmd/1.7.0/cat/cat4/dev.4), which
>>> could
>>> be checked as well if it still holds.
>>>
>>> Currently, Minix seems to be restricted to a maximum of 16 network
>>> devices,
>>> so /dev/tcp{,{0..15}} should describe the legal values for TCP_DEVICE.
>>> (see http://www.minix3.org/manpages/html8/inet.html)
>>>
>>> Anyway, the first the current code tries to do with the opened
>>> TCP_DEVICE
>>> is to an ioctl() call. If that fails, the file descriptor is closed. As
>>> long as ioctl() is not exploitable, this does not constitute a
>>> vulnerability.
>>>
>>> A sanity check of program input, in this case from an environment
>>> variable,
>>> is prudent. Verifying a prefix of "/dev/tcp" would go a long way.
>>
>> How about the following patch?
>
> I think this patch should be fine.  If someone can play a symlink
> attack here, then they already have root access and all is lost
> already :)

I don't really like my first patch and suggest the following (same idea, 
different realization).

---8<---
Index: server/os/connection.c
===================================================================
--- server/os/connection.c	(revision 288)
+++ server/os/connection.c	(working copy)
@@ -421,10 +421,16 @@

      /* Allow the audio server to run on a different IP device with the
       * TCP_DEVICE environment variable, otherwise we take the default.
+     * A different IP device is specified via its number concatenated
+     * to the TCP device name, so check that the default TCP device is
+     * a prefix of the value read from the TCP_DEVICE environment
+     * variable.
       */
      tcp_dev = getenv("TCP_DEVICE");
-    if (tcp_dev == NULL)
+    if (!tcp_dev || strncmp(tcp_dev, TCP_DEVICE, strlen(TCP_DEVICE))) {
+        fprintf(stderr, "Ignoring invalid TCP_DEVICE environment 
variable.\n");
          tcp_dev = TCP_DEVICE;
+    }

      fd = open(tcp_dev, O_RDWR);
      if (fd == -1) {
---8<---

[Patch looks white space damaged (fprintf() line) in my mail client.]

Please double-check the if condition, as str[n]cmp can be quite 
confusing. ;-)

Using osLogMsg() instead of fprintf() might be better, but I did not 
want to introduce this to connection.c without a Minix system to test 
the change.

Thanks,
Erik