| a0440a66c3418842f309fc4f78f2aad87ba6c96f |
|
31-Aug-2009 |
Stephen Smalley <sds@tycho.nsa.gov> |
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 <sds@tycho.nsa.gov>
/external/selinux/libsepol/src/module.c
|