Improve FuzzBuffer chunking

Now allocates a new buffer for each chunk to better detect over-reads
This commit is contained in:
Jon Siwek 2020-04-27 16:25:56 -07:00
parent 8e6539b55f
commit 2922bf71b6
3 changed files with 37 additions and 28 deletions

View file

@ -17,31 +17,28 @@ bool zeek::FuzzBuffer::Valid() const
return true; return true;
} }
int zeek::FuzzBuffer::Next(const unsigned char** chunk, size_t* len, bool* is_orig) std::optional<zeek::FuzzBuffer::Chunk> zeek::FuzzBuffer::Next()
{ {
if ( begin == end ) if ( begin == end )
{ return {};
*chunk = nullptr;
*len = 0;
return 0;
}
auto pos = (const unsigned char*)memmem(begin, end - begin, auto pos = (const unsigned char*)memmem(begin, end - begin,
PKT_MAGIC, PKT_MAGIC_LEN); PKT_MAGIC, PKT_MAGIC_LEN);
if ( ! pos ) if ( ! pos )
return -1; return {};
begin += PKT_MAGIC_LEN; begin += PKT_MAGIC_LEN;
auto remaining = end - begin; auto remaining = end - begin;
if ( remaining < 2 ) if ( remaining < 2 )
return -2; return {};
*is_orig = begin[0] & 0x01; Chunk rval;
rval.is_orig = begin[0] & 0x01;
begin += 1; begin += 1;
*chunk = begin; auto chunk_begin = begin;
auto next = (const unsigned char*)memmem(begin, end - begin, auto next = (const unsigned char*)memmem(begin, end - begin,
PKT_MAGIC, PKT_MAGIC_LEN); PKT_MAGIC, PKT_MAGIC_LEN);
@ -51,6 +48,19 @@ int zeek::FuzzBuffer::Next(const unsigned char** chunk, size_t* len, bool* is_or
else else
begin = end; begin = end;
*len = begin - *chunk; rval.size = begin - chunk_begin;
return 0;
if ( rval.size )
{
// The point of allocating a new buffer here is to better detect
// analyzers that may over-read within a chunk -- ASan wouldn't
// complain if that happens to land within the full input buffer
// provided by the fuzzing engine, but will if we allocate a new buffer
// for each chunk.
rval.data = std::make_unique<unsigned char[]>(rval.size);
memcpy(rval.data.get(), chunk_begin, rval.size);
return rval;
}
return {};
} }

View file

@ -1,6 +1,8 @@
#pragma once #pragma once
#include <cstddef> #include <cstddef>
#include <memory>
#include <optional>
namespace zeek { namespace zeek {
@ -15,6 +17,13 @@ namespace zeek {
* send along to an analyzers Deliver method. * send along to an analyzers Deliver method.
*/ */
class FuzzBuffer { class FuzzBuffer {
public:
struct Chunk {
std::unique_ptr<unsigned char[]> data;
size_t size;
bool is_orig;
};
static constexpr int PKT_MAGIC_LEN = 4; static constexpr int PKT_MAGIC_LEN = 4;
static constexpr unsigned char PKT_MAGIC[PKT_MAGIC_LEN + 1] = "\1PKT"; static constexpr unsigned char PKT_MAGIC[PKT_MAGIC_LEN + 1] = "\1PKT";
@ -35,13 +44,9 @@ class FuzzBuffer {
bool Valid() const; bool Valid() const;
/** /**
* Finds the next chunk of data to pass along to an analyzer. * @return the next chunk to deliver, if one could be extracted
* @param chunk the data chunk to return
* @param len the size of the chunk returned in *chunk*
* @param is_orig whether returned chunk is from originator or responder
* @return a value less than zero if a chunk could not be extracted
*/ */
int Next(const unsigned char** chunk, size_t* len, bool* is_orig); std::optional<Chunk> Next();
private: private:

View file

@ -53,28 +53,22 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
auto conn = add_connection(); auto conn = add_connection();
auto a = add_analyzer(conn); auto a = add_analyzer(conn);
const unsigned char* chunk;
size_t chunk_size;
bool is_orig;
for ( ; ; ) for ( ; ; )
{ {
auto err = fb.Next(&chunk, &chunk_size, &is_orig); auto chunk = fb.Next();
if ( err ) if ( ! chunk )
break;
if ( chunk_size == 0 )
break; break;
try try
{ {
a->DeliverStream(chunk_size, chunk, is_orig); a->DeliverStream(chunk->size, chunk->data.get(), chunk->is_orig);
} }
catch ( const binpac::Exception& e ) catch ( const binpac::Exception& e )
{ {
} }
chunk = {};
mgr.Drain(); mgr.Drain();
} }