Bugzilla Bug 77414

Erik Sj

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);

Created an attachment (id=48563) [details]
CAN-2005-0013+4.patch

Upstream patch for the 2.2.6 release of ncpfs.

Maurice: this is confidential. Could you please test the patches and prepare an
ebuild ? We can't commit it to portage until disclosure date, so please attach
it here if it's ready. We'll call arch testing on the bug.

A couple of questions: 
- where is 2.2.6?
- what do you want me to test? (I never used ncpfs)
- jaervosz made the patch and seems to have tested it. Isn't he the right person
to fix the ebuild?

Maurice,
- probably going to be released soon
- you committed the last security fix for ncpfs (anyone from net-fs using this?)
- patch is from upstream, I only passed it on

Patches in the initial description is for the Debian.

1 & 3) So does this mean: 
a) 2.2.6 will already include this fix and we just need to prepare a fixed 2.2.5 
   ebuild right away
 or
b) 2.2.6 will not include the fix for some reason and we need to patch it when it
   comes out

2) I'm not aware of anyone using it. Currently net-fs has 3 people listed: 
rphillips (don't know about him), vapier (only helps with nfs-related stuff) and 
latexer (don't know anything about him either). I am not part of net-fs myself.

Created an attachment (id=48689) [details]
CAN-2005-0013.patch

Patch for 2.2.5 from initial bug report above.

Created an attachment (id=48690) [details]
CAN-2005-0014.patch

If nobody uses this better wait for disclosure so that we can handle it in the
open.

Maurice, upstream has a fixed version. Please bump.

I'll be unable to test beyond just emerging it, so I'll just check in as stable
if it merges successfully. Please respond with any objections before I get to 
repoman commit ;)

Maurice: if in doubt you can commit it as ~* or -*, and we'll ask for arches to
test.

ppc64 arch people, please test 2.2.6 as far as possible and then mark it stable
for your arch.

I have the same problem as in comment #10. But as this package merges fine and
the given programms "run" fine, I've marked it stable on ppc64.

GLSA 200501-44