LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Willy Tarreau <w@1wt.eu>
To: Santiago Garcia Mantinan <manty@debian.org>
Cc: linux-kernel@vger.kernel.org, debian-kernel@lists.debian.org,
	dannf@dannf.org
Subject: Re: problems with latest smbfs changes on 2.4.34 and security backports
Date: Wed, 17 Jan 2007 22:55:19 +0100	[thread overview]
Message-ID: <20070117215519.GX24090@1wt.eu> (raw)
In-Reply-To: <20070117100030.GA11251@clandestino.aytolacoruna.es>

Hello Santiago,

On Wed, Jan 17, 2007 at 11:00:30AM +0100, Santiago Garcia Mantinan wrote:
> Hi!
> 
> I have discovered a problem with the changes applied to smbfs in 2.4.34 and
> in the security backports like last Debian's 2.4 kernel update these changes
> seem to be made to solve CVE-2006-5871 and they have broken symbolic links
> and changed the way that special files (like devices) are seen.
> 
> For example:
> Before: symbolic links were seen as that, symbolic links an thus if you tried
> to open the file the link was followed and you ended up reading the
> destination file
> Now: symbolic links are seen as normal files (at least with a ls) but their
> length (N) is the length of the symbolic link, if you read it, you'll get the
> first N characters of the destination file. For example, on my filesystem
> bin/sh is a symlink to bash, thus it is 4 bytes long, if I to a cat bin/sh I
> get ELF (this is, the first 4 characters of the bash program, first one
> being a DEL).
> 
> Another example:
> Before: if you did a ls of a device file, like dev/zero you could see it as
> a device, if you tried opening it, it wouldn't work, but if you did a cp -a
> of that file to a local filesystem the result was a working zero device.
> Now: the devices are seen as normal files with a length of 0 bytes.
> 
> Seems to me like a mask is erasing some mode bits that shouldn't be erased.
> 
> I have carried my tests on a Debian Sarge machine always mounting the share
> using: smbmount //server/share /mnt without any other options. The tests
> were carried on a unpatched 2.4.34 comparing it to 2.4.33 and also on
> Debian's 2.4.27 comparing 2.4.27-10sarge4 vs -10sarge5. The server is a
> samba 3.0.23d and I have experienced the same behaviour with samba's
> unix extensions = yes and unix extensions = no.
> 
> I don't know what else to add, if you need any more info or want me to do
> any tests just ask for it.

Well, there is not much to add there. Thanks very much for all your tests.
This problem was not easy to fix, and Dann Frazier did a careful job at
backporting it and testing it. Unfortunately, corner cases like this may
sometimes pass through the tests.

Dann, do you still have your samba server ready to try to reproduce this
problem ? Also, there are very suspect lines right there in the patch :

@@ -505,8 +510,13 @@
 		mnt->file_mode = (oldmnt->file_mode & S_IRWXUGO) | S_IFREG;
 		mnt->dir_mode = (oldmnt->dir_mode & S_IRWXUGO) | S_IFDIR;
 
-		mnt->flags = (oldmnt->file_mode >> 9);
+		mnt->flags = (oldmnt->file_mode >> 9) | SMB_MOUNT_UID |
+			SMB_MOUNT_GID | SMB_MOUNT_FMODE | SMB_MOUNT_DMODE;
 	} else {
+		mnt->file_mode = mnt->dir_mode = S_IRWXU | S_IRGRP | S_IXGRP |
+						S_IROTH | S_IXOTH | S_IFREG;
+		mnt->dir_mode = mnt->dir_mode = S_IRWXU | S_IRGRP | S_IXGRP |
+						S_IROTH | S_IXOTH | S_IFDIR;
 		if (parse_options(mnt, raw_data))
 			goto out_bad_option;
 	}


See above ? mnt->dir_mode being assigned 3 times. It still *seems* to do the
expected thing like this but I wonder if the initial intent was exactly this.
Also, would not it be necessary to add "|S_IFLNK" to the file_mode ? Maybe
what I say is stupid, but it's just a guess.

Santiago, if you feel brave enough to try completely untested code, I
would suggest to try this change :

 	} else {
-		mnt->file_mode = mnt->dir_mode = S_IRWXU | S_IRGRP | S_IXGRP |
-						S_IROTH | S_IXOTH | S_IFREG;
-		mnt->dir_mode = mnt->dir_mode = S_IRWXU | S_IRGRP | S_IXGRP |
-						S_IROTH | S_IXOTH | S_IFDIR;
+		mnt->file_mode = S_IRWXU | S_IRGRP | S_IXGRP |
+				S_IROTH | S_IXOTH | S_IFREG | S_IFLNK;
+		mnt->dir_mode = S_IRWXU | S_IRGRP | S_IXGRP |
+				S_IROTH | S_IXOTH | S_IFDIR;
 		if (parse_options(mnt, raw_data))
 			goto out_bad_option;


Also, please try making symlinks to directories to see how they behave.

Thanks in advance,
Willy


  reply	other threads:[~2007-01-17 21:55 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-17 10:00 Santiago Garcia Mantinan
2007-01-17 21:55 ` Willy Tarreau [this message]
2007-01-18  0:09   ` Grant Coady
2007-01-18  4:21     ` Willy Tarreau
2007-01-18  5:59       ` Grant Coady
2007-01-18 22:51   ` dann frazier
2007-01-19  1:00   ` dann frazier
2007-01-19  5:15     ` Willy Tarreau
2007-01-20  1:05     ` dann frazier
2007-01-20  6:18       ` Willy Tarreau
2007-01-21 22:52       ` Grant Coady
2007-01-21 23:03         ` Willy Tarreau
2007-01-21 23:50           ` Grant Coady
2007-01-22 18:19             ` dann frazier
2007-01-23  5:42               ` Willy Tarreau
2007-01-23 21:12                 ` dann frazier
2007-01-23 22:00                   ` Willy Tarreau
2007-01-23 23:46                   ` Grant Coady
2007-01-24  0:11                     ` dann frazier
2007-01-22  8:54           ` Santiago Garcia Mantinan
2007-01-22  9:18             ` Willy Tarreau
2007-01-22  9:36               ` Santiago Garcia Mantinan
2007-01-22 10:49                 ` Grant Coady
2007-01-22 10:46               ` Grant Coady
2007-01-23 20:19           ` dann frazier
2007-01-23 21:04             ` Grant Coady
2007-01-23 21:35               ` dann frazier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070117215519.GX24090@1wt.eu \
    --to=w@1wt.eu \
    --cc=dannf@dannf.org \
    --cc=debian-kernel@lists.debian.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=manty@debian.org \
    --subject='Re: problems with latest smbfs changes on 2.4.34 and security backports' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).