Opened 6 weeks ago

Closed 6 weeks ago

#21450 closed defect (fixed)

Consistently parse tor versions regardless of word size

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

Description

We use strtol() in tor_version_parse(), but longs are different sizes on 32-bit and 64-bit.

Casting long to int means that some versions that look different on 64-bit platforms could be truncated and look the same on 32-bit platforms. And some versions that parse on 64-bit platforms fail to parse on 32-bit platforms (particularly after #21278, because the cast makes some of them negative).

This fix does not need a new consensus method, because we reject router descriptors with version components that don't parse in #21278.

Here's my patch:

diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index 58b9a22438..9d8ef11ac7 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -4840,6 +4840,7 @@ tor_version_parse(const char *s, tor_version_t *out)
 {
   char *eos=NULL;
   const char *cp=NULL;
+  int ok = 1;
   /* Format is:
    *   "Tor " ? NUM dot NUM [ dot NUM [ ( pre | rc | dot ) NUM ] ] [ - tag ]
    */
@@ -4855,7 +4856,9 @@ tor_version_parse(const char *s, tor_version_t *out)
 
 #define NUMBER(m)                               \
   do {                                          \
-    out->m = (int)strtol(cp, &eos, 10);         \
+    out->m = (int)tor_parse_uint64(val, 10, 0, INT32_MAX, &ok, &eos); \
+    if (!ok)                                    \
+      return -1;                                \
     if (!eos || eos == cp)                      \
       return -1;                                \
     cp = eos;                                   \

This might also need a torspec patch saying that INT_MAX is the limit, or that implementations can place limits on version numbers.

Child Tickets

Change History (4)

comment:1 follow-up: Changed 6 weeks ago by arma

Should we take the opportunity to put a much lower bound on the range of version subcomponents? Like, even if we start putting out Tor 0.3.0.20170215, that's still way lower than INT32_MAX. :)

(I am also fine with "maybe but how will we pick the lower bound, let's just stick with INT32_MAX and have the discipline not to pick stupid version numbers.)

comment:2 in reply to: ↑ 1 Changed 6 weeks ago by teor

Replying to arma:

Should we take the opportunity to put a much lower bound on the range of version subcomponents? Like, even if we start putting out Tor 0.3.0.20170215, that's still way lower than INT32_MAX. :)

20170215 > 2^25, so I can't see any smaller limit, at least on a byte boundary.

(I am also fine with "maybe but how will we pick the lower bound, let's just stick with INT32_MAX and have the discipline not to pick stupid version numbers.)

+1

comment:3 Changed 6 weeks ago by nickm

  • Status changed from new to merge_ready

I'm fine with taking this along with the dirauth part of #21278.

comment:4 Changed 6 weeks ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed

Applied to 0.2.9 as cb6b3b7cadb641b648577e5d5536735222cc68da. Thank you!

Note: See TracTickets for help on using tickets.