From a4950aca66ba2ce8fb62119536b3179c4f44cb7c Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Wed, 8 Apr 2020 17:51:36 +0200 Subject: [PATCH] Safer checks in type serializer --- libretroshare/src/serialiser/rsserial.cc | 21 ++---------- libretroshare/src/serialiser/rsserial.h | 24 ++++++++------ .../src/serialiser/rstypeserializer.h | 33 ++++++++++++++----- 3 files changed, 41 insertions(+), 37 deletions(-) diff --git a/libretroshare/src/serialiser/rsserial.cc b/libretroshare/src/serialiser/rsserial.cc index dda560867..52123abf1 100644 --- a/libretroshare/src/serialiser/rsserial.cc +++ b/libretroshare/src/serialiser/rsserial.cc @@ -125,9 +125,7 @@ RsItem::RsItem(uint8_t ver, uint8_t cls, uint8_t t, uint8_t subtype) type = (ver << 24) + (cls << 16) + (t << 8) + subtype; } -RsItem::~RsItem() -{ -} +RsItem::~RsItem() = default; void RsItem::print_string(std::string &out, uint16_t indent) { @@ -243,10 +241,7 @@ uint32_t RsSerialType::PacketId() const -RsSerialiser::RsSerialiser() -{ - return; -} +RsSerialiser::RsSerialiser() = default; RsSerialiser::~RsSerialiser() @@ -559,17 +554,7 @@ std::ostream &RsRawItem::print(std::ostream &out, uint16_t indent) return out; } - -uint32_t getRsPktMaxSize() -{ - //return 65535; /* 2^16 (old artifical low size) */ - //return 1048575; /* 2^20 -1 (Too Big! - must remove fixed static buffers first) */ - /* Remember that every pqistreamer allocates an input buffer of this size! - * So don't make it too big! - */ - return 262143; /* 2^18 -1 */ -} - +uint32_t getRsPktMaxSize() { return RsSerialiser::MAX_SERIAL_SIZE; } uint32_t getRsPktBaseSize() { diff --git a/libretroshare/src/serialiser/rsserial.h b/libretroshare/src/serialiser/rsserial.h index 3ce56a424..168db8d75 100644 --- a/libretroshare/src/serialiser/rsserial.h +++ b/libretroshare/src/serialiser/rsserial.h @@ -19,16 +19,16 @@ * along with this program. If not, see . * * * *******************************************************************************/ -#ifndef RS_BASE_SERIALISER_H -#define RS_BASE_SERIALISER_H +#pragma once -#include -#include +#include #include #include #include -#include -#include +#include +#include + +#include "util/rsdeprecate.h" /******************************************************************* * This is the Top-Level serialiser/deserialise, @@ -66,7 +66,11 @@ class RsSerialType ; class RsSerialiser { - public: +public: + /** Remember that every pqistreamer allocates an input buffer of this size! + * So don't make it too big! */ + static constexpr uint32_t MAX_SERIAL_SIZE = 262143; /* 2^18 -1 */ + RsSerialiser(); ~RsSerialiser(); bool addSerialType(RsSerialType *type); @@ -76,7 +80,7 @@ class RsSerialiser RsItem * deserialise(void *data, uint32_t *size); - private: +private: std::map serialisers; }; @@ -95,6 +99,8 @@ uint16_t getRsItemService(uint32_t type); /* size constants */ uint32_t getRsPktBaseSize(); + +RS_DEPRECATED_FOR(RsSerialiser::MAX_SERIAL_SIZE) uint32_t getRsPktMaxSize(); @@ -106,5 +112,3 @@ std::ostream &printRsItemEnd(std::ostream &o, std::string n, uint16_t i); /* defined in rstlvtypes.cc - redeclared here for ease */ std::ostream &printIndent(std::ostream &out, uint16_t indent); /* Wrapper class for data that is serialised somewhere else */ - -#endif /* RS_BASE_SERIALISER_H */ diff --git a/libretroshare/src/serialiser/rstypeserializer.h b/libretroshare/src/serialiser/rstypeserializer.h index adf885c31..d72280bb3 100644 --- a/libretroshare/src/serialiser/rstypeserializer.h +++ b/libretroshare/src/serialiser/rstypeserializer.h @@ -480,7 +480,7 @@ struct RsTypeSerializer switch(j) { - case RsGenericSerializer::SIZE_ESTIMATE: // [[falltrough]] + case RsGenericSerializer::SIZE_ESTIMATE: // [[falltrough]]; case RsGenericSerializer::SERIALIZE: { uint32_t aSize = member.size(); @@ -499,10 +499,11 @@ struct RsTypeSerializer if(!ctx.mOk) break; /* This check is not perfect but will catch most pathological cases. - * Avoid multiplying by sizeof(el_t) as it is not a good exitimation + * Avoid multiplying by sizeof(el_t) as it is not a good estimation * of the actual serialized size, depending on the elements - * structure and on the so it would raises many false positives. */ - if(elCount > ctx.mSize - ctx.mOffset) + * structure and on the so it would raises many false positives. + * Arithmetic operations on elCount are also at risk of overflow */ + if(elCount > RsSerialiser::MAX_SERIAL_SIZE) { ctx.mOk = false; RsErr() << __PRETTY_FUNCTION__ << " attempt to deserialize a " @@ -626,7 +627,7 @@ struct RsTypeSerializer { uint32_t len = static_cast(member.length()); RS_SERIAL_PROCESS(len); - if(len > ctx.mSize - ctx.mOffset) + if(len + ctx.mOffset > ctx.mSize) { RsErr() << __PRETTY_FUNCTION__ << std::errc::no_buffer_space << std::endl; @@ -641,19 +642,33 @@ struct RsTypeSerializer uint32_t len; RS_SERIAL_PROCESS(len); if(!ctx.mOk) break; - if(len > ctx.mSize - ctx.mOffset) + + if(len > RsSerialiser::MAX_SERIAL_SIZE) { ctx.mOk = false; RsErr() << __PRETTY_FUNCTION__ << " attempt to deserialize a " - << "string with apparently malformed elements count." + << "string with apparently malformed length." << " len: " << len << " ctx.mSize: " << ctx.mSize << " ctx.mOffset: " << ctx.mOffset << " " - << std::errc::argument_out_of_domain - << std::endl; + << std::errc::argument_out_of_domain << std::endl; print_stacktrace(); break; } + + if(len + ctx.mOffset > ctx.mSize) + { + ctx.mOk = false; + RsErr() << __PRETTY_FUNCTION__ << " attempt to deserialize a " + << "string with a length bigger available data." + << " len: " << len + << " ctx.mSize: " << ctx.mSize + << " ctx.mOffset: " << ctx.mOffset << " " + << std::errc::no_buffer_space << std::endl; + print_stacktrace(); + break; + } + member.resize(len); memcpy(&member[0], ctx.mData + ctx.mOffset, len); ctx.mOffset += len;