[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #21278 [Core Tor/Tor]: Avoid signed integer underflow when comparing versions (Fix TROVE-2017-001) (was: Fix TROVE-2017-001)



#21278: Avoid signed integer underflow when comparing versions (Fix TROVE-2017-001)
--------------------------+------------------------------------
 Reporter:  nickm         |          Owner:  nickm
     Type:  defect        |         Status:  needs_review
 Priority:  Medium        |      Milestone:  Tor: 0.3.0.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  029-backport  |  Actual Points:
Parent ID:                |         Points:
 Reviewer:                |        Sponsor:
--------------------------+------------------------------------
Changes (by nickm):

 * status:  assigned => needs_review


Comment:

 The problem here is that nothing in our spec unambiguously prevents the
 components of versions being negative, and so the `if ((i = (a-b))) return
 i;` pattern we use in `tor_version_compare()` potentially underflows.

 This is bad when we may have -ftrapv or ubsan enabled: both of those turn
 signed underflow into a crash.  (And it's still undefined behavior in any
 case, which we should really try to prevent.)

 My branch `bug21278_024_v2` tries to fix this, with two approaches:
    * `tor_version_compare()` now uses unsigned arithmetic to produce the
 same results while avoiding undefined behavior.  This should mean -- if I
 coded it right -- that we don't have any visible behavior differences form
 before (except "not crashing").
    * `dirserv_get_status_impl()` now rejects incoming descriptors with
 negative versions, while leaving voting unchanged.  Changes to this
 function operate at a single authority, and don't require a change in the
 consensus method number.

 ---

 Additionally, I found two more cases where we use the `if ((i = (a-b)))
 return i;` pattern to implement a comparison function.  I believe that
 they are both safe, but somebody should look them over.  The fixes for
 those are in my `bug21278_024_v2_extra` branch, on top of my
 `bug21278_024_v2` branch.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21278#comment:3>
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