Opened 10 months ago

Closed 9 months ago

#21278 closed defect (fixed)

Avoid signed integer underflow when comparing versions (Fix TROVE-2017-001)

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There's a medium-severity bug to fix here.

Child Tickets

Change History (18)

comment:1 Changed 10 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 9 months ago by nickm

Status: acceptedassigned

comment:3 Changed 9 months ago by nickm

Status: assignedneeds_review
Summary: Fix TROVE-2017-001Avoid signed integer underflow when comparing versions (Fix TROVE-2017-001)

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.

comment:4 Changed 9 months ago by teor

Status: needs_reviewneeds_revision

0a2abb3780:

The function comment change does not describe what the function now does: it only returns 0 or 1. And it says "renturn".

4e720ad736:

else has an extra space in:

+  if (a->git_tag_len)
+     return fast_memcmp(a->git_tag, b->git_tag, a->git_tag_len);
+   else

comment:5 Changed 9 months ago by teor

Also, from arma:

4e720ad736:

This bug is harmless, except when Tor has been build with

s/build/built/

comment:6 Changed 9 months ago by teor

Also, I think we should patch the 64-bit to 32-bit truncation bug in #21450 at the same time as this.

It means that 32-bit and 64-bit tors treat version components >= INT32_MAX differently.

comment:7 in reply to:  3 Changed 9 months ago by arma

Replying to nickm:

My branch bug21278_024_v2 tries to fix this

I pushed a bug21278_024_v3 branch that has an extra commit to better document the new function.

That said, I think your commits 2c768c2c0 and 54e2e027 are only for dir auths, right? So there isn't any point in backporting them earlier than 0.2.9?

Speaking of which, for 4e720ad7 which looks like the main fix here, the changelog stanza says This bug is harmless, except when Tor has been build with --enable-expensive-hardening, which would turn it into a crash. Am I remembering correctly that only recent Tor branches have put expensive-hardening on by default? That is, the earlier fix for this TROVE (i.e. disabling ftrapv) only went into 0.2.9.x and 0.3.0.x? Why backport a fix for a harmless bug so far back? :)

comment:8 in reply to:  3 Changed 9 months ago by arma

Replying to nickm:

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.

I'm a big fan of what 0a2abb37 is doing (modulo the fixes that teor suggests). Since it's more of a belt-and-suspenders thing, is it more suited for 0.3.0?

Whereas 557385874 is dir auth only (I think?), so there's no need for it to go earlier than 0.2.9. But you never know when sha1 collisions are going to drop, so I think putting it in 0.2.9 is reasonable if you want to do that. (On the other hand, once sha1 goes bad, "hey directory authorities, either start using the default value of --enable-fragile or upgrade to 0.3.0.x" seems like a totally doable statement too.

comment:9 in reply to:  6 Changed 9 months ago by arma

Replying to teor:

Also, I think we should patch the 64-bit to 32-bit truncation bug in #21450 at the same time as this.

It means that 32-bit and 64-bit tors treat version components >= INT32_MAX differently.

I can get behind this plan (to merge the fix for 21450 to 0.3.0, or to 0.3.0 and 0.2.9, depending on how far back we decide to backport).

comment:10 Changed 9 months ago by nickm

Am I remembering correctly that only recent Tor branches have put expensive-hardening on by default? That is, the earlier fix for this TROVE (i.e. disabling ftrapv) only went into 0.2.9.x and 0.3.0.x?

This bug affects everybody who has trapv or ubsan turned on. In 0.2.9.1-alpha, we turned trapv on by default, which caused this bug to affect 0.2.9.1-alpha through 0.2.9.8.

This bug will still affect all older versions if they have --enable-expensive-hardening turned on.

comment:11 Changed 9 months ago by nickm

Status: needs_revisionneeds_review

Okay. So here's where we stand:

  • I have a bug21278_024_v4 that has only the minimal fix for the integer issue. I propose that it go into 0.2.4.
  • I have a bug21278_redux_029 that blocks the bogus versions at the directory level, and includes a changes file and roger's function documentation. I propose that it go into 0.2.9.
  • I agree that it's okay to merge bug21278_024_v2_extra to 0.2.9. I have a bug21278_extra_029 branch to port those forward. I'm okay with taking that in 0.2.9 or 0.3.0.

comment:12 in reply to:  11 Changed 9 months ago by teor

Status: needs_reviewneeds_revision

Replying to nickm:

Okay. So here's where we stand:

  • I have a bug21278_024_v4 that has only the minimal fix for the integer issue. I propose that it go into 0.2.4.

This is ok, as anyone on a private network can stop their own relays misbehaving.
make check passes for me on macOS 10.12 i386 and x86_64.

  • I have a bug21278_redux_029 that blocks the bogus versions at the directory level, and includes a changes file and roger's function documentation. I propose that it go into 0.2.9.

This refuses to compile for me with:

src/or/routerparse.c:5555:32: error: comparison of unsigned enum expression < 0
      is always false [-Werror,-Wtautological-compare]
        router_version->status < 0 ||

on:

clang -arch i386 --version >&5
clang version 3.9.1 (tags/RELEASE_391/final)
Target: i386-apple-darwin16.4.0
Thread model: posix

The 64-bit arch compiles and passes make check test-network-all.

I don't know what the extra newline is doing in f1c2cea165, but that's a nitpick.

The changes file in 1ff289a745 includes trailing whitespace.

  • I agree that it's okay to merge bug21278_024_v2_extra to 0.2.9. I have a bug21278_extra_029 branch to port those forward. I'm okay with taking that in 0.2.9 or 0.3.0.

I think 029, in case there is a security issue here.

make check test-network-all passes for me on macOS 10.12 i386 and x86_64.

comment:13 Changed 9 months ago by teor

(After the nitpicks are fixed or ignored, I'm happy for this to go into marge-ready.)

comment:14 Changed 9 months ago by nickm

Status: needs_revisionmerge_ready

7697736fcd5e6e46a5e92da974ed5288ae7334ec is a fixup on bug21278_redux_029 to resolve the tautological comparison issue.

The extra newline is Roger's; I don't mind leaving it in for now.

Squashing and merging!

comment:15 Changed 9 months ago by teor

Should we add a unit test for this? Or did we add a fuzzing test case for it?
(In general, how do we want to test for TROVE regressions?)

comment:16 Changed 9 months ago by nickm

Either would be fine with me.

comment:17 Changed 9 months ago by nickm

(Merged)

comment:18 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks to Teor for opening #21470 to track the regression test issue. Closing this one.

Note: See TracTickets for help on using tickets.