[nas] Improper fixes in the r288

[Petr Pisar]

On Sat, Sep 14, 2013 at 09:24:20PM +0200, Erik Auerswald wrote:
> >--- 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
> 
That's right. I mistaken the strlen() return value somehow.

> >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.
>
Similar issue is with unlinking files on different places. But I agree that
the severity is not as high. And if you think it's not worth to fix it as part
of this security patches, I'm not going to argue.

> >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.
> 
Matter of personal taste.

Thank you for handling my spots. I'm glad the NAS is not dead yet.

-- Petr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 230 bytes
Desc: not available
URL: <http://radscan.com/pipermail/nas/attachments/20130916/2dce7f62/attachment.pgp>