New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

read_fragment_table_n: Fix recommendation for stack overflow. #5

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Jun 17, 2015

There seems to be a stack overflow and an integer overflow in read_fragment_table_4.

CVE-2015-4645

The first problem overflows the bytes variable, so that the allocation of fragments_bytes[] has an erroneous size.

int bytes = SQUASHFS_FRAGMENT_BYTES(sBlk.s.fragments);
...
fragment_table = malloc(bytes);
CVE-2015-4646

If we fix this by making the variable size_t, we run into an unrelated problem in which the stack VLA allocation of fragment_table_index[] can easily exceed RLIMIT_STACK.

long long fragment_table_index[indexes];

In the case of my system, the RLIMIT_STACK is 8388608 bytes, and VLA is asking for 15728648 bytes. This is what I believe ultimately leads to the stack overflow. Plus the stack probably already has a bunch of other things, and I don't know a portable way to check how much stack space is available.

Afterwards, the disastrous call to read_fs_bytes is made, which initiates the transfer from the squashfs image to the stack. At this stage, a stack overflow appears to be in full effect.

 res = read_fs_bytes(fd, sBlk.s.fragment_table_start,
         SQUASHFS_FRAGMENT_INDEX_BYTES(sBlk.s.fragments),
         fragment_table_index);

ASAN Output

Parallel unsquashfs: Using 8 processors
ASAN:SIGSEGV
=================================================================
==8221==ERROR: AddressSanitizer: stack-overflow on address 0x7ffef3ae9608 (pc 0x000000559011 bp 0x7ffef49e9670 sp 0x7ffef3ae9610 T0)
    #0 0x559010 in read_fragment_table_4 /home/septimus/vr/squashfs-vr/squashfs-tools/unsquash-4.c:40:9
    #1 0x525073 in main /home/septimus/vr/squashfs-vr/squashfs-tools/unsquashfs.c:2763:5
    #2 0x7fb56c533a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)
    #3 0x418468 in _start (/home/septimus/vr/squashfs-vr/squashfs-tools/unsquashfs+0x418468)
SUMMARY: AddressSanitizer: stack-overflow /home/septimus/vr/squashfs-vr/squashfs-tools/unsquash-4.c:40:9 in read_fragment_table_4
==8221==ABORTING

Proposed Fix

Perhaps we should avoid using VLA altogether, and allocate fragment_table_index to the heap? It's likely that the fragment_table_index[] can be a lot bigger than 8388608 bytes, according to the squashfs specification.

This problem is present in other read_fragment_table_N functions, and in in the original squashfs-tools.

This pull request is an example implementation of the recommended fix for unsquash-4, but I don't have enough test vectors to verify it doesn't break anything. All code that uses VLA should probably be converted to use the heap instead.

VLA Usages (In squashfs-tools)

unsquashfs.c:693:14: warning: variable length array used [-Wvla]
                char buffer[c_byte];
                           ^
unsquash-2.c:35:27: warning: variable length array used [-Wvla]
                unsigned int sblock_list[blocks];
                                        ^
unsquash-2.c:48:35: warning: variable length array used [-Wvla]
        unsigned int fragment_table_index[indexes];
                                         ^
unsquash-2.c:65:38: warning: variable length array used [-Wvla]
                 unsigned int sfragment_table_index[indexes];
                                                   ^
unsquash-3.c:35:32: warning: variable length array used [-Wvla]
        long long fragment_table_index[indexes];
                                      ^
unsquash-3.c:52:34: warning: variable length array used [-Wvla]
                long long sfragment_table_index[indexes];
                                               ^
unsquash-1.c:335:26: warning: variable length array used [-Wvla]
                unsigned int suid_table[sBlk.no_uids + sBlk.no_guids];

unsquashfs.c:2099:10: warning: variable length array used [-Wvla]
        char tmp[block_size];
                ^
unsquash-4.c:374:26: warning: variable length array used [-Wvla]
        long long id_index_table[indexes];

Please let me know what you think.

Thanks.

Update unsquash-4.c
There seems to be a stack overflow in read_fragment_table_4 at via what seems to be an integer overflow. Still looking into this problem, it seems like two or three different problems combined.

The first problem overflows the bytes variable, so that the allocation is enormous.
```c
int bytes = SQUASHFS_FRAGMENT_BYTES(sBlk.s.fragments);
```

If we fix this by making the variable size_t, we run into an unrelated problem in which the stack VLA allocation of fragment_table_index can easily exceed RLIMIT_STACK.
```c
long long fragment_table_index[indexes];
```

In the case of my system, the RLIMIT_STACK is 8388608, and VLA is asking for 15728648. Plus the stack probably already has a bunch of other things. This is what I believe ultimately leads to the stack overflow.

Afterwards, the heap allocation seems to succeed, and the disastrous call to read_fs_bytes is made, which initiates transfer from the squashfs image to the stack. At this stage, a stack overflow appears to be in full effect.

```c
 res = read_fs_bytes(fd, sBlk.s.fragment_table_start,
         SQUASHFS_FRAGMENT_INDEX_BYTES(sBlk.s.fragments),
         fragment_table_index);
```
This problem is also present in other read_fragment_table_N functions, and in in the original squashfs-tools.

```
Parallel unsquashfs: Using 8 processors
ASAN:SIGSEGV
=================================================================
==8221==ERROR: AddressSanitizer: stack-overflow on address 0x7ffef3ae9608 (pc 0x000000559011 bp 0x7ffef49e9670 sp 0x7ffef3ae9610 T0)
    #0 0x559010 in read_fragment_table_4 /home/septimus/vr/squashfs-vr/squashfs-tools/unsquash-4.c:40:9
    #1 0x525073 in main /home/septimus/vr/squashfs-vr/squashfs-tools/unsquashfs.c:2763:5
    #2 0x7fb56c533a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)
    #3 0x418468 in _start (/home/septimus/vr/squashfs-vr/squashfs-tools/unsquashfs+0x418468)
SUMMARY: AddressSanitizer: stack-overflow /home/septimus/vr/squashfs-vr/squashfs-tools/unsquash-4.c:40:9 in read_fragment_table_4
==8221==ABORTING
```

Perhaps we should avoid using VLA altogether, and allocate fragment_table_index to the heap?
This pull request is an example implementation of the fix for unsquash-4, but I don't have enough test vectors to verify it will not break anything.

@ghost ghost referenced this pull request Jun 17, 2015

Closed

Update unsquash-4.c #4

@ghost ghost changed the title from Update unsquash-4.c to read_fragment_table_N: Recommended fix for stack overflow. Jun 20, 2015

@ghost ghost changed the title from read_fragment_table_N: Recommended fix for stack overflow. to read_fragment_table_n: Fix recommendation for stack overflow. Jun 20, 2015

@andreasstieger

This comment has been minimized.

Show comment
Hide comment
@andreasstieger

andreasstieger Jul 17, 2015

Is there any response from the project regarding this issue?

andreasstieger commented Jul 17, 2015

Is there any response from the project regarding this issue?

@devttys0

This comment has been minimized.

Show comment
Hide comment
@devttys0

devttys0 May 22, 2018

Owner

Since this project is just a patch file to add functionality to the original squashfs tools, this should be fixed upstream in the SquashFS project itself.

Owner

devttys0 commented May 22, 2018

Since this project is just a patch file to add functionality to the original squashfs tools, this should be fixed upstream in the SquashFS project itself.

@devttys0 devttys0 closed this May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment