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

changed milestone to %Tor: 0.3.0.x-final

added 029-backport component::core tor/tor milestone::Tor: 0.3.0.x-final owner::nickm priority::medium resolution::fixed severity::normal status::closed type::defect labels

Trac:
Owner: N/A to nickm
Status: new to accepted

Trac:
Status: accepted to assigned

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.

Trac:
Status: assigned to needs_review
Summary: Fix TROVE-2017-001 to Avoid signed integer underflow when comparing versions (Fix TROVE-2017-001)

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

Trac:
Status: needs_review to needs_revision

Also, from arma:

4e720ad736:

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

s/build/built/

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

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

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

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.

Replying to teor:

Also, I think we should patch the 64-bit to 32-bit truncation bug in #21450 (moved) 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).

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.

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.

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

(After the nitpicks are fixed or ignored, I'm happy for this to go into marge-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!

Trac:
Status: needs_revision to merge_ready

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?)

Either would be fine with me.

(Merged)

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

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #21450 (moved)

mentioned in issue #21470 (moved)

mentioned in issue #21507 (moved)

moved to tpo/core/tor#21278 (closed)

mentioned in issue tpo/core/tor#21450 (closed)

mentioned in issue tpo/core/tor#21470 (closed)

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

Child items ...

Activity