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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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):

 Replying to [comment:5 nickm]:
 > The branch `bug18162_024` in my public repository has a proposed fix for
 this bug, along with a small family of related bugs.   You can see the
 patch here:
 https://gitweb.torproject.org/nickm/tor.git/commit/?h=bug18162_024&id=bca7083e8285e8e6a4377076a7e432417eafc6d2
 >
 > Review would be much appreciated.
 >
 > From an abundance of caution, I've written the fix to apply to 0.2.4 and
 later, though I doubt that's necessary.

 Code review:

 These fixes look like they work and resolve this issue.

 In multiple functions:

 The calculation `((size_t) sl->num_used)+1` never overflows, because
 sl->num_used is at most MAX_CAPACITY, and MAX_CAPACITY is always less than
 SIZE_MAX.

 In smartlist_add_all():

 In fact, MAX_CAPACITY is strictly less than or equal to SIZE_MAX/2, as
 long as `sizeof(void*) >= 2`. So the addition will never overflow on any
 platform tor builds on, either (so the check is redundant):
 {{{
   size_t new_size = (size_t)s1->num_used + (size_t)s2->num_used;
   tor_assert(new_size >= (size_t) s1->num_used); /* check for overflow. */
 }}}

 `s1->list + s1->num_used` also won't overflow, because tor_malloc() and
 tor_realloc() must ensure that `sl->list` is allocated low enough in
 memory to contain `s1->capacity`. This argument applies to `s1` before and
 after the smartlist_ensure_capacity() call, and to `s2`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18162#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs