[tor-bugs] #20081 [Core Tor/Tor]: potential memory corruption in or/buffers.c (not exploitable)

[Tor Bug Tracker & Wiki]

#20081: potential memory corruption in or/buffers.c  (not exploitable)
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.2.9.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  029-proposed, tor-bug-bounty,        |  Actual Points:
  review-group-9                                 |
Parent ID:                                       |         Points:  0.3
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by teor):

 I am happier with the second patch, but somewhat counter-intuitively, it
 limits the returned `sz` to `MAX_CHUNK_ALLOC << 1`, not `MAX_CHUNK_ALLOC`.

 `preferred_chunk_size(MAX_CHUNK_ALLOC)`:
 * passes both initial checks, and
 * passes `while (CHUNK_SIZE_WITH_ALLOC(sz) < target)` even when `sz` is
 `MAX_CHUNK_ALLOC`, because `CHUNK_SIZE_WITH_ALLOC(sz)` subtracts the chunk
 header offset.

 `preferred_chunk_size(CHUNK_SIZE_WITH_ALLOC(MAX_CHUNK_ALLOC))`:
 * actually produces a result of `MAX_CHUNK_ALLOC`, and is the highest
 value that does so.

 If we want to keep this behaviour, we should document it, but I'd prefer
 we fix it so that `MAX_CHUNK_ALLOC` really is the maximum allocated chunk
 size (or, perhaps, to avoid changing behaviour, make `MAX_CHUNK_ALLOC`
 equal to `65536 << 1`?). But either way, this is not a security issue.

 The first patch is not ideal: it would let the overflow happen, then
 abort, which is not undefined behaviour for unsigned types, but it's not
 best practice. (And it might cause various static analysis tools to
 complain, and also likely crash on `-fsanitize=unsigned-integer`, which is
 somewhat immaterial given we crash straight after anyway...)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20081#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online