Re: [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:  enhancement | Status:  closed
 Priority:  Medium  |  Milestone:  Tor:
|  0.4.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:  fixed
 Keywords:  BugSmashFund, extra-review  |  Actual Points:
Parent ID:  | Points:  0.2
 Reviewer:  nickm   |Sponsor:
+--
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => fixed


Comment:

 Squashed and merged to master.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  enhancement | Status:  merge_ready
 Priority:  Medium  |  Milestone:  Tor:
|  0.4.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  BugSmashFund, extra-review  |  Actual Points:
Parent ID:  | Points:  0.2
 Reviewer:  nickm   |Sponsor:
+--
Changes (by nickm):

 * status:  needs_review => merge_ready


Comment:

 Okay.  In that case, I think we can call this merge-ready for 0.4.4

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  enhancement | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor:
|  0.4.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  BugSmashFund, extra-review  |  Actual Points:
Parent ID:  | Points:  0.2
 Reviewer:  nickm   |Sponsor:
+--

Comment (by teor):

 Replying to [comment:15 nickm]:
 > > Now with the last changes, we do use the heap quite a bit with the
 smartlist_t so why would we prefer not using the heap in the first place?
 Does it still matter now?
 >
 > It doesn't matter except to the extent that it slows us down... we
 should keep an eye on profiles after we merge this.
 >
 > > Can we add a smartlist_core dependency to lib/net ?
 >
 > Yes: to see why, run `./scripts/maint/practracker/includes.py
 --toposort` for a topological sort of our current modules from lowest to
 highest level.  It appears that `net` is already much higher than
 smartlist_core.
 >
 > One thing I want to think about, though: do we care about the
 fingerprinting issue?  That is, this patch will create a class of
 addresses that some clients will parse and some clients will reject.  Is
 that a security issue to worry about, or are we cool here?

 Authorities canonicalise addresses before they create the microdesc
 consensus and microdescriptors, so the only affected clients are bridge
 clients. (Bridge clients download relay descriptors.)

 And for clients that use full relay descriptors on the public network,
 authorities will stop voting for those relays pretty soon.

 So I don't think this has a significant impact.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  enhancement | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor:
|  0.4.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  BugSmashFund, extra-review  |  Actual Points:
Parent ID:  | Points:  0.2
 Reviewer:  nickm   |Sponsor:
+--

Comment (by nickm):

 > Now with the last changes, we do use the heap quite a bit with the
 smartlist_t so why would we prefer not using the heap in the first place?
 Does it still matter now?

 It doesn't matter except to the extent that it slows us down... we should
 keep an eye on profiles after we merge this.

 > Can we add a smartlist_core dependency to lib/net ?

 Yes: to see why, run `./scripts/maint/practracker/includes.py --toposort`
 for a topological sort of our current modules from lowest to highest
 level.  It appears that `net` is already much higher than smartlist_core.

 One thing I want to think about, though: do we care about the
 fingerprinting issue?  That is, this patch will create a class of
 addresses that some clients will parse and some clients will reject.  Is
 that a security issue to worry about, or are we cool here?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  enhancement | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor:
|  0.4.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  BugSmashFund, extra-review  |  Actual Points:
Parent ID:  | Points:  0.2
 Reviewer:  nickm   |Sponsor:
+--
Changes (by teor):

 * keywords:  BugSmashFund => BugSmashFund, extra-review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  enhancement   | Status:  needs_review
 Priority:  Medium|  Milestone:  Tor: 0.4.4.x-final
Component:  Core Tor/Tor  |Version:
 Severity:  Normal| Resolution:
 Keywords:  BugSmashFund  |  Actual Points:
Parent ID:| Points:  0.2
 Reviewer:  nickm |Sponsor:
--+
Changes (by teor):

 * version:  Tor: 0.4.1.6 =>
 * points:   => 0.2
 * milestone:  Tor: 0.4.3.x-final => Tor: 0.4.4.x-final
 * keywords:  043-can => BugSmashFund
 * reviewer:  dgoulet, nickm, teor => nickm
 * type:  defect => enhancement


Comment:

 This code looks fine to me, but I'd like nickm to answer this question
 before we merge:

 Can we add a smartlist_core dependency to lib/net ?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_review
 Priority:  Medium|  Milestone:  Tor: 0.4.3.x-final
Component:  Core Tor/Tor  |Version:  Tor: 0.4.1.6
 Severity:  Normal| Resolution:
 Keywords:  043-can   |  Actual Points:
Parent ID:| Points:
 Reviewer:  dgoulet, nickm, teor  |Sponsor:
--+
Changes (by dgoulet):

 * reviewer:  dgoulet => dgoulet, nickm, teor


Comment:

 One comment says: `This avoids using the heap.`.

 Now with the last changes, we do use the heap quite a bit with the
 `smartlist_t` so why would we prefer not using the heap in the first
 place? Does it still matter now?

 Apart from that, this looks OK to me.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_review
 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 neel):

 * status:  needs_revision => needs_review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_review
 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 neel):

 * status:  needs_revision => needs_review


Comment:

 Made the change.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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 dgoulet):

 * status:  needs_review => needs_revision
 * reviewer:   => dgoulet


Comment:

 We should be good, I would really like just the small fixup I asked for.
 Thanks!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_review
 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:|Sponsor:
--+

Comment (by dgoulet):

 Left a comment on the PR. Might be something, might be nothing.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_review
 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:|Sponsor:
--+
Changes (by neel):

 * status:  needs_revision => needs_review


Comment:

 New PR: https://github.com/torproject/tor/pull/1639

 New PR since the old one has merge conflicts.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:|Sponsor:
--+
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 CI is failing:

 {{{
 addr/ip6_helpers:
   FAIL src/test/test_addr.c:662: assert(r OP_EQ AF_INET): -1 vs 2
   [ip6_helpers FAILED]
 }}}

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_review
 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:|Sponsor:
--+
Changes (by neel):

 * status:  assigned => needs_review


Comment:

 PR: https://github.com/torproject/tor/pull/1630

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  assigned
 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:|Sponsor:
--+
Changes (by neel):

 * status:  new => assigned
 * cc: neel (added)
 * owner:  (none) => neel


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  (none)
 Type:  defect| Status:  new
 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:|Sponsor:
--+
Changes (by nickm):

 * milestone:   => Tor: 0.4.3.x-final


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs