Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Chris Mason]

On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <pcg@goof.com> wrote:

>>> EIP; c013f911 <filldir+20b/221>   <=====
> Trace; c013f706 <filldir+0/221>
> Trace; c0136e01 <reiserfs_getblk+2a/16d>

The buffer reiserfs is sending to filldir is big enough for
the huge file name, so I think the real fix should be done in VFSland.

But, in the interest of providing a quick, obviously correct fix, this
reiserfs only patch will refuse to create file names larger 
than 255 chars, and skip over any directory entries larger than 
255 chars.

--- linux/include/linux/reiserfs_fs.h.1	Tue Jan  9 21:56:18 2001
+++ linux/include/linux/reiserfs_fs.h	Tue Jan  9 21:56:33 2001
@@ -467,7 +467,7 @@
 /* name by bh, ih and entry_num */
 #define B_I_E_NAME(entry_num,bh,ih) ((char *)(bh->b_data + ih->ih_item_location + (B_I_DEH(bh,ih)+(entry_num))->deh_location))
 
-#define REISERFS_MAX_NAME_LEN(block_size) (block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE)	/* -SD_SIZE when entry will contain stat data */
+#define REISERFS_MAX_NAME_LEN(block_size) 255
 
 /* this structure is used for operations on directory entries. It is not a disk structure. */
 /* When reiserfs_find_entry or search_by_entry_key find directory entry, they return filled reiserfs_dir_entry structure */
--- linux/fs/reiserfs/dir.c.1	Tue Jan  9 22:06:06 2001
+++ linux/fs/reiserfs/dir.c	Tue Jan  9 22:15:17 2001
@@ -159,6 +159,10 @@
 		d_name = B_I_DEH_ENTRY_FILE_NAME (bh, ih, deh);
 		d_off = deh->deh_offset;
 		d_ino = deh->deh_objectid;
+		if (d_reclen > REISERFS_MAX_NAME_LEN(inode->i_sb->s_blocksize)){
+		    /* it is too big to send back to VFS */
+		    continue ;
+		}
 		if (d_reclen <= 32) {
 		    local_buf = small_buf ;
 		} else {

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[David Ford]

Why not use the limits from <linux/limits.h> instead?

-d

Chris Mason wrote:

> On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <pcg@goof.com> wrote:
>
> >>> EIP; c013f911 <filldir+20b/221>   <=====
> > Trace; c013f706 <filldir+0/221>
> > Trace; c0136e01 <reiserfs_getblk+2a/16d>
>
> The buffer reiserfs is sending to filldir is big enough for
> the huge file name, so I think the real fix should be done in VFSland.
>
> But, in the interest of providing a quick, obviously correct fix, this
> reiserfs only patch will refuse to create file names larger
> than 255 chars, and skip over any directory entries larger than
> 255 chars.
>
> --- linux/include/linux/reiserfs_fs.h.1 Tue Jan  9 21:56:18 2001
> +++ linux/include/linux/reiserfs_fs.h   Tue Jan  9 21:56:33 2001
> @@ -467,7 +467,7 @@
>  /* name by bh, ih and entry_num */
>  #define B_I_E_NAME(entry_num,bh,ih) ((char *)(bh->b_data + ih->ih_item_location + (B_I_DEH(bh,ih)+(entry_num))->deh_location))
>
> -#define REISERFS_MAX_NAME_LEN(block_size) (block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE)        /* -SD_SIZE when entry will contain stat data */
> +#define REISERFS_MAX_NAME_LEN(block_size) 255
>
>  /* this structure is used for operations on directory entries. It is not a disk structure. */
>  /* When reiserfs_find_entry or search_by_entry_key find directory entry, they return filled reiserfs_dir_entry structure */
> --- linux/fs/reiserfs/dir.c.1   Tue Jan  9 22:06:06 2001
> +++ linux/fs/reiserfs/dir.c     Tue Jan  9 22:15:17 2001
> @@ -159,6 +159,10 @@
>                 d_name = B_I_DEH_ENTRY_FILE_NAME (bh, ih, deh);
>                 d_off = deh->deh_offset;
>                 d_ino = deh->deh_objectid;
> +               if (d_reclen > REISERFS_MAX_NAME_LEN(inode->i_sb->s_blocksize)){
> +                   /* it is too big to send back to VFS */
> +                   continue ;
> +               }
>                 if (d_reclen <= 32) {
>                     local_buf = small_buf ;
>                 } else {
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> Please read the FAQ at http://www.tux.org/lkml/

-- ---NOTICE

-- fwd: fwd: fwd: type emails will be deleted automatically.
      "There is a natural aristocracy among men. The grounds of this are
      virtue and talents", Thomas Jefferson [1742-1826], 3rd US President



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Alexander Viro]

On Tue, 9 Jan 2001, Chris Mason wrote:

> 
> 
> On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <pcg@goof.com> wrote:
> 
> >>> EIP; c013f911 <filldir+20b/221>   <=====
> > Trace; c013f706 <filldir+0/221>
> > Trace; c0136e01 <reiserfs_getblk+2a/16d>
> 
> The buffer reiserfs is sending to filldir is big enough for
> the huge file name, so I think the real fix should be done in VFSland.
 
> But, in the interest of providing a quick, obviously correct fix, this

ITYM "band-aid"

> reiserfs only patch will refuse to create file names larger 
> than 255 chars, and skip over any directory entries larger than 
> 255 chars.

Chris, I seriously suspect that it's not that simple (read: trace is a
BS). 0x20b is just too large for filldir().

