Prevent offset + size overflow.
PATCH 1/1 Prevent offset + size overflow.
Hi,
it is possible to overflow uint64_t by summing variables offset and size up in elf_get_section_info. Thee values are extracted from module file and are possibly maliciously tampered with.
If offset is in valid range and size very large, the result will overflow and the size check passes. Later on, this will most likely lead to a segmentation fault due to accessing uninitialized memory.
Attached please find a proof of concept module, which will trigger a segmentation fault on modinfo. Tested on amd64:
tobias:~$ modinfo poc.ko filename: /home/tobias/poc.ko Segmentation fault
Tobias
PS: There are more errors of this type in the ELF handling code, so let
me know if you are okay with the additional check in the if-block. I will send patches like this one for the other occurrences then.
--- libkmod/libkmod-elf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
libkmod/libkmod-elf.c +1 -1
@@ -251,7 +251,7 @@ static inline int elf_get_section_info(const struct kmod_elf *elf, uint16_t idx,#undef READV min_size = *offset + *size; -if (min_size > elf->size) { +if (ULLONG_MAX - *offset < *size || min_size > elf->size) {ELFDBG (elf,"out-of-bounds: %" PRIu64" >= %" PRIu64" (ELF size)\n" , min_size, elf->size);return -EINVAL ;
Signature · 1 line
2.3.0
Quoted 9 lines · Tobias Stoeckmann
Hi,
it is possible to overflow
uint64_tby summing variables offset and size up inelf_get_section_info. Thee values are extracted from module file and are possibly maliciously tampered with.If offset is in valid range and size very large, the result will overflow and the size check passes. Later on, this will most likely lead to a segmentation fault due to accessing uninitialized memory.
Indeed, thanks for looking into this.
Quoted 9 lines
Attached please find a proof of concept module, which will trigger a segmentation fault on modinfo. Tested on amd64:
tobias:~$ modinfo poc.ko filename: /home/tobias/poc.ko Segmentation fault
Quoted 5 lines
Tobias
PS: There are more errors of this type in the ELF handling code, so let
me know if you are okay with the additional check in the if-block. I will send patches like this one for the other occurrences then.
The more critical ones (if any) are the ones in the path of loading a module. For modinfo and depmod, although desirable to have it fixed it's not as critical since there's little use of this code outside of kmod tools. Anyway the fix looks good I would accept for fixes like this. However see the suggestion below.
Quoted 9 lines
--- libkmod/libkmod-elf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)libkmod/libkmod-elf.c +0
-0
libkmod/libkmod-elf.c static inline int elf_get_section_info(const struct kmod_elf *elf, uint16_t idx, · line 254
Quoted 5 lines
libkmod/libkmod-elf.c +1
-1@@ -251,7 +251,7 @@ static inline int elf_get_section_info(const struct kmod_elf *elf, uint16_t idx,#undef READV min_size = *offset + *size; -if (min_size > elf->size) { +if (ULLONG_MAX - *offset < *size || min_size > elf->size) {
could we use __builtin_uaddl_overflow() for this? then it would be (whitespace damaged):
libkmod/libkmod-elf.c +0 -0
@@ -250,8 +250,8 @@ static inline int elf_get_section_info(const
struct kmod_elf *elf, uint16_t idx,
}
#undef READV
- min_size = *offset + *size;
- if (min_size > elf->size) {
+ if (__builtin_uaddl_overflow(*offset, *size, &min_size)
+ || min_size > elf->size) {
ELFDBG(elf, "out-of-bounds: %"PRIu64" >= %"PRIu64"
(ELF size)\n",
min_size, elf->size);
return -EINVAL;AFAICS only gcc >= 5.0 supports this builtin (clang also has it). So we may want to add a fallback in missing.h. I just added the support in the build system so we can check for this builtin.
thanks again.
Signature · 1 line
Lucas De Marchi
Hi,
Review
Quoted 9 lines
Quoted 2 lines · Lucas De Marchi
(quoted diff) +1
-1- if (min_size > elf->size) { + if (ULLONG_MAX - *offset < *size || min_size > elf->size) {(quoted diff) +2
-2- min_size = *offset + *size; - if (min_size > elf->size) { + if (__builtin_uaddl_overflow(*offset, *size, &min_size) + || min_size > elf->size) {
If at all, it would have to be __builtin_uaddll_overflow due to uint64_t even on 32 bit systems. But even then the prototype looks a bit off to me:
from https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html__builtin_uaddll_overflow (unsigned long long int a, unsigned long long int b, unsigned long int *res)
I hope it's a typo and they meant "unsigned long long int *res", otherwise that built-in function by itself is prone to truncation.
If it works, i.e. the poc.ko module doesn't trigger a segmentation fault, I am fine with that solution. When it's in the tree, I will create fixes for the other occurrences in that style as well.
Please keep in mind that these libkmod issues are not limited to just modinfo. The tool modinfo is just the easiest way to trigger them, because it doesn't need any advanced permissions.
Tobias
Quoted 3 lines · On Tue, Feb 10, 2015 at 6:26 AM, Tobias St=C3=B6ckmann <[email protected]=…
Hi,
Quoted 1 line
On February 10, 2015 at 3:56 AM Lucas De Marchi <[email protected]=
om>
Quoted 10 lines
Review
Quoted 8 lines
Quoted 2 lines · wrote:
(quoted diff) +1
-1- if (min_size > elf->size) { + if (ULLONG_MAX - *offset < *size || min_size > elf->size) {(quoted diff) +2
-2- min_size =3D *offset + *size; - if (min_size > elf->size) { + if (__builtin_uaddl_overflow(*offset, *size, &min_size) + || min_size > elf->size) {If at all, it would have to be
__builtin_uaddll_overflowdue touint64_t=
even on
Quoted 1 line
32 bit systems. But even then the prototype looks a bit off to me:
sizeof(uint64_t) =3D=3D sizeof(unsigned long) both on 32 and 64 bits... no need to use long long.
Quoted 2 lines
from https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
__builtin_uaddll_overflow(unsigned long long int a, unsigned long long i=
nt b,
Quoted 3 lines
unsigned long int *res)
I hope it's a typo and they meant "unsigned long long int *res", otherwis=
e that
Quoted 1 line
built-in function by itself is prone to truncation.
yep, it seems to be a typo
Quoted 1 line
If it works, i.e. the poc.ko module doesn't trigger a segmentation fault,=
I am
Quoted 1 line
fine with that solution. When it's in the tree, I will create fixes for t=
he
Quoted 1 line
other occurrences in that style as well.
are you going to submit the patch or you're waiting on me?
Quoted 1 line
Please keep in mind that these libkmod issues are not limited to just mod=
info.
Quoted 1 line
The tool modinfo is just the easiest way to trigger them, because it does=
n't
Quoted 1 line
need any advanced permissions.
yep, it'd be good to have the other fixes, even more than this one. If this is pointed out by any static analysis tool, feel free to add that in the commit message.
thanks
--=20
Lucas De Marchi
Hi again,
Quoted 4 lines
On February 10, 2015 at 11:31 AM Lucas De Marchi <[email protected]> wrote:
sizeof(uint64_t)== sizeof(unsigned long) both on 32 and 64 bits... no need to use long long.
long on 32 bit systems (at least my i686) is 32 bit -- granted, the C standard doesn't explicitly require that. Yet I didn't encounter a 32 bit Linux with 64 bit long, please correct me if I'm wrong and there are such systems out there. long long would be 64 bit on both.
Quoted 1 line
are you going to submit the patch or you're waiting on me?
I have no commit access, so I am waiting for you. :) As stated before, please use the long long variant.
Quoted 2 lines
If this is pointed out by any static analysis tool, feel free to add that in the commit message.
It's a review by hand. But speaking of analysis, I wonder what afl could come up with...
Tobias
Quoted 3 lines · On Tue, Feb 10, 2015 at 8:44 AM, Tobias St=C3=B6ckmann <[email protected]=…
Hi again,
Quoted 1 line
On February 10, 2015 at 11:31 AM Lucas De Marchi <lucas.de.marchi@gmail.=
com>
Quoted 2 lines
Quoted 2 lines
wrote:
sizeof(uint64_t)=3D=3D sizeof(unsigned long) both on 32 and 64 bits... =
no
Quoted 3 lines
Quoted 1 line
need to use long long.
long on 32 bit systems (at least my i686) is 32 bit -- granted, the C sta=
ndard
Quoted 1 line
doesn't explicitly require that. Yet I didn't encounter a 32 bit Linux wi=
th 64
Quoted 1 line
bit long, please correct me if I'm wrong and there are such systems out t=
here.
Quoted 1 line
long long would be 64 bit on both.
oohh... I blame the lack of coffee early in the morning for my stupid comment. Indeed, you are right.
Quoted 5 lines
Quoted 2 lines
are you going to submit the patch or you're waiting on me?
I have no commit access, so I am waiting for you. :) As stated before, please use the long long variant.
well, you could submit to the mailing list a patch series :-)
Anyway, I applied this patch, added addu64_overflow() abstracting if the compiler has the builtin and converted your patch to use this new function.
Let me know of the other problems you talked about.
thanks
--=20
Lucas De Marchi
Quoted 9 lines
On February 10, 2015 at 1:55 PM Lucas De Marchi <[email protected]> wrote:
well, you could submit to the mailing list a patch series :-)Anyway, I applied this patch, added
addu64_overflow()abstracting if the compiler has the builtin and converted your patch to use this new function.Let me know of the other problems you talked about.
Sure thing, I will send patches with the addu64_overflow() merged into them when I'm back home.
Thanks for your fast response,
Tobias
Quoted 1 line · On Tue, Feb 10, 2015 at 6:26 AM, Tobias St=C3=B6ckmann <[email protected]=…
If it works, i.e. the poc.ko module doesn't trigger a segmentation fault,=
I am
Quoted 1 line
fine with that solution. When it's in the tree, I will create fixes for t=
he
Quoted 1 line
other occurrences in that style as well.
yeah, it works
kmod $ ./tools/modinfo ~/Downloads/poc.ko
filename: /home/lucas/Downloads/poc.ko
modinfo: ERROR: could not get modinfo from 'poc': Invalid argument
--=20
Lucas De Marchi