tor-bytes: defend against misuse of extract_n().

Previously, if somebody wrote this code, an attacker could easily
use it to cause an OOM panic:

```
let n = r.take_u64();
let items: Vec<Foo> = r.extract_n(n as usize)?;
```

The first line of defense here is not to write protocols like that:
we don't actually _have_ any 32-bit counters in our protocol
AFAICT.

The second line of defense is to pre-check `n` for reasonableness
before calling `extract_n`.

Here we add a third line of defense: whereas previously we would do
`Vec::with_capacity(n)` in `extract_n`, we now allocate an initial
capacity of `min(n, r.remaining())`.  This ensures that the size of
the allocation can't exceed the remaining length of the message,
which (for our cell types at least) should prevent it from
overflowing or running OOM.
This commit is contained in:
Nick Mathewson 2023-03-06 12:33:10 -05:00
parent e60ab8087d
commit dab21bc624
1 changed files with 14 additions and 1 deletions

View File

@ -262,7 +262,20 @@ impl<'a> Reader<'a> {
///
/// On failure, consumes nothing.
pub fn extract_n<E: Readable>(&mut self, n: usize) -> Result<Vec<E>> {
let mut result = Vec::with_capacity(n);
// This `min` will help us defend against a pathological case where an
// attacker tells us that there are BIGNUM elements forthcoming, and our
// attempt to allocate `Vec::with_capacity(BIGNUM)` makes us panic.
//
// The `min` can be incorrect if E is somehow encodable in zero bytes
// (!?), but that will only cause our initial allocation to be too
// small.
//
// In practice, callers should always check that `n` is reasonable
// before calling this function, and protocol designers should not
// provide e.g. 32-bit counters for object types of which we should
// never allocate u32::MAX.
let n_alloc = std::cmp::min(n, self.remaining());
let mut result = Vec::with_capacity(n_alloc);
let off_orig = self.off;
for _ in 0..n {
match E::take_from(self) {