However, actual code really looks like the end of filldir(). If that's the
case we are deep in it - argument of filldir() gets screwed. buf, that is.
Since it happens after we've already done dereferencing of buf in filldir()
and we don't trigger them... Fsck knows. copy_to_user() and put_user() should
not be able to screw the kernel stack.

Marc, could you please get larger area before the point of oops + your
fs/readdir.c and send them to me? As much as I hate disassembling, it's
better than guessing the version of compiler you've used and flags you've
built with.

Another useful thing would be printk() of buf prior and post calls of
copy_to_user() and put_user() + value of &dirent before these calls.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Chris Mason]

On Wednesday, January 10, 2001 12:47:17 AM -0500 Alexander Viro
<viro@math.psu.edu> wrote:

> However, actual code really looks like the end of filldir(). If that's the
> case we are deep in it - argument of filldir() gets screwed. buf, that is.
> Since it happens after we've already done dereferencing of buf in
> filldir() and we don't trigger them... Fsck knows. copy_to_user() and
> put_user() should not be able to screw the kernel stack.
> 
In filldir, I don't like the line where we ((char *)dirent += reclen ;  If
reclen is much larger than the buffer sent from userspace, I don't see how
we stay in bounds.

-chris

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Alexander Viro]

On Wed, 10 Jan 2001, Chris Mason wrote:

> 
> 
> On Wednesday, January 10, 2001 12:47:17 AM -0500 Alexander Viro
> <viro@math.psu.edu> wrote:
> 
> > However, actual code really looks like the end of filldir(). If that's the
> > case we are deep in it - argument of filldir() gets screwed. buf, that is.
> > Since it happens after we've already done dereferencing of buf in
> > filldir() and we don't trigger them... Fsck knows. copy_to_user() and
> > put_user() should not be able to screw the kernel stack.
> > 
> In filldir, I don't like the line where we ((char *)dirent += reclen ;  If
> reclen is much larger than the buffer sent from userspace, I don't see how
> we stay in bounds.

	So? copy_to_user() and put_user() will refuse to scramble the
kernel memory. IOW, dirent can be out of the userspace. Hell, user could
call getdents() and pass it a kernel pointer. Try it and you'll see what
happens.

	BTW, in that particular line we are actually even protected from
getting too large dirent, since if the sum was out of the user space we
would die several lines above upon copy_to_user().

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Chris Mason]

On Wednesday, January 10, 2001 12:38:34 PM -0500 Alexander Viro
<viro@math.psu.edu> wrote:

