[tor-bugs] #32363 [Core Tor/Tor]: tor_inet_aton parsing of IPv4 literals is too lax

[Tor Bug Tracker & Wiki]

#32363: tor_inet_aton parsing of IPv4 literals is too lax
--------------------------+------------------------------------
 Reporter:  liberat       |          Owner:  neel
     Type:  defect        |         Status:  needs_revision
 Priority:  Medium        |      Milestone:  Tor: 0.4.3.x-final
Component:  Core Tor/Tor  |        Version:  Tor: 0.4.1.6
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:
Parent ID:                |         Points:
 Reviewer:  dgoulet       |        Sponsor:
--------------------------+------------------------------------
Changes (by teor):

 * cc: nickm (added)
 * status:  needs_review => needs_revision


Comment:

 Some feedback:

 **Tests**

 I'd like to see more tests here:
 * pass: a zero in the first, last, and a middle position, and
 * fail: an octal value in the first, last, and a middle position.

 **Consensus Changes**

 It's also worth noting that this change modifies directory document
 parsing. So authorities will reject some descriptors and votes that were
 previously valid. That's ok, because authorities can safely reject more
 directory documents, without breaking the consensus. (And Tor doesn't
 produce directory documents with octal IPv4 addresses by default.)

 **Implementation**

 This code looks correct, but it took me about 5 minutes to check it. We
 try not to write manual string-parsing code, because it's error-prone, and
 hard to maintain.

 Instead of splitting the string manually, you could do something like:
 {{{
 bool is_octal = false;

 smartlist_t *sl = smartlist_new();
 smartlist_split_string(sl, str, ".", 0, 0);
 SMARTLIST_FOREACH(sl, const char *, octet, is_octal = (strlen(octet) > 1
 && octet[0] == '0'));
 SMARTLIST_FOREACH(sl, char *, octet, tor_free(octet));
 smartlist_free(sl);

 if (is_octal)
   return 0;
 }}}

 Here are the relevant smartlist functions and macros:

 smartlist_split_string():
 https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/lib/smartlist_core/smartlist_split.c#L21

 SMARTLIST_FOREACH():
 https://github.com/torproject/tor/blob/4f02812242d1fd90d859eb98ac3fb1ed182f18cf/src/lib/smartlist_core/smartlist_foreach.h#L104

 You'd also need to add:
 {{{
 lib/smartlist_core/*.h
 }}}
 to `lib/net/.may_include`.

 But I think the extra dependency is worth it, to make the code simpler.

 Before you make this change, neel, I'd like to check what dgoulet thinks.
 I'd also like to check with nickm that there's no reason we're avoiding a
 smartlist dependency at this level.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32363#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online