Skip to content
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.

Sign up for GitHub

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

Check that dest is valid for decompression #3555

Merged
merged 6 commits into from
Apr 1, 2023
Merged

Check that dest is valid for decompression #3555

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a871a5
add check for valid dest buffer and fuzz on random dest ptr when mall…
daniellerozenblit Mar 15, 2023
1af377b
add uptrval to linux-kernel
daniellerozenblit Mar 15, 2023
6057309
Merge branch 'facebook:dev' into fix-decompress-ub-for-dstCapacity-0
daniellerozenblit Mar 15, 2023
4b1771a
remove bin files
daniellerozenblit Mar 16, 2023
62dd65f
get rid of uptrval
daniellerozenblit Mar 29, 2023
7ca70ef
restrict max pointer value check to platforms where sizeof(size_t) ==…
daniellerozenblit Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/decompress/zstd_decompress_block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2124,7 +2124,9 @@ ZSTD_decompressBlock_internal(ZSTD_DCtx* dctx,
ip += seqHSize;
srcSize -= seqHSize;

RETURN_ERROR_IF(dst == NULL && nbSeq > 0, dstSize_tooSmall, "NULL not handled");
RETURN_ERROR_IF((dst == NULL || dstCapacity == 0) && nbSeq > 0, dstSize_tooSmall, "NULL not handled");
RETURN_ERROR_IF(MEM_64bits() && (size_t)(-1) - (size_t)dst < (size_t)(1 << 20), dstSize_tooSmall,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take it,

but note that this formulation essentially replaces uintptr_t by size_t,
which implies that sizeof(size_t) == sizeof(void*).
This is often true, for "common" platforms, but not always.
See this comment for an example.

In this specific example, where sizeof(void*)==16 while sizeof(size_t)==4,
the proposed test will trigger for any address close enough to a multiple of 4 GB.

To be fair, this is a hard issue to fix "properly" (i.e. without UB), so that's why I accept this fix proposal.
Evaluating the difference between 2 addresses which are not part of the same memory segment is UB to begin with (even if it works fine in a single-plane memory addressing architecture). That makes it hard to check the distance of an address from the end of its address space.

For the same reason, a dangling pointer that randomly points anywhere in memory is also UB, even if there is a second variable that states size == 0. So we are trying to fix at runtime a user-side UB. No wonder it's difficult to fix without triggering UB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure of the fully correct way to solve this.

As you said, checking the distance between these two addresses is UB regardless.

I guess the major concern with my solution is that on some unusual platforms where sizeof(size_t) == 8 but sizeof(void*) != 8 we may produce a lot of false positives. Would it be better to not check at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the major concern with my solution is that on some unusual platforms where sizeof(size_t) == 8 but sizeof(void*) != 8 we may produce a lot of false positives. Would it be better to not check at all?

That's indeed the question.
It seems that your check dstCapacity==0 should catch the issues described in #3507, and possibly #3506.
In which case, maybe that's enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, the first check catches all of the cases in #3507 and #3506.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe the first check is enough to fix the situation.

The second test is fine as long as sizeof(size_t) == sizeof(void*),
maybe that condition could be checked before triggering the second test.

"invalid dst");

/* If we could potentially have long offsets, or we might want to use the prefetch decoder,
* compute information about the share of long offsets, and the maximum nbAdditionalBits.
Expand Down
6 changes: 5 additions & 1 deletion tests/fuzz/block_decompress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* decompression function to ensure the decompressor never crashes.
*/

#include "fuzz_data_producer.h"
#define ZSTD_STATIC_LINKING_ONLY

#include <stddef.h>
Expand All @@ -28,11 +29,12 @@ static size_t bufSize = 0;
int LLVMFuzzerTestOneInput(const uint8_t *src, size_t size)
{
size_t const neededBufSize = ZSTD_BLOCKSIZE_MAX;
FUZZ_dataProducer_t *producer = FUZZ_dataProducer_create(src, size);

/* Allocate all buffers and contexts if not already allocated */
if (neededBufSize > bufSize) {
free(rBuf);
rBuf = FUZZ_malloc(neededBufSize);
rBuf = FUZZ_malloc_rand(neededBufSize, producer);
bufSize = neededBufSize;
}
if (!dctx) {
Expand All @@ -42,6 +44,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *src, size_t size)
ZSTD_decompressBegin(dctx);
ZSTD_decompressBlock(dctx, rBuf, neededBufSize, src, size);

FUZZ_dataProducer_free(producer);

#ifndef STATEFUL_FUZZING
ZSTD_freeDCtx(dctx); dctx = NULL;
#endif
Expand Down
1 change: 1 addition & 0 deletions tests/fuzz/fuzz_data_producer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* You may select, at your option, one of the above-listed licenses.
*/

#include "fuzz_helpers.h"
#include "fuzz_data_producer.h"

struct FUZZ_dataProducer_s{
Expand Down
1 change: 0 additions & 1 deletion tests/fuzz/fuzz_data_producer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include <stdio.h>
#include <stdlib.h>

#include "fuzz_helpers.h"

/* Struct used for maintaining the state of the data */
typedef struct FUZZ_dataProducer_s FUZZ_dataProducer_t;
Expand Down
16 changes: 16 additions & 0 deletions tests/fuzz/fuzz_helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,22 @@ void* FUZZ_malloc(size_t size)
return NULL;
}

void* FUZZ_malloc_rand(size_t size, FUZZ_dataProducer_t *producer)
{
if (size > 0) {
void* const mem = malloc(size);
FUZZ_ASSERT(mem);
return mem;
} else {
uintptr_t ptr = 0;
/* Add +- 1M 50% of the time */
if (FUZZ_dataProducer_uint32Range(producer, 0, 1))
FUZZ_dataProducer_int32Range(producer, -1000000, 1000000);
return (void*)ptr;
}

}

int FUZZ_memcmp(void const* lhs, void const* rhs, size_t size)
{
if (size == 0) {
Expand Down
7 changes: 7 additions & 0 deletions tests/fuzz/fuzz_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "fuzz.h"
#include "xxhash.h"
#include "zstd.h"
#include "fuzz_data_producer.h"
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
Expand Down Expand Up @@ -62,6 +63,12 @@ extern "C" {
*/
void* FUZZ_malloc(size_t size);

/**
* malloc except returns random pointer for zero sized data and FUZZ_ASSERT
* that malloc doesn't fail.
*/
void* FUZZ_malloc_rand(size_t size, FUZZ_dataProducer_t *producer);

/**
* memcmp but accepts NULL.
*/
Expand Down