Skip to content

[Rust] Harden FlexBuffers reader against malformed input - #9245

Open
avalon1k wants to merge 1 commit into
google:masterfrom
avalon1k:fix/rust-flexbuffers-checked-reads
Open

avalon1k wants to merge 1 commit into
google:masterfrom
avalon1k:fix/rust-flexbuffers-checked-reads

Conversation

@avalon1k

Copy link
Copy Markdown

The Problem

Many of the public API Result<T, Error> return values are fragile. Malformed input can bypass the Error contract and panic, wrap buffer positions, or decode truncated data incorrectly.

reader/mod.rs

  • :307 - arithmetic overflow.
  • :326 - unchecked indexing -> panic.
  • :339 - unterminated key accepted.
  • :356 - arithmetic overflow.
  • :369 - arithmetic overflow.
  • :390 - mismatched map key/value lengths accepted (was a TODO).
  • :604, :606 - unchecked indexing -> panic.
  • :611, :616, :621 - truncated integer read returns 0.
  • :621 - W64 truncation on 32-bit.

reader/vector.rs

  • :55, :58 - arithmetic overflow.
  • :73 - arithmetic overflow.

reader/map.rs

  • :83 - unterminated map key accepted.
  • :91, :92 - arithmetic overflow.
  • :94 - reports malformed offsets as KeyNotFound instead of FlexbufferOutOfBounds.
  • :118, :119 - arithmetic overflow.

The Change

Three approaches were considered:

  1. Add checks directly at every affected site. | Not chosen. Too much code duplication, messy.
  2. Introduce checked helper functions for affected call sites. | Not chosen. Requires disciplined usage. Does not prevent future bugs of same classes.
  3. Encapsulate the buffer position and reads behind a checked cursor. | Chosen. Fixes current bugs and prevents the same bug classes from appearing in future reader code.

No breaking changes. Public API and default behavior are unchanged.

Testing

Added 15 new integration tests that cover all relevant bugs listed above.

NOTE: 32-bit specific tests were excluded as active Rust CI targets are 64-bit.

Ran cargo test on both branches, 0/15 PASSED on old code, 15/15 PASSED on new code.
All existing tests still pass.

* encapsulate buffer reads and address arithmetic behind a checked cursor
* route Reader, VectorReader, and MapReader through the cursor
* reject invalid ranges, overflow, unterminated keys, and mismatched map key/value lengths
* add 15 malformed-input tests
@avalon1k

Copy link
Copy Markdown
Author

Related: #8924, #8980, and #9105 (c++).

This PR was developed independently and takes a broader, future-proof approach, and addresses additional failure cases, at the cost of a substantially larger diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant