From a0440a66c3418842f309fc4f78f2aad87ba6c96f Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Mon, 31 Aug 2009 16:37:40 -0400 Subject: [PATCH] Unchecked input leades to integer underflow On Mon, 2009-08-31 at 08:55 -0500, Manoj Srivastava wrote: > On Mon, Aug 31 2009, Stephen Smalley wrote: > > > On Sun, 2009-08-30 at 10:19 -0500, Manoj Srivastava wrote: > >> Hi, > >> > >> This bug was discovered, and the analysis done, buy Max > >> Kellermann. I have never been able to replicate the problem, so I can't > >> help debug this error. > >> > >> Strace: > >> --8<---------------cut here---------------start------------->8--- > >> brk(0x3233000) = 0x3233000 > >> mmap(NULL, 18446744073703178240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory) > >> mmap(NULL, 18446744073703313408, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory) > >> mmap(NULL, 134217728, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x7fdfda316000 > >> --8<---------------cut here---------------end--------------->8--- > >> > >> > 0xffffffffff9ec000 == 18446744073703178240 (the size of the first > >> > large allocation). It's also equal to -6373376. This just looks like > >> > an integer underflow, doesn't it? > >> > >> --8<---------------cut here---------------start------------->8--- > >> Breakpoint 4, 0x00007f9bc4c05400 in mmap64 () from /lib/libc.so.6 > >> (gdb) p $rsi > >> $25 = -6373376 > >> (gdb) bt > >> #0 0x00007f9bc4c05400 in mmap64 () from /lib/libc.so.6 > >> #1 0x00007f9bc4baf6bb in _int_malloc () from /lib/libc.so.6 > >> #2 0x00007f9bc4bb0a78 in malloc () from /lib/libc.so.6 > >> #3 0x00007f9bc5301a8e in sepol_module_package_read (mod=0xb1d170, spf=0xb202e0, verbose=0) at module.c:533 > >> #4 0x00007f9bc4ea7838 in ?? () from /lib/libsemanage.so.1 > >> > >> (gdb) frame 3 > >> #3 0x00007f9bc5301a8e in sepol_module_package_read (mod=0xb1d170, spf=0xb202e0, verbose=0) at module.c:533 > >> 533 module.c: No such file or directory. > >> in module.c > >> (gdb) p len > >> $26 = 18446744073703176358 > >> (gdb) p i > >> $27 = 3 > >> (gdb) p nsec > >> $30 = 4 > >> (gdb) p offsets[i+1] > >> $28 = 8192 > >> (gdb) p offsets[i] > >> $29 = 6383450 > >> --8<---------------cut here---------------end--------------->8--- > >> > >> > line 456: > >> > len = offsets[i + 1] - offsets[i]; > >> > >> > Voila, integer underflow. The function module_package_read_offsets() > >> > reads the offsets from the input file, but does not verify them. > >> > off[nsec] = policy_file_length(file); > >> > Here, the check is missing. > >> > >> We should probably have: > >> --8<---------------cut here---------------start------------->8--- > >> off[nsec] = policy_file_length(file); > >> if (off[nsec] < off[nsec-1]) { > >> ERR(file->handle, "file size smaller than previous offset (at %u, " > >> "offset %zu -> %zu", nsec, off[nsec - 1], > >> off[nsec]); > >> return -1; > >> } > >> --8<---------------cut here---------------end--------------->8--- > > > > Perhaps I am missing something, but module_package_read_offsets() > > already checks that the offsets are increasing and aborts if not. > > Well, almost. It does check for most of the offsets: > --8<---------------cut here---------------start------------->8--- > > 406 for (i = 0; i < nsec; i++) { > 407 off[i] = le32_to_cpu(buf[i]); > 408 if (i && off[i] < off[i - 1]) { > 409 ERR(file->handle, "offsets are not increasing (at %u, " > 410 "offset %zu -> %zu", i, off[i - 1], > 411 off[i]); > 412 return -1; > 413 } > 414 } > --8<---------------cut here---------------end--------------->8--- > So far, so good. > --8<---------------cut here---------------start------------->8--- > 415 > 416 free(buf); > 417 off[nsec] = policy_file_length(file); > 418 *offsets = off; > 419 return 0; > --8<---------------cut here---------------end--------------->8--- > > The problem is line 417, where there is no check; and in the > case reported, the file length was less than the previous offset, and > this resulted in a negative number passed to the memory allocator, > which resulted in a huge allocation request. > > Above, I just propose adding a check after line 417. Check the last offset against the file size, and ensure that we free the buffer and offset array in the error cases. Signed-off-by: Stephen Smalley --- libsepol/src/module.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libsepol/src/module.c b/libsepol/src/module.c index 3337808b..b5b807ee 100644 --- a/libsepol/src/module.c +++ b/libsepol/src/module.c @@ -409,13 +409,19 @@ static int module_package_read_offsets(sepol_module_package_t * mod, ERR(file->handle, "offsets are not increasing (at %u, " "offset %zu -> %zu", i, off[i - 1], off[i]); - return -1; + goto err; } } - free(buf); off[nsec] = policy_file_length(file); + if (nsec && off[nsec] < off[nsec-1]) { + ERR(file->handle, "offset greater than file size (at %u, " + "offset %zu -> %zu", nsec, off[nsec - 1], + off[nsec]); + goto err; + } *offsets = off; + free(buf); return 0; err: