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.
>>>>> "Gregory" == Gregory Maxwell <[email protected]> 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 ) {
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, [email protected]
"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
>>>>> "Francis" == Francis Galiegue <[email protected]> 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
On Mon, 13 Nov 2000, Torsten Duwe wrote:
> >>>>> "Francis" == Francis Galiegue <[email protected]> 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
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 ?
>>>>> "Chris" == Chris Evans <[email protected]> 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 ) {
[Torsten Duwe]
> >>>>> "Francis" == Francis Galiegue <[email protected]> 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
Chris Evans <[email protected]> said:
> On Mon, 13 Nov 2000, Torsten Duwe wrote:
> > >>>>> "Francis" == Francis Galiegue <[email protected]> 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 [email protected]
Casilla 9G, Vin~a del Mar, Chile +56 32 672616
[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
On Mon, 13 Nov 2000 23:02:10 -0600,
Peter Samuelson <[email protected]> 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.
Keith Owens <[email protected]> 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 [email protected]
University of Stuttgart http://cert.uni-stuttgart.de/
RUS-CERT +49-711-685-5973/fax +49-711-685-5898
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 <[email protected]>
Unix Systems Programmer
Oxford University Computing Services
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
On Tue, 14 Nov 2000 10:42:41 +0000,
Malcolm Beattie <[email protected]> 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.
Peter Samuelson wrote:
>
> [Torsten Duwe]
> > >>>>> "Francis" == Francis Galiegue <[email protected]> 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
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
[email protected] (Torsten Duwe) writes:
> Chris Evans <[email protected]> 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' | [email protected]
// ^ \\ | http://www.pyrites.org.uk/
[email protected] 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 ([email protected]), 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
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
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.
[email protected] 514 W. Keech Ave.
http://www.osagesoftware.com Ann Arbor, MI 48103
voice: 734.821.8800 fax: 734.821.8800
Daniel Phillips <[email protected]> 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 [email protected]
Casilla 9G, Vin~a del Mar, Chile +56 32 672616
> >> + 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)
Followup to: <[email protected]>
By author: Alan Cox <[email protected]>
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
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
On 15 Nov 2000 22:04:47 -0800,
"H. Peter Anvin" <[email protected]> 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.
Keith Owens wrote:
>
> On 15 Nov 2000 22:04:47 -0800,
> "H. Peter Anvin" <[email protected]> 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
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt
>> >> + 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
> 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