From f63ccce09e47fbfbc0205cd24ecbaa9bb144fc09 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 26 Oct 2020 15:58:40 -0400 Subject: [PATCH] Add a buffer reader helper class that can be used to read buffers safely. The idea is to not have people doing ad-hoc, and often incorrect when they forget to update lengths, length checks. I did measure the codesize with the Read methods out-of-line, and it's larger than with the inline methods as far as I can tell. It might be worth re-measuring once we have more callers to see what things look like then. --- src/lib/support/BUILD.gn | 1 + src/lib/support/BufferReader.h | 150 ++++++++++++++++++++++++++++ src/transport/raw/MessageHeader.cpp | 72 +++++++------ 3 files changed, 194 insertions(+), 29 deletions(-) create mode 100644 src/lib/support/BufferReader.h diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index 1c8402e4082cdf..3e23c890569fcd 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -49,6 +49,7 @@ static_library("support") { "Base64.h", "BitFlags.h", "BufBound.h", + "BufferReader.h", "CHIPArgParser.cpp", "CHIPCounter.cpp", "CHIPCounter.h", diff --git a/src/lib/support/BufferReader.h b/src/lib/support/BufferReader.h new file mode 100644 index 00000000000000..e1cea426e49af3 --- /dev/null +++ b/src/lib/support/BufferReader.h @@ -0,0 +1,150 @@ +/* + * + * Copyright (c) 2020 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * Utility classes for safely reading from size-limited buffers. + */ + +#pragma once + +#include +#include +#include +#include +#include + +namespace chip { +namespace Encoding { +namespace LittleEndian { + +/** + * @class Reader + * + * Simple reader for reading little-endian things out of buffers. + */ +class Reader +{ +public: + /** + * Create a data model reader from a given buffer and length. + * + * @param buffer The octet buffer to read from. The caller must ensure + * (most simply by allocating the reader on the stack) that + * the buffer outlives the reader. The buffer is allowed to + * be null if buf_len is 0. + * @param buf_len The number of octets in the buffer. + */ + Reader(const uint8_t * buffer, uint16_t buf_len) : mBufStart(buffer), mReadPtr(buffer), mAvailable(buf_len) {} + + /** + * Number of octets we have read so far. This might be able to go away once + * we do less switching back and forth between DataModelReader and raw + * buffers. + */ + uint16_t OctetsRead() const { return static_cast(mReadPtr - mBufStart); } + + /** + * Read a single 8-bit unsigned integer. + * + * @param [out] dest Where the 8-bit integer goes. + * + * @return Whether the read succeeded. The read can fail if there are not + * enough octets available. + */ + CHECK_RETURN_VALUE + CHIP_ERROR Read8(uint8_t * dest) { return Read(dest); } + + /** + * Read a single 16-bit unsigned integer. + * + * @param [out] dest Where the 16-bit integer goes. + * + * @return Whether the read succeeded. The read can fail if there are not + * enough octets available. + */ + CHECK_RETURN_VALUE + CHIP_ERROR Read16(uint16_t * dest) { return Read(dest); } + + /** + * Read a single 32-bit unsigned integer. + * + * @param [out] dest Where the 32-bit integer goes. + * + * @return Whether the read succeeded. The read can fail if there are not + * enough octets available. + */ + CHECK_RETURN_VALUE + CHIP_ERROR Read32(uint32_t * dest) { return Read(dest); } + + /** + * Read a single 64-bit unsigned integer. + * + * @param [out] dest Where the 64-bit integer goes. + * + * @return Whether the read succeeded. The read can fail if there are not + * enough octets available. + */ + CHECK_RETURN_VALUE + CHIP_ERROR Read64(uint64_t * dest) { return Read(dest); } + +protected: + template + CHIP_ERROR Read(T * retval) + { + static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing"); + + static constexpr size_t data_size = sizeof(T); + + if (mAvailable < data_size) + { + return CHIP_ERROR_BUFFER_TOO_SMALL; + } + + Read(mReadPtr, retval); + mAvailable = static_cast(mAvailable - data_size); + return CHIP_NO_ERROR; + } + +private: + // These helper methods return void and put the value being read into an + // outparam because that allows us to easily overload on the type of the + // thing being read. + void Read(const uint8_t *& p, uint8_t * dest) { *dest = Encoding::Read8(p); } + void Read(const uint8_t *& p, uint16_t * dest) { *dest = Encoding::LittleEndian::Read16(p); } + void Read(const uint8_t *& p, uint32_t * dest) { *dest = Encoding::LittleEndian::Read32(p); } + void Read(const uint8_t *& p, uint64_t * dest) { *dest = Encoding::LittleEndian::Read64(p); } + + /** + * Our buffer start. + */ + const uint8_t * const mBufStart; + + /** + * Our current read point. + */ + const uint8_t * mReadPtr; + + /** + * The number of octets we can still read starting at mReadPtr. + */ + uint16_t mAvailable; +}; + +} // namespace LittleEndian +} // namespace Encoding +} // namespace chip diff --git a/src/transport/raw/MessageHeader.cpp b/src/transport/raw/MessageHeader.cpp index 8e5d1c47ba394d..64b5f034f0862a 100644 --- a/src/transport/raw/MessageHeader.cpp +++ b/src/transport/raw/MessageHeader.cpp @@ -29,8 +29,8 @@ #include -#include #include +#include #include /********************************************** @@ -137,27 +137,29 @@ uint16_t MessageAuthenticationCode::TagLenForEncryptionType(Header::EncryptionTy CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint16_t * decode_len) { - CHIP_ERROR err = CHIP_NO_ERROR; - const uint8_t * p = data; + CHIP_ERROR err = CHIP_NO_ERROR; + LittleEndian::Reader reader(data, size); int version; - - VerifyOrExit(size >= kFixedUnencryptedHeaderSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT); + uint16_t octets_read; uint16_t header; - header = LittleEndian::Read16(p); + err = reader.Read16(&header); + SuccessOrExit(err); version = ((header & kVersionMask) >> kVersionShift); VerifyOrExit(version == kHeaderVersion, err = CHIP_ERROR_VERSION_MISMATCH); mEncryptionType = static_cast((header & kEncryptionTypeMask) >> kEncryptionTypeShift); mFlags.SetRaw(header & Header::kFlagsMask); - mMessageId = LittleEndian::Read32(p); + err = reader.Read32(&mMessageId); + SuccessOrExit(err); if (mFlags.Has(Header::FlagValues::kSourceNodeIdPresent)) { - static_assert(kNodeIdSizeBytes == 8, "Read64 might read more bytes than we have in the buffer"); - VerifyOrExit(size >= static_cast(p - data) + kNodeIdSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT); - mSourceNodeId.SetValue(LittleEndian::Read64(p)); + uint64_t sourceNodeId; + err = reader.Read64(&sourceNodeId); + SuccessOrExit(err); + mSourceNodeId.SetValue(sourceNodeId); } else { @@ -166,20 +168,25 @@ CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint1 if (mFlags.Has(Header::FlagValues::kDestinationNodeIdPresent)) { - VerifyOrExit(size >= static_cast(p - data) + kNodeIdSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT); - mDestinationNodeId.SetValue(LittleEndian::Read64(p)); + uint64_t destinationNodeId; + err = reader.Read64(&destinationNodeId); + SuccessOrExit(err); + mDestinationNodeId.SetValue(destinationNodeId); } else { mDestinationNodeId.ClearValue(); } - VerifyOrExit(size >= static_cast(p - data) + sizeof(uint16_t) * 2, err = CHIP_ERROR_INVALID_ARGUMENT); - mEncryptionKeyID = LittleEndian::Read16(p); - mPayloadLength = LittleEndian::Read16(p); + err = reader.Read16(&mEncryptionKeyID); + SuccessOrExit(err); - VerifyOrExit(p - data == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL); - *decode_len = static_cast(p - data); + err = reader.Read16(&mPayloadLength); + SuccessOrExit(err); + + octets_read = reader.OctetsRead(); + VerifyOrExit(octets_read == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL); + *decode_len = octets_read; exit: @@ -188,20 +195,26 @@ CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint1 CHIP_ERROR PayloadHeader::Decode(Header::Flags flags, const uint8_t * const data, uint16_t size, uint16_t * decode_len) { - CHIP_ERROR err = CHIP_NO_ERROR; - const uint8_t * p = data; + CHIP_ERROR err = CHIP_NO_ERROR; + LittleEndian::Reader reader(data, size); uint8_t header; + uint16_t octets_read; + + err = reader.Read8(&header); + SuccessOrExit(err); - VerifyOrExit(size >= kEncryptedHeaderSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT); + err = reader.Read8(&mMessageType); + SuccessOrExit(err); - header = Read8(p); - mMessageType = Read8(p); - mExchangeID = LittleEndian::Read16(p); + err = reader.Read16(&mExchangeID); + SuccessOrExit(err); if (flags.Has(Header::FlagValues::kVendorIdPresent)) { - VerifyOrExit(size >= static_cast(p - data) + kVendorIdSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT); - mVendorId.SetValue(LittleEndian::Read16(p)); + uint16_t vendor_id; + err = reader.Read16(&vendor_id); + SuccessOrExit(err); + mVendorId.SetValue(vendor_id); } else { @@ -210,11 +223,12 @@ CHIP_ERROR PayloadHeader::Decode(Header::Flags flags, const uint8_t * const data mExchangeFlags.SetRaw(header); - VerifyOrExit(size >= static_cast(p - data) + sizeof(uint16_t), err = CHIP_ERROR_INVALID_ARGUMENT); - mProtocolID = LittleEndian::Read16(p); + err = reader.Read16(&mProtocolID); + SuccessOrExit(err); - VerifyOrExit(p - data == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL); - *decode_len = static_cast(p - data); + octets_read = reader.OctetsRead(); + VerifyOrExit(octets_read == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL); + *decode_len = octets_read; exit: