Skip to content

Check that dest is valid for decompression #3555

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

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 2 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
1 change: 1 addition & 0 deletions contrib/linux-kernel/mem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ typedef uint32_t U32;
typedef int32_t S32;
typedef uint64_t U64;
typedef int64_t S64;
typedef uintptr_t uptrval;

/*-**************************************************************
* Memory I/O API
Expand Down
Binary file added contrib/linux-kernel/test/static_test
View file
Open in desktop
Copy link
Contributor

@yoniko yoniko Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Was this file included on purpose? (same question for contrib/linux-kernel/test/test)

Copy link
Contributor Author

@daniellerozenblit daniellerozenblit Mar 16, 2023

Choose a reason for hiding this comment

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

oops, nope!

Binary file not shown.
Binary file added contrib/linux-kernel/test/test
View file
Open in desktop
Binary file not shown.
2 changes: 2 additions & 0 deletions lib/common/mem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ extern "C" {
typedef int32_t S32;
typedef uint64_t U64;
typedef int64_t S64;
typedef uintptr_t uptrval;
#else
# include <limits.h>
#if CHAR_BIT != 8
Expand All @@ -81,6 +82,7 @@ extern "C" {
* limits exist in C99, however, in such case, <stdint.h> is preferred */
typedef unsigned long long U64;
typedef signed long long S64;
typedef size_t uptrval; /* generally true, except OpenVMS-64 */
#endif


Expand Down
3 changes: 3 additions & 0 deletions lib/decompress/zstd_decompress_block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2125,6 +2125,9 @@ ZSTD_decompressBlock_internal(ZSTD_DCtx* dctx,
srcSize -= seqHSize;

RETURN_ERROR_IF(dst == NULL && nbSeq > 0, dstSize_tooSmall, "NULL not handled");
RETURN_ERROR_IF(dstCapacity == 0 && nbSeq > 0, dstSize_tooSmall, "dstCapacity == 0, but nbSeq is non-zero");
Copy link
Contributor

@terrelln terrelln Mar 20, 2023

Choose a reason for hiding this comment

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

nit: We can combine this check with the line 2127. dst may only be NULL if dstCapacity == 0.

RETURN_ERROR_IF(MEM_64bits() && (uptrval)dst + dstCapacity > (uptrval)-1 - (1 << 20), dstSize_tooSmall,
Copy link
Contributor

@terrelln terrelln Mar 20, 2023

Choose a reason for hiding this comment

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

@Cyan4973 what do you think of requiring uintptr_t here? We already do the same in LZ4.

Copy link
Contributor

@Cyan4973 Cyan4973 Mar 27, 2023

Choose a reason for hiding this comment

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

I'm generally not a fan of uintptr_t, because it's an optional type,
and we learned the hard way that this type breaks on some platforms.

What about the following alternative test ?

#define MAX_ADDRESS (const char*)(size_t)(-1)
#define SAFE_DISTANCE  (1 << 20)

if (MAX_ADDRESS - (const char*)dst < SAFE_DISTANCE) { ... }

"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