[nas] Improper fixes in the r288

[Erik Auerswald]

Hello Petr,

On 09/12/2013 04:46 PM, Petr Pisar wrote:
> I read some patches for the `Multiple Vulnerabilities in nas 1.9.3' issue and
> I think some changes are not sufficient.

Thanks for your review!

> E.g. here:
>
> --- server/os/access.c  (revision 287)
> +++ server/os/access.c  (revision 288)
> @@ -478,9 +478,9 @@
>           validhosts = host->next;
>           FreeHost(host);
>       }
> -    strcpy(fname, "/etc/X");
> -    strcat(fname, display);
> -    strcat(fname, ".hosts");
> +    strncpy(fname, "/etc/X", sizeof fname); fname[sizeof fname - 1] = '\0';
> +    strncat(fname, display, sizeof fname - strlen(fname) - 1);
> +    strncat(fname, ".hosts", sizeof fname - strlen(fname) - 1);
>       if (fd = fopen(fname, "r")) {
>           while (fgets(hostname, sizeof(hostname), fd)) {
>               if (ptr = index(hostname, '\n'))
>
> the `sizeof fname - strlen(fname) - 1' will result into -1 if previous command
> fills fname completely. And because the last argument is of type size_t that
> is unsigned, you will pass a very high positive number.

Well, 'sizeof fname' is the length of the buffer, let's call it L. The 
last element of the buffer is '\0'[1]. Thus any string inside the buffer 
is of length L-1. Thus:

sizeof fname - strlen(fname) - 1 >= L - (L-1) - 1 == 0

[1] fname[sizeof fname - 1] = '\0'

I have written a small example program to show this:

---8<---
~/tmp$ rm a.out
~/tmp$ cat a.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(void)
{
   char buf[10];
   char *too_long = "this string is too long for the buffer 'buf'";
   strncpy(buf, too_long, sizeof buf);
   buf[sizeof buf - 1] = '\0';

   printf("example string:                '%s'\n", too_long);
   printf("length of string:               %lu\n", strlen(too_long));
   printf("size of buf:                    %lu\n", sizeof buf);
   printf("contents of buf:               '%s'\n", buf);
   printf("length of string in buf:        %lu\n", strlen(buf));
   printf("'sizeof buf - strlen(buf) - 1': %lu\n",
          sizeof buf - strlen(buf) - 1);

   exit(EXIT_SUCCESS);
}
~/tmp$ gcc a.c
~/tmp$ ./a.out
example string:                'this string is too long for the buffer 
'buf''
length of string:               44
size of buf:                    10
contents of buf:               'this stri'
length of string in buf:        9
'sizeof buf - strlen(buf) - 1': 0
---8<---

> Moreover if the copied text gets clamped, you won't notice it and you will
> try to open wrong file in turn.

You are right. The server will try to read a non-existing or malformed 
file. But I think that is acceptable, i.e. no security problem. Since 
the code in question is used on AMOEBA only and thus not testable for 
me, I do not intend to change this.

> In this case, it would be much more elligible to use snprintf() and check the
> return value:
>
>    int r = snprinf(fname, sizeof fname, "/etc/X%s.hosts", display);
>    if (r < 0 || r >= sizeof fname))
>      FatalError();

I changed the code in a way that only the length of the buffers is 
checked. That can be done quite mechanically and verified mechanically 
as well. Changing the strcpy() + strcat() + strcat() to snprintf() is 
less "_obviously_ correct" IMHO.

> Or here:
>
> --- server/os/connection.c      (revision 287)
> +++ server/os/connection.c      (revision 288)
> @@ -2115,7 +2127,7 @@
>           if (AuServerHostName == NULL)
>               FatalError
>                       ("AUDIOHOST not set, or server host name not given\n");
> -        sprintf(host, "%s/%s:%s", DEF_AUSVRDIR, AuServerHostName,
> +        snprintf(host, sizeof host, "%s/%s:%s", DEF_AUSVRDIR, AuServerHostName,
>                   0 /* port */ );
>
>           uniqport(&Au.cap_port);
>
> the host will be undefined, if snprintf() cannot fit the string into the its
> size. You should check for sprintf clamp again.

The connection will probably not succeed in that case. Improving 
troubleshooting and debug friendliness is out-of-scope for the security 
fixes. Again, this affects AMOEBA only.

I do not have an AMOEBA machine to test on, so I want to change as 
little as possible of the AMOEBA specific code.

> -- Petr

Thanks again for the review!

Erik