> On Wed, 10 Jan 2001, Chris Mason wrote:
> 
>> In filldir, I don't like the line where we ((char *)dirent += reclen ;
>> If reclen is much larger than the buffer sent from userspace, I don't
>> see how we stay in bounds.
> 
>	 So? copy_to_user() and put_user() will refuse to scramble the
> kernel memory. IOW, dirent can be out of the userspace. Hell, user could
> call getdents() and pass it a kernel pointer. Try it and you'll see what
> happens.
> 

Ah thanks, that makes more sense.  But, copy_to_user is only working on
namelen bytes, and reclen is bigger than that.  So, who is checking the
value for the buf->current_dir pointer?

-chris

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Alexander Viro]

On Wed, 10 Jan 2001, Chris Mason wrote:

> Ah thanks, that makes more sense.  But, copy_to_user is only working on
> namelen bytes, and reclen is bigger than that.  So, who is checking the
> value for the buf->current_dir pointer?

Look at the thing again. Especially at the place where reclen is calculated.
Notice the calls of put_user() before and after copy_to_user(). Add them
up. Round.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Andrea Arcangeli]

On Wed, Jan 10, 2001 at 12:47:17AM -0500, Alexander Viro wrote:
> Chris, I seriously suspect that it's not that simple (read: trace is a
> BS). 0x20b is just too large for filldir().
[..]
> and we don't trigger them... Fsck knows. copy_to_user() and put_user() should
> not be able to screw the kernel stack.

That's what I said Chris yesterday.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Vladimir V. Saveliev]

Hi

Chris Mason wrote:

> On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <pcg@goof.com> wrote:
>
> >>> EIP; c013f911 <filldir+20b/221>   <=====
> > Trace; c013f706 <filldir+0/221>
> > Trace; c0136e01 <reiserfs_getblk+2a/16d>
>
> The buffer reiserfs is sending to filldir is big enough for
> the huge file name, so I think the real fix should be done in VFSland.
>
> But, in the interest of providing a quick, obviously correct fix, this
> reiserfs only patch will refuse to create file names larger
> than 255 chars, and skip over any directory entries larger than
> 255 chars.
>

Hmm, wouldn't it make existing long named files unreachable?

Thanks,
vs




>
> --- linux/include/linux/reiserfs_fs.h.1 Tue Jan  9 21:56:18 2001
> +++ linux/include/linux/reiserfs_fs.h   Tue Jan  9 21:56:33 2001
> @@ -467,7 +467,7 @@
>  /* name by bh, ih and entry_num */
>  #define B_I_E_NAME(entry_num,bh,ih) ((char *)(bh->b_data + ih->ih_item_location + (B_I_DEH(bh,ih)+(entry_num))->deh_location))
>
> -#define REISERFS_MAX_NAME_LEN(block_size) (block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE)        /* -SD_SIZE when entry will contain stat data */
> +#define REISERFS_MAX_NAME_LEN(block_size) 255
>
>  /* this structure is used for operations on directory entries. It is not a disk structure. */
>  /* When reiserfs_find_entry or search_by_entry_key find directory entry, they return filled reiserfs_dir_entry structure */
> --- linux/fs/reiserfs/dir.c.1   Tue Jan  9 22:06:06 2001
> +++ linux/fs/reiserfs/dir.c     Tue Jan  9 22:15:17 2001
> @@ -159,6 +159,10 @@
>                 d_name = B_I_DEH_ENTRY_FILE_NAME (bh, ih, deh);
>                 d_off = deh->deh_offset;
>                 d_ino = deh->deh_objectid;
> +               if (d_reclen > REISERFS_MAX_NAME_LEN(inode->i_sb->s_blocksize)){
> +                   /* it is too big to send back to VFS */
> +                   continue ;
> +               }
>                 if (d_reclen <= 32) {
>                     local_buf = small_buf ;
>                 } else {

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Chris Mason]

