[tor-bugs] #18162 [Tor]: Potential heap corruption in smartlist_add(), smartlist_insert()

[Tor Bug Tracker & Wiki]

#18162: Potential heap corruption in smartlist_add(), smartlist_insert()
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  nickm
     Type:  defect                               |         Status:
 Priority:  High                                 |  needs_review
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  security 025-backport 026-backport   |        Version:
  027-backport 024-backport                      |     Resolution:
Parent ID:                                       |  Actual Points:
  Sponsor:                                       |         Points:
-------------------------------------------------+-------------------------

Comment (by teor):

 I agree that it's much harder to program securely using int than size_t.
 For a new API, I'd use size_t.

 But it is possible to check ints before you use them: we need to make sure
 the int is in range. And we have to check for underflow and overflow
 before they happen, as they are undefined behaviour.

 The risks of converting all tor code over to size_t are that we introduce
 bugs while doing it,  that it becomes a hell of merge conflicts, or that
 we don't find every instance.

 It's worth noting that every smartlist() function that takes an int
 asserts that it's greater than 0, and less than the size of the smartlist.

 And internally, smartlist_ensure_capacity() does similar overflow checks.
 We could add an overflow check to smartlist_ensure_capacity(), but I don't
 think it can actually overflow.

 Neither can num_used overflow, because it's bound by capacity. It can't
 underflow, because we check it's greater than 0 before decrementing it.

 That said, we should build with -ftrapv (the conclusion we reached in
 #17983, despite the bug summary), and then if we miss any checks, it won't
 be a security issue, Tor will just crash.

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