Modprobe local root exploit

Modprobe local root exploit

[Gregory Maxwell]

After seeing the modprobe local root exploit today, I asked myself why
kmod executes modprobe with full root and doesn't drop some capabilities
first.

Why? It wouldn't close the hole, but it would narrow it down.
-
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/

Modprobe local root exploit

[Torsten Duwe]

>>>>> "Gregory" == Gregory Maxwell <greg@linuxpower.cx> writes:

    Gregory> After seeing the modprobe local root exploit today, I asked
    Gregory> myself why kmod executes modprobe with full root and doesn't
    Gregory> drop some capabilities first.

    Gregory> Why? It wouldn't close the hole, but it would narrow it down.

This might also be a good idea; but my suggestion is to not allow arbitrary
strings as module names in the first place. As far as I can see, all valid
strings for KMOD requests consist of alphanumeric chars plus dash and
underscore. Anybody with autoloaded modules that don't fit this pattern even
after /etc/modules.conf translation please object !

Here's the patch...

	Torsten

--- linux/kernel/kmod.c.orig	Tue Sep 26 01:18:55 2000
+++ linux/kernel/kmod.c	Mon Nov 13 16:57:02 2000
@@ -168,6 +168,22 @@
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	 * only of allowed characters. Especially whitespace and
+	 * shell metacharacters might confuse modprobe.
+	 */
+	for (p = module_name; *p; p++)
+	{
+	  if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z')
+	    continue;
+	  if (*p >= '0' && *p <= '9')
+	    continue;
+	  if (*p == '_' || *p == '-')
+	    continue;
+	  return -EINVAL;
+	}
 
 	/* Don't allow request_module() before the root fs is mounted!  */
 	if ( ! current->fs->root ) {

-
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: Modprobe local root exploit

[Francis Galiegue]

On Mon, 13 Nov 2000, Torsten Duwe wrote:

> 
> --- linux/kernel/kmod.c.orig	Tue Sep 26 01:18:55 2000
> +++ linux/kernel/kmod.c	Mon Nov 13 16:57:02 2000
> @@ -168,6 +168,22 @@
>  	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
>  #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
>  	static int kmod_loop_msg;
> +	const char * p;
> +
> +	/* For security reasons ensure the requested name consists
> +	 * only of allowed characters. Especially whitespace and
> +	 * shell metacharacters might confuse modprobe.
> +	 */
> +	for (p = module_name; *p; p++)
> +	{
> +	  if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z')
> +	    continue;
> +	  if (*p >= '0' && *p <= '9')
> +	    continue;
> +	  if (*p == '_' || *p == '-')
> +	    continue;
> +	  return -EINVAL;
> +	}
>  

Just in case... Some modules have uppercase letters too :)

-- 
Francis Galiegue, fg@mandrakesoft.com
"Programming is a race between programmers, who try and make more and more
idiot-proof software, and universe, which produces more and more remarkable
idiots. Until now, universe leads the race"  -- R. Cook

-
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: Modprobe local root exploit

[Torsten Duwe]

>>>>> "Francis" == Francis Galiegue <fg@mandrakesoft.com> writes:

    >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;

    Francis> Just in case... Some modules have uppercase letters too :)

That's what the &0xdf is intended for...

	Torsten
-
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: Modprobe local root exploit

[Chris Evans]

On Mon, 13 Nov 2000, Torsten Duwe wrote:

> >>>>> "Francis" == Francis Galiegue <fg@mandrakesoft.com> writes:
> 
>     >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
> 
>     Francis> Just in case... Some modules have uppercase letters too :)
> 
> That's what the &0xdf is intended for...

Code in a security sensitive area needs to be crystal clear.

What's wrong with isalnum() ?

Cheers
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: Modprobe local root exploit

[Jan Dvorak]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

On Mon, Nov 13, 2000 at 04:56:40PM +0000, Chris Evans wrote:
> 
> On Mon, 13 Nov 2000, Torsten Duwe wrote:
> 
> Code in a security sensitive area needs to be crystal clear.
> 
> What's wrong with isalnum() ?
> 

What about this then ?


