Add missing signedness check in Keymaster buffer
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)
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 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 @@
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 @@
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 @@
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 @@
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 {