major security bug in reiserfs (may affect SuSE Linux)

reiserfs-list Chris Mason 7 participants 15 messages Jan 10, 2001 – Jan 11, 2001 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Chris Mason

author

2001-01-10 02:24 UTC #1 lore raw

Quoted 3 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…

Quoted 1 line
Quoted 1 line
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.

include/linux/reiserfs_fs.h renamed from include/linux/reiserfs_fs.h.1 +1 -1

@@ -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 */

fs/reiserfs/dir.c renamed from fs/reiserfs/dir.c.1 +4 -0

@@ -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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David Ford

2001-01-10 04:43 UTC #2 lore raw

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

-d

Quoted 40 lines · Chris Mason

Quoted 3 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…
Quoted 1 line
Quoted 1 line
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.
include/linux/reiserfs_fs.h Tue Jan 9 21:56:33 2001 renamed from include/linux/reiserfs_fs.h.1 Tue Jan 9 21:56:18 2001 +1 -1
@@ -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 */
fs/reiserfs/dir.c Tue Jan 9 22:15:17 2001 renamed from fs/reiserfs/dir.c.1 Tue Jan 9 22:06:06 2001 +4 -1
@@ -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 {
 
-

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Alexander Viro

2001-01-10 05:47 UTC #3 lore raw

In reply to Chris Mason

Quoted 4 lines · Chris Mason

On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]> wrote:

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Quoted 1 line
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"

Quoted 3 lines

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Chris Mason

author

2001-01-10 15:49 UTC #4 lore raw

Quoted 6 lines · On Wednesday, January 10, 2001 12:47:17 AM -0500 Alexander Viro…

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Alexander Viro

2001-01-10 17:39 UTC #5 lore raw

Quoted 11 lines · Chris Mason

Quoted 6 lines · On Wednesday, January 10, 2001 12:47:17 AM -0500 Alexander Viro…
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.

Quoted 3 lines

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Chris Mason

author

2001-01-10 18:49 UTC #6 lore raw

Quoted 11 lines · On Wednesday, January 10, 2001 12:38:34 PM -0500 Alexander Viro…

Quoted 3 lines · Chris Mason
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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Alexander Viro

2001-01-11 00:47 UTC #7 lore raw

Quoted 3 lines · Chris Mason

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Vladimir V. Saveliev

2001-01-10 16:05 UTC #8 lore raw

In reply to Chris Mason

Quoted 6 lines · Chris Mason

Quoted 3 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…
Quoted 1 line
Quoted 1 line
EIP; c013f911 <filldir+20b/221> <=====
Trace; c013f706 <filldir+0/221>
Trace; c0136e01 <reiserfs_getblk+2a/16d>

Quoted 8 lines

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

Quoted 15 lines

include/linux/reiserfs_fs.h Tue Jan 9 21:56:33 2001 renamed from include/linux/reiserfs_fs.h.1 Tue Jan 9 21:56:18 2001 +1 -1

@@ -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 */

fs/reiserfs/dir.c Tue Jan 9 22:15:17 2001 line 168

Quoted 10 lines

fs/reiserfs/dir.c Tue Jan 9 22:15:17 2001 renamed from fs/reiserfs/dir.c.1 Tue Jan 9 22:06:06 2001 +4 -0

@@ -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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Chris Mason

author

2001-01-10 16:09 UTC #9 lore raw

Quoted 14 lines · On Wednesday, January 10, 2001 07:02:08 PM +0300 "Vladimir V. Saveliev"…

Hi
Quoted 10 lines · Chris Mason
Quoted 3 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…
Quoted 1 line
Quoted 1 line
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.

Quoted 8 lines

Quoted 5 lines
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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Vladimir V. Saveliev

2001-01-10 16:32 UTC #10 lore raw

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

Quoted 23 lines · Chris Mason

Quoted 20 lines · On Wednesday, January 10, 2001 07:02:08 PM +0300 "Vladimir V. Saveliev"…
Hi
Quoted 15 lines · Chris Mason
Quoted 3 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…
Quoted 1 line
Quoted 1 line
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.

Quoted 8 lines

Quoted 2 lines
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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Andrea Arcangeli

2001-01-10 16:42 UTC #11 lore raw

In reply to Chris Mason

Quoted 2 lines · Alexander Viro

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

[..]

Quoted 2 lines

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Stefan Traby

2001-01-10 17:05 UTC #12 lore raw

In reply to Chris Mason

Quoted 1 line · Vladimir V. Saveliev

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.

Signature · 14 lines

  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://[email protected]
Europe                                   mailto://[email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Stefan Traby

2001-01-10 17:12 UTC #13 lore raw

In reply to Chris Mason

Quoted 2 lines · Stefan Traby

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...

Signature · 5 lines

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

Chris Mason

author

2001-01-10 02:41 UTC #14 lore raw

In reply to a message not in the archive

Quoted 4 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…

Quoted 1 line
Quoted 1 line
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

include/linux/reiserfs_fs.h renamed from include/linux/reiserfs_fs.h.1 +1 -2

@@ -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

fs/reiserfs/dir.c renamed from fs/reiserfs/dir.c.1 +4 -0

@@ -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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Hans Reiser

2001-01-11 12:41 UTC #15 lore raw

Quoted 27 lines · Chris Mason

Quoted 4 lines · On Wednesday, January 10, 2001 02:32:09 AM +0100 Marc Lehmann <[email protected]>…
Quoted 1 line
Quoted 1 line
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:
(quoted diff) +0 -1
-chris
include/linux/reiserfs_fs.h Tue Jan 9 21:22:55 2001 renamed from include/linux/reiserfs_fs.h.1 Tue Jan 9 21:22:27 2001 +1 -2
@@ -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
 
 
 

fs/reiserfs/dir.c Tue Jan 9 21:21:02 2001 line 151

Quoted 10 lines

fs/reiserfs/dir.c Tue Jan 9 21:21:02 2001 renamed from fs/reiserfs/dir.c.1 Tue Jan 9 21:22:19 2001 +4 -0

@@ -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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/