[-- Attachment #2: patch.kmod --]
[-- Type: text/plain, Size: 676 bytes --]

--- kmod.c.orig	Sat Nov  4 20:02:11 2000
+++ kmod.c	Mon Nov 13 18:18:06 2000
@@ -169,6 +169,20 @@
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	* only of allowed characters. Especially whitespace and
+	* shell metacharacters might confuse modprobe.
+	*/
+	for (p = module_name; *p; p++)
+	{
+		if (isalnum(*p) || *p=='_' || *p=='-')
+			continue;
+
+		return -EINVAL;
+	}
+
 	/* Don't allow request_module() before the root fs is mounted!  */
 	if ( ! current->fs->root ) {
 		printk(KERN_ERR "request_module[%s]: Root fs not mounted\n",

Re: Modprobe local root exploit

[Torsten Duwe]

>>>>> "Chris" == Chris Evans <chris@scary.beasts.org> writes:

    Chris> What's wrong with isalnum() ?

Hm, must admit that I wasn't 100% sure if that's in the kernel lib or an evil
gcc expands it inline to some static array lookup. Now I see that it's
already in the kernel. Do some of you also sometimes wonder why the kernel is
so big ;-) ?

OK, so let's go for the isalnum() version, and don't forget to
#include <linux/ctype.h>
above...

	Torsten

For those who joined the program late here's the complete patch:

--- linux/kernel/kmod.c.orig	Tue Sep 26 01:18:55 2000
+++ linux/kernel/kmod.c	Mon Nov 13 19:09:11 2000
@@ -18,6 +18,7 @@
 #include <linux/config.h>
 #include <linux/sched.h>
 #include <linux/unistd.h>
+#include <linux/ctype.h>
 #include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
@@ -168,6 +169,19 @@
 	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
 #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
+	const char * p;
+
+	/* For security reasons ensure the requested name consists
+	 * only of allowed characters. Especially whitespace and
+	 * shell metacharacters might confuse modprobe.
+	 */
+	for (p = module_name; *p; p++)
+	{
+	  if (isalnum(*p) || *p == '_' || *p == '-')
+	    continue;
+
+	  return -EINVAL;
+	}
 
 	/* Don't allow request_module() before the root fs is mounted!  */
 	if ( ! current->fs->root ) {
-
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: Modprobe local root exploit

[Peter Samuelson]

[Torsten Duwe]
> +	for (p = module_name; *p; p++)
> +	{
> +	  if (isalnum(*p) || *p == '_' || *p == '-')
> +	    continue;
> +
> +	  return -EINVAL;
> +	}

I think you just broke at least some versions of devfs.  I don't
remember if the feature is still around, but I know it *used* to be
possible to request_module("/dev/foobar"), which requires '/' in the
name.

Peter
-
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: Modprobe local root exploit

[Keith Owens]

On Mon, 13 Nov 2000 23:02:10 -0600, 
Peter Samuelson <peter@cadcamlab.org> wrote:
>
>[Torsten Duwe]
>> +	for (p = module_name; *p; p++)
>> +	{
>> +	  if (isalnum(*p) || *p == '_' || *p == '-')
>> +	    continue;
>> +
>> +	  return -EINVAL;
>> +	}
>
>I think you just broke at least some versions of devfs.  I don't
>remember if the feature is still around, but I know it *used* to be
>possible to request_module("/dev/foobar"), which requires '/' in the
>name.

That feature of devfs is definitely still around, it is critical to
devfs.

All these patches against request_module are attacking the problem at
the wrong point.  The kernel can request any module name it likes,
using any string it likes, as long as the kernel generates the name.
The real problem is when the kernel blindly accepts some user input and
passes it straight to modprobe, then the kernel is acting like a setuid
wrapper for a program that was never designed to run setuid.

At the very least, interface names which are taken directly from the
user should be prefixed with "user-interface-" before being passed to
modprobe.  The sysadmin can set "alias user-interface-eth0 eth0" if
they want to use this feature.  Passing the interface name unchanged is
asking for trouble, as Chris Evans pointed out, you can abuse this
"feature" to load any module.

I have nearly finished the security review of modutils, 2.3.20 will be
out later tonight.  It treats the kmod environment as tainted, forces
the last parameter to be treated as a module name even if it starts
with '-', only accepts one module name and refuses 'variable=value'
strings.  I find it rather ironic to be treating kernel supplied data
as tainted.

-
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: Modprobe local root exploit

[Florian Weimer]

Keith Owens <kaos@ocs.com.au> writes:

> All these patches against request_module are attacking the problem at
> the wrong point.

Agreed.

> The kernel can request any module name it likes, using any string it
> likes, as long as the kernel generates the name.  The real problem
> is when the kernel blindly accepts some user input and passes it
> straight to modprobe, then the kernel is acting like a setuid
> wrapper for a program that was never designed to run setuid.

I don't think it's a good idea to distribute such stuff over the whole
kernel.  Better control it at a single place, either when passing the
parameter down to modprobe, or in modprobe itself.  Everything else is
too error-prone.

-- 
Florian Weimer 	                  Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
-
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: Modprobe local root exploit

[Malcolm Beattie]

Keith Owens writes:
> All these patches against request_module are attacking the problem at
> the wrong point.  The kernel can request any module name it likes,
> using any string it likes, as long as the kernel generates the name.
> The real problem is when the kernel blindly accepts some user input and
> passes it straight to modprobe, then the kernel is acting like a setuid
> wrapper for a program that was never designed to run setuid.

Rather than add sanity checking to modprobe, it would be a lot easier
and safer from a security audit point of view to have the kernel call
/sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise
all the data and exec the real modprobe. That way the only thing that
needs auditing is a string munging/sanitising program.

--Malcolm

-- 
Malcolm Beattie <mbeattie@sable.ox.ac.uk>
Unix Systems Programmer
Oxford University Computing Services
-
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: Modprobe local root exploit

[Jakub Jelinek]

On Tue, Nov 14, 2000 at 10:42:41AM +0000, Malcolm Beattie wrote:
> Keith Owens writes:
> > All these patches against request_module are attacking the problem at
> > the wrong point.  The kernel can request any module name it likes,
> > using any string it likes, as long as the kernel generates the name.
> > The real problem is when the kernel blindly accepts some user input and
> > passes it straight to modprobe, then the kernel is acting like a setuid
> > wrapper for a program that was never designed to run setuid.
> 
> Rather than add sanity checking to modprobe, it would be a lot easier
> and safer from a security audit point of view to have the kernel call
> /sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise
> all the data and exec the real modprobe. That way the only thing that
> needs auditing is a string munging/sanitising program.

Well, no matter what kernel needs auditing as well, the fact that dev_load
will without any check load any module the user wants is already problematic
and no munging helps with it at all, especially loading old ISA drivers
might not be a good idea.

	Jakub
-
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: Modprobe local root exploit

[Chris Evans]

On Tue, 14 Nov 2000, Jakub Jelinek wrote:

> > Rather than add sanity checking to modprobe, it would be a lot easier
> > and safer from a security audit point of view to have the kernel call
> > /sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise
> > all the data and exec the real modprobe. That way the only thing that
> > needs auditing is a string munging/sanitising program.
>
> Well, no matter what kernel needs auditing as well, the fact that dev_load
> will without any check load any module the user wants is already problematic
> and no munging helps with it at all, especially loading old ISA drivers
> might not be a good idea.

FWIW: A quick look at the kernel source, and dev_load() seems to be the
only place that does this. Other places apply prefixes to user supplied
names.

Cheers
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: Modprobe local root exploit

[Keith Owens]

On Tue, 14 Nov 2000 10:42:41 +0000, 
Malcolm Beattie <mbeattie@sable.ox.ac.uk> wrote:
>Keith Owens writes:
>> All these patches against request_module are attacking the problem at
>> the wrong point.  The kernel can request any module name it likes,
>> using any string it likes, as long as the kernel generates the name.
>> The real problem is when the kernel blindly accepts some user input and
>> passes it straight to modprobe, then the kernel is acting like a setuid
>> wrapper for a program that was never designed to run setuid.
>
>Rather than add sanity checking to modprobe, it would be a lot easier
>and safer from a security audit point of view to have the kernel call
>/sbin/kmodprobe instead of /sbin/modprobe. Then kmodprobe can sanitise
>all the data and exec the real modprobe. That way the only thing that
>needs auditing is a string munging/sanitising program.

<spock>Insufficient data, Captain</spock>.  By the time request_module
is called, the code has no idea if the text is valid or not because all
information about where the string came from is in the higher levels.
Using kmodprobe instead of modprobe cannot change that fact.  As long
as the kernel passes user input to modprobe unchanged, any user can
load any module.  Post validation has no way of catching that.

The kernel is allowed to request module names containing '_', '-', '/'
(see devfs) and possibly ' '.  That kills a lot of the sanitisation
attempts.  OTOH, modprobe can detect that its input came from the
kernel and treat it as tainted, forcing the last parameter to be
treated as a module name, even if it contains special characters and
suppressing meta expansion for this "name".  AFAICT, that makes all
kernel input to modprobe safe.  Wait for modutils 2.3.20 then hunt for
any loopholes.

-
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: Modprobe local root exploit

[Nick Holloway]

duwe@caldera.de (Torsten Duwe) writes:
> Chris Evans <chris@scary.beasts.org> writes:
> > What's wrong with isalnum() ?
> 
> Hm, must admit that I wasn't 100% sure if that's in the kernel lib or an evil
> gcc expands it inline to some static array lookup. Now I see that it's
> already in the kernel. Do some of you also sometimes wonder why the kernel is
> so big ;-) ?

Someone could make it a bit smaller by patching fs/jffs/interp.c and
arch/ppc/xmon/xmon.c to use the kernel lib, rather than their own
versions.

-- 
 `O O'  | Nick.Holloway@pyrites.org.uk
// ^ \\ | http://www.pyrites.org.uk/
-
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: Modprobe local root exploit

[David Woodhouse]

Nick.Holloway@pyrites.org.uk said:
>  Someone could make it a bit smaller by patching fs/jffs/interp.c and
> arch/ppc/xmon/xmon.c to use the kernel lib, rather than their own
> versions. 

Makes sense to me. Patch attached. As an added bonus, this patch (not the
ctype change) also speeds up JFFS mounting by about an order of magnitude - 
by reading from the flash 4KiB at a time into a RAM cache, rather than 
scanning it a word at a time. Yeah, alright - I was looking for an excuse 
to update intrep.c anyway :)

Index: intrep.c
===================================================================
RCS file: /inst/cvs/linux/fs/jffs/Attic/intrep.c,v
retrieving revision 1.1.2.4
diff -u -r1.1.2.4 intrep.c
--- intrep.c	2000/09/11 08:19:11	1.1.2.4
+++ intrep.c	2000/11/14 13:58:20
@@ -10,7 +10,8 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  *
- * $Id: intrep.c,v 1.69 2000/08/24 09:35:47 dwmw2 Exp $
+ * - Based on Id: intrep.c,v 1.71 2000/10/27 16:51:29 dwmw2 Exp
+ * - With the ctype() changes from v1.77.
  *
  * Ported to Linux 2.3.x and MTD:
  * Copyright (C) 2000  Alexander Larsson (alex@cendio.se), Cendio Systems AB
@@ -68,15 +69,11 @@
 #include <linux/version.h>
 #include <linux/smp_lock.h>
 #include <linux/sched.h>
+#include <linux/ctype.h>
 
-
 #include "intrep.h"
 #include "jffs_fm.h"
 
-#if LINUX_VERSION_CODE < 0x20300
-#define set_current_state(x) do{current->state = x;} while (0)
-#endif
-
 #if defined(JFFS_MEMORY_DEBUG) && JFFS_MEMORY_DEBUG
 long no_jffs_file = 0;
 long no_jffs_node = 0;
@@ -94,48 +91,7 @@
 static __u8 flash_read_u8(struct mtd_info *mtd, loff_t from);
 
 #if 1
-#define _U      01
-#define _L      02
-#define _N      04
-#define _S      010
-#define _P      020
-#define _C      040
-#define _X      0100
-#define _B      0200
-
-const unsigned char jffs_ctype_[1 + 256] = {
-	0,
-	_C,     _C,     _C,     _C,     _C,     _C,     _C,     _C,
-	_C,     _C|_S,  _C|_S,  _C|_S,  _C|_S,  _C|_S,  _C,     _C,
-	_C,     _C,     _C,     _C,     _C,     _C,     _C,     _C,
-	_C,     _C,     _C,     _C,     _C,     _C,     _C,     _C,
-	_S|_B,  _P,     _P,     _P,     _P,     _P,     _P,     _P,
-	_P,     _P,     _P,     _P,     _P,     _P,     _P,     _P,
-	_N,     _N,     _N,     _N,     _N,     _N,     _N,     _N,
-	_N,     _N,     _P,     _P,     _P,     _P,     _P,     _P,
-	_P,     _U|_X,  _U|_X,  _U|_X,  _U|_X,  _U|_X,  _U|_X,  _U,
-	_U,     _U,     _U,     _U,     _U,     _U,     _U,     _U,
-	_U,     _U,     _U,     _U,     _U,     _U,     _U,     _U,
-	_U,     _U,     _U,     _P,     _P,     _P,     _P,     _P,
-	_P,     _L|_X,  _L|_X,  _L|_X,  _L|_X,  _L|_X,  _L|_X,  _L,
-	_L,     _L,     _L,     _L,     _L,     _L,     _L,     _L,
-	_L,     _L,     _L,     _L,     _L,     _L,     _L,     _L,
-	_L,     _L,     _L,     _P,     _P,     _P,     _P,     _C
-};
-
-#define jffs_isalpha(c)      ((jffs_ctype_+1)[(int)c]&(_U|_L))
-#define jffs_isupper(c)      ((jffs_ctype_+1)[(int)c]&_U)
-#define jffs_islower(c)      ((jffs_ctype_+1)[(int)c]&_L)
-#define jffs_isdigit(c)      ((jffs_ctype_+1)[(int)c]&_N)
-#define jffs_isxdigit(c)     ((jffs_ctype_+1)[(int)c]&(_X|_N))
-#define jffs_isspace(c)      ((jffs_ctype_+1)[(int)c]&_S)
-#define jffs_ispunct(c)      ((jffs_ctype_+1)[(int)c]&_P)
-#define jffs_isalnum(c)      ((jffs_ctype_+1)[(int)c]&(_U|_L|_N))
-#define jffs_isprint(c)      ((jffs_ctype_+1)[(int)c]&(_P|_U|_L|_N|_B))
-#define jffs_isgraph(c)      ((jffs_ctype_+1)[(int)c]&(_P|_U|_L|_N))
-#define jffs_iscntrl(c)      ((jffs_ctype_+1)[(int)c]&_C)
-
-void
+static void
 jffs_hexdump(struct mtd_info *mtd, loff_t pos, int size)
 {
 	char line[16];
@@ -169,7 +125,7 @@
 		printk("  ");
 
 		for (i = 0; i < j; i++) {
-			if (jffs_isgraph(line[i])) {
+			if (isgraph(line[i])) {
 				printk("%c", line[i]);
 			}
 			else {
@@ -193,9 +149,12 @@
 	size_t retlen;
 	int res;
 
+	D3(printk(KERN_NOTICE "flash_safe_read(%p, %08x, %p, %08x)\n",
+		  mtd, from, buf, count));
+
 	res = MTD_READ(mtd, from, count, &retlen, buf);
 	if (retlen != count) {
-		printk("Didn't read all bytes in flash_safe_read(). Returned %d\n", res);
+		panic("Didn't read all bytes in flash_safe_read(). Returned %d\n", res);
 	}
 	return res?res:retlen;
 }
@@ -367,9 +326,37 @@
 {
 	__u32 sum = 0;
 	loff_t ptr = start;
-	while (size-- > 0) {
-		sum += flash_read_u8(mtd, ptr++);
+	__u8 *read_buf;
+	int i, length;
+
+	/* Allocate read buffer */
+	read_buf = (__u8 *) kmalloc (sizeof(__u8) * 4096, GFP_KERNEL);
+
+	/* Loop until checksum done */
+	while (size) {
+		/* Get amount of data to read */
+		if (size < 4096)
+			length = size;
+		else
+			length = 4096;
+
+		/* Perform flash read */
+		D3(printk(KERN_NOTICE "jffs_checksum_flash\n"));
+		flash_safe_read(mtd, ptr, &read_buf[0], length);
+
+		/* Compute checksum */
+		for (i=0; i < length ; i++)
+			sum += read_buf[i];
+
+		/* Update pointer and size */
+		size -= length;
+		ptr += length;
 	}
+
+	/* Free read buffer */
+	kfree (read_buf);
+
+	/* Return result */
 	D3(printk("checksum result: 0x%08x\n", sum));
 	return sum;
 }
@@ -609,12 +596,17 @@
 	loff_t pos = fmc->flash_start;
 	loff_t start;
 	loff_t end = fmc->flash_start + fmc->flash_size;
+	__u8 *read_buf;
+	int i, len, retlen;
 
 	D1(printk("jffs_scan_flash(): start pos = 0x%lx, end = 0x%lx\n",
 		  (long)pos, (long)end));
 
 	flash_safe_acquire(fmc->mtd);
 
+	/* Allocate read buffer */
+	read_buf = (__u8 *) kmalloc (sizeof(__u8) * 4096, GFP_KERNEL);
+
 	/* Start the scan.  */
 	while (pos < end) {
 		deleted_file = 0;
@@ -629,9 +621,22 @@
 			   something else than 0xff is found.  */
 			D1(printk("jffs_scan_flash(): 0xff at pos 0x%lx.\n",
 				  (long)pos));
-			for (; pos < end
-			       && JFFS_EMPTY_BITMASK == flash_read_u32(fmc->mtd, pos);
-			     pos += 4);
+
+			len = end - pos < 4096 ? end - pos : 4096;
+
+			retlen = flash_safe_read(fmc->mtd, pos,
+						 &read_buf[0], len);
+
+			retlen &= ~3;
+
+			for (i=0 ; i < retlen ; i+=4, pos += 4) {
+				if(*((__u32 *) &read_buf[i]) !=
+						JFFS_EMPTY_BITMASK)
+					break;
+			}
+			if (i == retlen)
+				continue;
+
 			D1(printk("jffs_scan_flash(): 0xff ended at "
 				  "pos 0x%lx.\n", (long)pos));
 
@@ -748,7 +753,12 @@
 			if (!(node = (struct jffs_node *)
 				     kmalloc(sizeof(struct jffs_node),
 					     GFP_KERNEL))) {
+				/* Free read buffer */
+				kfree (read_buf);
+
+				/* Release the flash device */
 				flash_safe_release(fmc->mtd);
+	
 				return -ENOMEM;
 			}
 			DJM(no_jffs_node++);
@@ -893,7 +903,13 @@
 				D(printk("jffs_scan_flash(): !node->fm\n"));
 				kfree(node);
 				DJM(no_jffs_node--);
+
+				/* Free read buffer */
+				kfree (read_buf);
+
+				/* Release the flash device */
 				flash_safe_release(fmc->mtd);
+
 				return -ENOMEM;
 			}
 			if ((err = jffs_insert_node(c, 0, &raw_inode,
@@ -911,7 +927,13 @@
 					D(printk("jffs_scan_flash: !dl\n"));
 					kfree(node);
 					DJM(no_jffs_node--);
+
+					/* Release the flash device */
 					flash_safe_release(fmc->flash_part);
+
+					/* Free read buffer */
+					kfree (read_buf);
+
 					return -ENOMEM;
 				}
 				dl->ino = deleted_file;
@@ -936,6 +958,11 @@
 		DJM(no_jffs_node--);
 	}
 	jffs_build_end(fmc);
+
+	/* Free read buffer */
+	kfree (read_buf);
+
+	/* Return happy */
 	D3(printk("jffs_scan_flash(): Leaving...\n"));
 	flash_safe_release(fmc->mtd);
 	return 0;
@@ -1598,6 +1625,7 @@
 		  f->name, node->ino, node->version, node_offset));
 
 	r = jffs_min(avail, max_size);
+	D3(printk(KERN_NOTICE "jffs_get_node_data\n"));
 	flash_safe_read(fmc->mtd, pos, buf, r);
 
 	D3(printk("  jffs_get_node_data(): Read %u byte%s.\n",


--
dwmw2


-
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: Modprobe local root exploit

[Horst von Brand]

Chris Evans <chris@scary.beasts.org> said:
> On Mon, 13 Nov 2000, Torsten Duwe wrote:
> > >>>>> "Francis" == Francis Galiegue <fg@mandrakesoft.com> writes:
> > 
> >     >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
> > 
> >     Francis> Just in case... Some modules have uppercase letters too :)
> > 
> > That's what the &0xdf is intended for...

> Code in a security sensitive area needs to be crystal clear.

Nodz!

> What's wrong with isalnum() ?

Too efficient? ;)
--
Horst von Brand                             vonbrand@sleipnir.valparaiso.cl
Casilla 9G, Vin~a del Mar, Chile                               +56 32 672616
-
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: Modprobe local root exploit

[Peter Samuelson]

[Torsten Duwe]
> >>>>> "Francis" == Francis Galiegue <fg@mandrakesoft.com> writes:
> 
>     >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
> 
>     Francis> Just in case... Some modules have uppercase letters too :)
> 
> That's what the &0xdf is intended for...

It's wrong, then: you've converted to uppercase, not lowercase.

request_module is not a fast path.  Do it the obvious, unoptimized way:

  if ((*p < 'a' || *p > 'z') &&
      (*p < 'A' || *p > 'Z') &&
      (*p < '0' || *p > '9') &&
      *p != '-' && *p != '_')
    return -EINVAL;

Peter
-
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: Modprobe local root exploit

[Daniel Phillips]

Peter Samuelson wrote:
> 
> [Torsten Duwe]
> > >>>>> "Francis" == Francis Galiegue <fg@mandrakesoft.com> writes:
> >
> >     >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
> >
> >     Francis> Just in case... Some modules have uppercase letters too :)
> >
> > That's what the &0xdf is intended for...
> 
> It's wrong, then: you've converted to uppercase, not lowercase.
> 
> request_module is not a fast path.  Do it the obvious, unoptimized way:
> 
>   if ((*p < 'a' || *p > 'z') &&
>       (*p < 'A' || *p > 'Z') &&
>       (*p < '0' || *p > '9') &&
>       *p != '-' && *p != '_')
>     return -EINVAL;

Heading in the right direction, but this is equivalent to:

  if (isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;

which is faster, smaller and easier to read.

--
Daniel
-
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: Modprobe local root exploit

[Daniel Phillips]

On Tue, 14 Nov 2000, David Relson wrote:
> At 06:29 AM 11/14/00, Daniel Phillips wrote:
> 
> >Heading in the right direction, but this is equivalent to:
> >
> >   if (isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;
> >
> >which is faster, smaller and easier to read.
> 
> Almost right, but you forgot to negate isalnum().  Should be:
> 
>          if (!isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;
> 
> or
>          if (! (isalnum(*p) || *p == '-' || *p == '_')) return -EINVAL;
> 
> I think I prefer the older version with "continue" as I don't have to think 
> about all the negatives ("!"), i.e.
> 
>          for ( ... )
>          {
>                  if ( isalnum(*p) || *p == '-' || *p == '_' )
>                          continue;
>                  return -EINVAL;
>          }
> 

I reserve the right to make coding errors, thanks for not letting it get
written into history :-)

How about:

  for ( ... ) if (!isalnum(*p) && !strchr("-_", *p)) return -EINVAL;

-- 
Daniel
-
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: Modprobe local root exploit

[David Relson]

Daniel,

At 09:23 AM 11/14/00, you wrote:
I reserve the right to make coding errors, thanks for not letting it get
written into history :-)

I'm not going to give up my right to make errors until I'm ready to give up 
my keyboard.  I'll probably be pushing up daisies at that point in my life.


How about:

   for ( ... ) if (!isalnum(*p) && !strchr("-_", *p)) return -EINVAL;


I think that is correct.  However, it fails the "easy to understand" 
criterion, so I don't like it.]

David
--------------------------------------------------------
David Relson                   Osage Software Systems, Inc.
relson@osagesoftware.com       514 W. Keech Ave.
www.osagesoftware.com          Ann Arbor, MI 48103
voice: 734.821.8800            fax: 734.821.8800

-
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: Modprobe local root exploit

[Horst von Brand]

Daniel Phillips <news-innominate.list.linux.kernel@innominate.de> said:

[...]

> Heading in the right direction, but this is equivalent to:
> 
>   if (isalnum(*p) && *p != '-' && *p != '_') return -EINVAL;
> 
> which is faster, smaller and easier to read.

And wrong. ;-)
--
Horst von Brand                             vonbrand@sleipnir.valparaiso.cl
Casilla 9G, Vin~a del Mar, Chile                               +56 32 672616
-
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: Modprobe local root exploit

[Alan Cox]

>     >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
> 
>     Francis> Just in case... Some modules have uppercase letters too :)
> 
> That's what the &0xdf is intended for...

That looks wrong for UTF8 which is technically what the kernel uses 8)
-
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: Modprobe local root exploit

[H. Peter Anvin]

Followup to:  <E13wHVO-0007VB-00@the-village.bc.nu>
By author:    Alan Cox <alan@lxorguk.ukuu.org.uk>
In newsgroup: linux.dev.kernel
>
> >     >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
> > 
> >     Francis> Just in case... Some modules have uppercase letters too :)
> > 
> > That's what the &0xdf is intended for...
> 
> That looks wrong for UTF8 which is technically what the kernel uses 8)
> 

No, it's correct, actually, but probably not what you want.  It will
include all letters [A-Za-z], but if a module named "ärlig"...

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
-
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: Modprobe local root exploit

[Keith Owens]

On 15 Nov 2000 22:04:47 -0800, 
"H. Peter Anvin" <hpa@zytor.com> wrote:
>No, it's correct, actually, but probably not what you want.  It will
>include all letters [A-Za-z], but if a module named "ärlig"...

Trying to sanitise the module name in request_module is the wrong fix
anyway, the kernel can ask for any module name it likes.  What it must
not do is treat user supplied input _unchanged_ as a module name.

modutils 2.3.20 (just released) fixes all the known local root
exploits, without kernel changes.  However 2.3.20 does nothing about
this problem: "ping6 -I module_name" which lets any user load any
module.  That problem exists because the kernel passes user supplied
data unchanged to request_module.  The only fix is to add a prefix to
user supplied input (say 'user-interface-') before passing the text to
request_module.  This has to be fixed in the higher layers of the
kernel, it cannot be fixed in request_module or modprobe.

-
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: Modprobe local root exploit

[H. Peter Anvin]

Keith Owens wrote:
> 
> On 15 Nov 2000 22:04:47 -0800,
> "H. Peter Anvin" <hpa@zytor.com> wrote:
> >No, it's correct, actually, but probably not what you want.  It will
> >include all letters [A-Za-z], but if a module named "ärlig"...
> 
> Trying to sanitise the module name in request_module is the wrong fix
> anyway, the kernel can ask for any module name it likes.  What it must
> not do is treat user supplied input _unchanged_ as a module name.
> 
> modutils 2.3.20 (just released) fixes all the known local root
> exploits, without kernel changes.  However 2.3.20 does nothing about
> this problem: "ping6 -I module_name" which lets any user load any
> module.  That problem exists because the kernel passes user supplied
> data unchanged to request_module.  The only fix is to add a prefix to
> user supplied input (say 'user-interface-') before passing the text to
> request_module.  This has to be fixed in the higher layers of the
> kernel, it cannot be fixed in request_module or modprobe.
> 

Sure, but if you have to change the kernel anyway you ought to pass the
"--" option so modprobe knows that regardless what the string is, it's a
module name and not an option.

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
-
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: Modprobe local root exploit

[Torsten Duwe]

    >> >> + if ((*p & 0xdf) >= 'a' && (*p & 0xdf) <= 'z') continue;
    >> 
    Francis> Just in case... Some modules have uppercase letters too :)
    >> That's what the &0xdf is intended for...

Jah, Bummer from my side; use "|0x20" instead. But as discussed, isalnum()
does the perfect job, for readability and efficiency.

    Alan> That looks wrong for UTF8 which is technically what the kernel uses
    Alan> 8)

Hmm, haf-a-amiley. Are module names to be localized or are they considered
"international code" like the sources, limiting them to 7-Bit ASCII.

What's your opinion, Alan ? Linus ?

I'd consider it "system internal", not visible to the user and hence 7-Bit
must suffice. I also strongly agree with Keith: treating strings that come
from the kernel as tainted is weird at least.

I suggest to stick with [A-Za-z0-9_-]*, adding a check for the first char not
being '-', maybe modifying devfs do use dashes ("dev-") and auditing the rest
of the kernel BTW. The M$-FSes look a little suspicious to me with their
"nls_*" stuff. CAP_SYS_MOUNT (once established) might then be turned into
CAP_SYS_MODULE this way (mount -t vfat -o conv="; chmod ...") ???

Keith: what about the good-ole' "--" to specify end-of-options ? Every word
after that could be treated as a simple module name.

	Torsten

-
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: Modprobe local root exploit

[Alan Cox]

> I'd consider it "system internal", not visible to the user and hence 7-Bit
> must suffice. I also strongly agree with Keith: treating strings that come
> from the kernel as tainted is weird at least.

I would consider it to be an arbitary 8bit bytesequence. Fix the user space

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