From dab21bc6243a382e1881fe05e561686bf6c474a9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 6 Mar 2023 12:33:10 -0500 Subject: [PATCH] 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 = 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. --- crates/tor-bytes/src/reader.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/crates/tor-bytes/src/reader.rs b/crates/tor-bytes/src/reader.rs index cdd7dcc7f..8234631b9 100644 --- a/crates/tor-bytes/src/reader.rs +++ b/crates/tor-bytes/src/reader.rs @@ -262,7 +262,20 @@ impl<'a> Reader<'a> { /// /// On failure, consumes nothing. pub fn extract_n(&mut self, n: usize) -> Result> { - 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) {