[PATCH] Prevent offset + size overflow.

[PATCH] Prevent offset + size overflow.

[Tobias Stoeckmann]

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

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(-)

diff --git a/libkmod/libkmod-elf.c b/libkmod/libkmod-elf.c
index d1b0f33..8a8a73d 100644
--- a/libkmod/libkmod-elf.c
+++ b/libkmod/libkmod-elf.c
@@ -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;
-- 
2.3.0


[-- Attachment #2: poc.ko --]
[-- Type: application/octet-stream, Size: 208 bytes --]

Re: [PATCH] Prevent offset + size overflow.

[Lucas De Marchi]

On Mon, Feb 9, 2015 at 8:22 PM, Tobias Stoeckmann <tobias@stoeckmann.org> wrote:
> 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.

Indeed, thanks for looking into this.

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

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.

> ---
>  libkmod/libkmod-elf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libkmod/libkmod-elf.c b/libkmod/libkmod-elf.c
> index d1b0f33..8a8a73d 100644
> --- a/libkmod/libkmod-elf.c
> +++ b/libkmod/libkmod-elf.c
> @@ -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):

diff --git a/libkmod/libkmod-elf.c b/libkmod/libkmod-elf.c
index d1b0f33..c8c922c 100644
--- a/libkmod/libkmod-elf.c
+++ b/libkmod/libkmod-elf.c
@@ -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.

-- 
Lucas De Marchi

Re: [PATCH] Prevent offset + size overflow.

[Tobias Stöckmann]

Hi,

> On February 10, 2015 at 3:56 AM Lucas De Marchi <lucas.de.marchi@gmail.com>
> wrote:
> > -       if (min_size > elf->size) {
> > +       if (ULLONG_MAX - *offset < *size || min_size > elf->size) {

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

Re: [PATCH] Prevent offset + size overflow.

[Lucas De Marchi]

On Tue, Feb 10, 2015 at 6:26 AM, Tobias St=C3=B6ckmann <tobias@stoeckmann.o=
rg> wrote:
> Hi,
>
>> On February 10, 2015 at 3:56 AM Lucas De Marchi <lucas.de.marchi@gmail.c=
om>
>> wrote:
>> > -       if (min_size > elf->size) {
>> > +       if (ULLONG_MAX - *offset < *size || min_size > elf->size) {
>
>> -       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_overflow due to uint64_t =
even on
> 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.

> 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,
> unsigned long int *res)
>
> I hope it's a typo and they meant "unsigned long long int *res", otherwis=
e that
> built-in function by itself is prone to truncation.

yep, it seems to be a typo

> 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 t=
he
> other occurrences in that style as well.

are you going to submit the patch or you're waiting on me?

> Please keep in mind that these libkmod issues are not limited to just mod=
info.
> The tool modinfo is just the easiest way to trigger them, because it does=
n't
> 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

Re: [PATCH] Prevent offset + size overflow.

[Lucas De Marchi]

On Tue, Feb 10, 2015 at 6:26 AM, Tobias St=C3=B6ckmann <tobias@stoeckmann.o=
rg> wrote:
> 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 t=
he
> 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

Re: [PATCH] Prevent offset + size overflow.

[Tobias Stöckmann]

Hi again,

> On February 10, 2015 at 11:31 AM Lucas De Marchi <lucas.de.marchi@gmail.com>
> 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.

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

> 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

Re: [PATCH] Prevent offset + size overflow.

[Lucas De Marchi]

On Tue, Feb 10, 2015 at 8:44 AM, Tobias St=C3=B6ckmann <tobias@stoeckmann.o=
rg> wrote:
> Hi again,
>
>> On February 10, 2015 at 11:31 AM Lucas De Marchi <lucas.de.marchi@gmail.=
com>
>> wrote:
>> sizeof(uint64_t) =3D=3D 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 sta=
ndard
> doesn't explicitly require that. Yet I didn't encounter a 32 bit Linux wi=
th 64
> bit long, please correct me if I'm wrong and there are such systems out t=
here.
> 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.

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

Re: [PATCH] Prevent offset + size overflow.

[Tobias Stöckmann]

> On February 10, 2015 at 1:55 PM Lucas De Marchi <lucas.de.marchi@gmail.com>
> 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