Bug 144692

This was reported to vendor-sec on 2005-01-08

Erik Sjölund discovered two vulnerabilities in programs contained in ncpfs:

CAN-2005-0013 Unauthorised file access in ncpfs 2.2.x
CAN-2005-0014 Buffer overflow in ncpfs 2.2.5.

I've also contacted the upstream maintainer.

Erik Sjölund wrote:
> On debian stable,
> as root do the following
>
> # apt-get install ncpfs
> # touch /tmp/foo
> # chmod 600 /tmp/foo
> # ls -l --full-time --time=atime /tmp/foo
> -rw-------    1 root     root            0 Fri Jan 07 10:51:55 2005
> /tmp/foo
>
> then as a user do the following
>
> $ cd ~
> $ mkdir foodir
> $ ln -s /tmp/foo .nwclient
> $ ncpmount foodir
> ncpmount: Connection to specified server does not exist (0x880F) in
> find_conn_spec
>
> Then check if the access time of the file has changed. As root do,
>
> # ls -l --full-time --time=atime /tmp/foo
> -rw-------    1 root     root            0 Fri Jan 07 10:54:45 2005
> /tmp/foo

I couldn't confirm this in a chroot environment but other factors may
have mitigated the problem.

> The file /tmp/foo was read and thus its file permissions were violated.
> Now what happens if a user links to an interesting device file instead?
> ( I didn't test this though )
>
> Instead of a soft symlink the same can be achieved with a hard symlink.

(symlink || hardlink)

> the relevant code piece is in the function ncp_fopen_nwc() in the
> file ncpfs-2.2.0.18/lib/ncplib.c
>
> The problem is that it doesn't check that the file ownership matches
> getuid().

This is CAN-2005-0013.

This patch should be sufficient:

--- ncpfs-2.2.0.18.orig/lib/ncplib.c
+++ ncpfs-2.2.0.18/lib/ncplib.c
@@ -2210,6 +2210,10 @@
                *err = errno;
                return NULL;
        }
+        if (st.st_uid != getuid()) {
+                *err = EACCES;
+                return NULL;
+        }
        if ((st.st_mode & (S_IRWXO | S_IRWXG)) != 0) {
                *err = NCPLIB_INVALID_MODE;
                return NULL;

> Then to make it safer: Maybe ncpfs shouldn't allow ~/.nwclient to be a
> link either. And maybe ncpfs should not trust the HOME variable because
> it is a setuid program.

Yes.

It would also be a good idea to drop privileges, read the file, regain
privileges to fulfil the task that requires root, and maybe drop them
again when they are not needed anymore.  (--> Hint for Petr as upstream)

I'm not sure disallowing ~/.nw* being a symlink would buy us any
security.  (I'm rather sure that it doesn't, but it's always good
to discuss the possibilities)

> Now a look at the situation with ncpfs 2.2.5 ( not in debian stable )
> ------------------------------------------------
>
> The source code has been restructured, new file names and new function
> names but the exploit still exist there.
>
> First as root create a file:
>
> # touch /tmp/foo
> # chmod 600 /tmp/foo
>
> Then as a user
>
> $ export HOME=/tmp
> $ ln /tmp/foo $HOME/.nwinfos
> $ ls -l --full-time --time=atime /tmp/foo
> -rw-------  2 root root 0 2005-01-05 20:13:09.822116800 +0100 /tmp/foo
> $ ncplogin
> failed:Cannot attach to tree e:. Err:Server not found (0x8847)
> $ ls -l --full-time --time=atime /tmp/foo
> -rw-------  2 root root 0 2005-01-05 20:13:55.147983129 +0100 /tmp/foo
>
> readnwinfosfile() in lib/nwclient.c
> does trust $HOME blindly and opens $HOME/.nwinfos and reads lines with
> fgets()

This is also CAN-2005-0013.

Similar patch:

--- ncpfs-2.2.5.orig/lib/nwclient.c
+++ ncpfs-2.2.5/lib/nwclient.c
@@ -482,6 +482,10 @@
                 *err = errno;
                 return NULL;
         }
+        if (st.st_uid != getuid()) {
+                *err = EACCES;
+                return NULL;
+        }
         if ((st.st_mode & (S_IRWXO | S_IRWXG)) != 0) {
                 *err = NCPLIB_INVALID_MODE;
                 return NULL;

> And now another thing present in ncpfs-2.2.5 but not in ncpfs-2.2.0
> --------------------------------------------------------------------
>
> In the file: ncpfs-2.2.5/sutil/ncplogin.c
>
> static int opt_set_volume_after_parsing_all_options(struct
> ncp_mount_info* info) {
> char tmpNWPath[1024];
> int e;
>
> /* we DID check in main that -V has been specified !*/
> strcpy(tmpNWPath,info->remote_path);
> if (info->root_path) {
>    strcat(tmpNWPath,"/");
>    strcat(tmpNWPath,info->root_path);
> }
>
> It seems root_path comes from the command line with no length check
> done. Potential buffer overflow. But this function is called some way
> into the program so I think without having any Novell NetWare servers it
> is hard to try this buffer overflow out.

This is CAN-2005-0014.

Proposed patch:

--- ncpfs-2.2.5.orig/sutil/ncplogin.c
+++ ncpfs-2.2.5/sutil/ncplogin.c
@@ -166,10 +166,11 @@
                int e;

                 /* we DID check in main that -V has been specified !*/
-                strcpy(tmpNWPath,info->remote_path);
+                memset(tmpNWPath,0,sizeof(tmpNWPath));
+                strncpy(tmpNWPath,sizeof(tmpNWPath)-1,info->remote_path);
                 if (info->root_path) {
-                        strcat(tmpNWPath,"/");
-                        strcat(tmpNWPath,info->root_path);
+                       
strncat(tmpNWPath,"/",sizeof(tmpNWPath)-strlen(tmpNWPath)-1);
+                       
strncat(tmpNWPath,info->root_path,sizeof(tmpNWPath)-strlen(tmpNWPath)-1);
                 }
                 /* I keep forgeting typing it in uppercase so let's do it here */
                 str_upper(tmpNWPath);

> cheers,
> Erik Sjölund
> Adivo Datakonsult AB