diff options
author | Eran Messeri <eranm@google.com> | 2021-07-19 17:46:11 +0100 |
---|---|---|
committer | Eran Messeri <eranm@google.com> | 2021-11-25 16:50:11 +0000 |
commit | 7f7cabaaeec3748a1cf3e58c76c39685abf0d75a (patch) | |
tree | 8c0d74c912d98b53dcfda489c980b46d96bc23ad | |
parent | 10b5d82d1ebf209d98e7d66b4c01c2e7e9bad498 (diff) | |
download | keymaster-android12-qpr3-s2-release.tar.gz |
Add missing signedness check in Keymaster bufferandroid-12.1.0_r9android-12.1.0_r8android-12.1.0_r7android-12.1.0_r22android-12.1.0_r21android-12.1.0_r20android-12.1.0_r19android-12.1.0_r11android-12.1.0_r10android12L-devandroid12-qpr3-s7-releaseandroid12-qpr3-s6-releaseandroid12-qpr3-s5-releaseandroid12-qpr3-s4-releaseandroid12-qpr3-s3-releaseandroid12-qpr3-s2-releaseandroid12-qpr3-s1-releaseandroid12-qpr3-release
Add a check in the Serializable Buffer implementation of Keymaster for
the signedness of the input parameter to advance_read and advance_write.
Both methods take a distance of type int, and add it to the buffer
position regardless of whether it's positive or negative.
This leads to violation of buffer state invariants (specifically
read_position_) and (ultimately) to reading from an invalid
memory region.
In this change:
* advance_read is removed as it's not used.
* advance_write is moved out of the header file.
* Guards against negative distance values and wrapping are added.
* A method for validating buffer state is added and used in reserve()
Ignore-AOSP-First: Security fix
Bug: 173567719
Test: Run libkeymaster_fuzz_buffer on clusterfuzz-testcase-minimized-libkeymaster_fuzz_buffer-5372592199434240
Merged-In: I15330a2f23c3461e23daad450af33e3f92e6730c
Change-Id: I15330a2f23c3461e23daad450af33e3f92e6730c
(cherry picked from commit 48edbcdb981c980b27f4826563b4ca46754df885)
-rw-r--r-- | android_keymaster/serializable.cpp | 23 | ||||
-rw-r--r-- | include/keymaster/serializable.h | 17 | ||||
-rw-r--r-- | tests/fuzzers/buffer_fuzz.cpp | 3 |
3 files changed, 25 insertions, 18 deletions
diff --git a/android_keymaster/serializable.cpp b/android_keymaster/serializable.cpp index b1f1e31..fe0d742 100644 --- a/android_keymaster/serializable.cpp +++ b/android_keymaster/serializable.cpp @@ -74,6 +74,10 @@ bool copy_size_and_data_from_buf(const uint8_t** buf_ptr, const uint8_t* end, si bool Buffer::reserve(size_t size) { if (available_write() < size) { + if (!valid_buffer_state()) { + return false; + } + size_t new_size = buffer_size_ + size - available_write(); uint8_t* new_buffer = new (std::nothrow) uint8_t[new_size]; if (!new_buffer) return false; @@ -121,6 +125,10 @@ size_t Buffer::available_read() const { return write_position_ - read_position_; } +bool Buffer::valid_buffer_state() const { + return (buffer_size_ >= write_position_) && (write_position_ >= read_position_); +} + bool Buffer::write(const uint8_t* src, size_t write_length) { if (available_write() < write_length) return false; memcpy(buffer_.get() + write_position_, src, write_length); @@ -135,6 +143,21 @@ bool Buffer::read(uint8_t* dest, size_t read_length) { return true; } +bool Buffer::advance_write(int distance) { + if (distance < 0) { + return false; + } + + const size_t validated_distance = static_cast<size_t>(distance); + const size_t new_write_position = write_position_ + validated_distance; + + if (new_write_position <= buffer_size_ && new_write_position >= write_position_) { + write_position_ = new_write_position; + return true; + } + return false; +} + size_t Buffer::SerializedSize() const { return sizeof(uint32_t) + available_read(); } diff --git a/include/keymaster/serializable.h b/include/keymaster/serializable.h index fdc97f1..4532485 100644 --- a/include/keymaster/serializable.h +++ b/include/keymaster/serializable.h @@ -238,26 +238,13 @@ class Buffer : public Serializable { size_t available_write() const; size_t available_read() const; size_t buffer_size() const { return buffer_size_; } + bool valid_buffer_state() const; bool write(const uint8_t* src, size_t write_length); bool read(uint8_t* dest, size_t read_length); const uint8_t* peek_read() const { return buffer_.get() + read_position_; } - bool advance_read(int distance) { - if (static_cast<size_t>(read_position_ + distance) <= write_position_) { - read_position_ += distance; - return true; - } - return false; - } uint8_t* peek_write() { return buffer_.get() + write_position_; } - bool advance_write(int distance) { - if (static_cast<size_t>(write_position_ + distance) <= buffer_size_) { - write_position_ += distance; - return true; - } - return false; - } - + bool advance_write(int distance); size_t SerializedSize() const; uint8_t* Serialize(uint8_t* buf, const uint8_t* end) const; bool Deserialize(const uint8_t** buf_ptr, const uint8_t* end); diff --git a/tests/fuzzers/buffer_fuzz.cpp b/tests/fuzzers/buffer_fuzz.cpp index 0b02b3f..e0928cd 100644 --- a/tests/fuzzers/buffer_fuzz.cpp +++ b/tests/fuzzers/buffer_fuzz.cpp @@ -37,9 +37,6 @@ std::vector<std::function<void(keymaster::Buffer*, FuzzedDataProvider*)>> operat buf->reserve(fdp->ConsumeIntegralInRange<int>(kMinBufferSize, kMaxBufferSize)); }, [](keymaster::Buffer* buf, FuzzedDataProvider* fdp) -> void { - buf->advance_read(fdp->ConsumeIntegral<int>()); - }, - [](keymaster::Buffer* buf, FuzzedDataProvider* fdp) -> void { buf->advance_write(fdp->ConsumeIntegral<int>()); }, [](keymaster::Buffer* buf, FuzzedDataProvider* fdp) -> void { |