On Wednesday, January 10, 2001 07:02:08 PM +0300 "Vladimir V. Saveliev"
<vs@namesys.botik.ru> wrote:

> Hi
> 
> Chris Mason wrote:
> 
>> On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann
>> <pcg@goof.com> wrote:
>> 
>> >>> EIP; c013f911 <filldir+20b/221>   <=====
>> > Trace; c013f706 <filldir+0/221>
>> > Trace; c0136e01 <reiserfs_getblk+2a/16d>
>> 
>> The buffer reiserfs is sending to filldir is big enough for
>> the huge file name, so I think the real fix should be done in VFSland.
>> 
>> But, in the interest of providing a quick, obviously correct fix, this
>> reiserfs only patch will refuse to create file names larger
>> than 255 chars, and skip over any directory entries larger than
>> 255 chars.
>> 
> 
> Hmm, wouldn't it make existing long named files unreachable?
> 

Yes, that was intentional.  We can make a different version of the patch
that changes reiserfs_find_entry to allow opening the large filenames for
delete and such.  But, as a quick fix, I wanted to close all possible paths
to the long names.

-chris
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSELinux)

[Vladimir V. Saveliev]

Chris Mason wrote:

> On Wednesday, January 10, 2001 07:02:08 PM +0300 "Vladimir V. Saveliev"
> <vs@namesys.botik.ru> wrote:
>
> > Hi
> >
> > Chris Mason wrote:
> >
> >> On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann
> >> <pcg@goof.com> wrote:
> >>
> >> >>> EIP; c013f911 <filldir+20b/221>   <=====
> >> > Trace; c013f706 <filldir+0/221>
> >> > Trace; c0136e01 <reiserfs_getblk+2a/16d>
> >>
> >> The buffer reiserfs is sending to filldir is big enough for
> >> the huge file name, so I think the real fix should be done in VFSland.
> >>
> >> But, in the interest of providing a quick, obviously correct fix, this
> >> reiserfs only patch will refuse to create file names larger
> >> than 255 chars, and skip over any directory entries larger than
> >> 255 chars.
> >>
> >
> > Hmm, wouldn't it make existing long named files unreachable?
> >
>
> Yes, that was intentional.  We can make a different version of the patch
> that changes reiserfs_find_entry to allow opening the large filenames for
> delete and such.  But, as a quick fix, I wanted to close all possible paths
> to the long names.
>

Sorry, I still do not understand what you are fixing :)

Btw, I do not see how does fs/readdir.c:fillonedir (I am looking at
2.4.0-test10) check that buffer provided by user is long enough to keep the
name in. (that is old_readdir looks broken for me - am I missing something?)
Whereas filldir seems to check whether there is enough space left in the
buffer.

Thanks,
vs


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Stefan Traby]

On Wed, Jan 10, 2001 at 07:02:08PM +0300, Vladimir V. Saveliev wrote:

> Hmm, wouldn't it make existing long named files unreachable?

This is not of primary interest. Security first.
The only way to recover those files secure without risking a crash
is maybe to let fsck rename those long files after the patch.

Before the 255-limit-patch a rename(2) may work, but without a directory
lookup from userland; quite hard to do.

When I played with Marc's case, I needed to reboot 2 times because I
tried to use tab-expansion on bash to get the filename; which caused
a machine freeze.

perl -e 'rmdir "x" x 768' worked,

or under bash

rmdir <ESC>768x  should work, too.

Really, the 255-limit is essential as long as "struct dirent/64" has
d_name[255] hard coded. Somebody should send Drepper a patch;
I did not understand why he accepted a NAME_MAX of 4032 patch for
reiserfs while knowing the hardcoded dirent limit.


-- 

  ciao - 
    Stefan

"     ( cd /lib ; ln -s libBrokenLocale-2.2.so libNiedersachsen.so )     "
    
Stefan Traby                Linux/ia32               fax:  +43-3133-6107-9
Mitterlasznitzstr. 13       Linux/alpha            phone:  +43-3133-6107-2
8302 Nestelbach             Linux/sparc       http://www.hello-penguin.com
Austria                                    mailto://st.traby@opengroup.org
Europe                                   mailto://stefan@hello-penguin.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Stefan Traby]

On Wed, Jan 10, 2001 at 06:03:53PM +0100, Stefan Traby wrote:

> Really, the 255-limit is essential as long as "struct dirent/64" has
> d_name[255] hard coded. Somebody should send Drepper a patch;

sorry, d_name[256], not 255 in both, glibc and kernel...

-- 

    Stefan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Chris Mason]

On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <pcg@goof.com> wrote:
>>> EIP; c013f911 <filldir+20b/221>   <=====
> Trace; c013f706 <filldir+0/221>
> Trace; c0136e01 <reiserfs_getblk+2a/16d>
> 

Here is a patch against our 2.4 code (3.6.25) that does the
same as the patch posted for 3.5.29:

-chris

--- linux/include/linux/reiserfs_fs.h.1	Tue Jan  9 21:22:27 2001
+++ linux/include/linux/reiserfs_fs.h	Tue Jan  9 21:22:55 2001
@@ -926,8 +926,7 @@
 //((block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE * 2) / 2)
 
 // two entries per block (at least)
-#define REISERFS_MAX_NAME_LEN(block_size) \
-((block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE))
+#define REISERFS_MAX_NAME_LEN(block_size) 255
 
 
 
--- linux/fs/reiserfs/dir.c.1	Tue Jan  9 21:22:19 2001
+++ linux/fs/reiserfs/dir.c	Tue Jan  9 21:21:02 2001
@@ -142,6 +142,10 @@
 		if (!d_name[d_reclen - 1])
 		    d_reclen = strlen (d_name);
 	
+		if (d_reclen > REISERFS_MAX_NAME_LEN(inode->i_sb->s_blocksize)){
+		    /* too big to send back to VFS */
+		    continue ;
+		}
 		d_off = deh_offset (deh);
 		filp->f_pos = d_off ;
 		d_ino = deh_objectid (deh);


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [reiserfs-list] major security bug in reiserfs (may affect SuSE Linux)

[Hans Reiser]

Chris Mason wrote:
> 
> On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <pcg@goof.com> wrote:
> >>> EIP; c013f911 <filldir+20b/221>   <=====
> > Trace; c013f706 <filldir+0/221>
> > Trace; c0136e01 <reiserfs_getblk+2a/16d>
> >
> 
> Here is a patch against our 2.4 code (3.6.25) that does the
> same as the patch posted for 3.5.29:
> 
> -chris
> 
> --- linux/include/linux/reiserfs_fs.h.1 Tue Jan  9 21:22:27 2001
> +++ linux/include/linux/reiserfs_fs.h   Tue Jan  9 21:22:55 2001
> @@ -926,8 +926,7 @@
>  //((block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE * 2) / 2)
> 
>  // two entries per block (at least)
> -#define REISERFS_MAX_NAME_LEN(block_size) \
> -((block_size - BLKH_SIZE - IH_SIZE - DEH_SIZE))
> +#define REISERFS_MAX_NAME_LEN(block_size) 255
> 
> 
> 
> --- linux/fs/reiserfs/dir.c.1   Tue Jan  9 21:22:19 2001
> +++ linux/fs/reiserfs/dir.c     Tue Jan  9 21:21:02 2001
> @@ -142,6 +142,10 @@
>                 if (!d_name[d_reclen - 1])
>                     d_reclen = strlen (d_name);
> 
> +               if (d_reclen > REISERFS_MAX_NAME_LEN(inode->i_sb->s_blocksize)){
> +                   /* too big to send back to VFS */
> +                   continue ;
> +               }
>                 d_off = deh_offset (deh);
>                 filp->f_pos = d_off ;
>                 d_ino = deh_objectid (deh);


I think that in the short term, so as to make it easier to merge us into 2.4, it is reasonable to
restrict us to small names, so go ahead and merge this code into cvs if not done already.

Hans
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/