Opened 3 months ago

Last modified 3 days ago

#32363 needs_review defect

tor_inet_aton parsing of IPv4 literals is too lax

Reported by: liberat Owned by: neel
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.6
Severity: Normal Keywords: 043-can
Cc: neel, nickm Actual Points:
Parent ID: Points:
Reviewer: dgoulet, nickm, teor Sponsor:

Description

The function tor_inet_aton accepts strings that include leading zeroes.

For example, "010.010.010.010" is parsed as "10.10.10.10".

This could potentially be a problem because "010.010.010.010" is obsolete notation for an octal IP address.

At least in glibc, inet_aton or getaddrinfo treats "010.010.010.010" as "8.8.8.8", whereas inet_ntop rejects it as invalid.

Child Tickets

Change History (12)

comment:1 Changed 3 months ago by nickm

Milestone: Tor: 0.4.3.x-final

comment:2 Changed 5 weeks ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:3 Changed 5 weeks ago by neel

Status: assignedneeds_review

comment:4 Changed 2 weeks ago by dgoulet

Status: needs_reviewneeds_revision

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]

comment:5 Changed 2 weeks ago by neel

Status: needs_revisionneeds_review

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

New PR since the old one has merge conflicts.

comment:6 Changed 2 weeks ago by dgoulet

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

comment:7 Changed 9 days ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

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

comment:8 Changed 9 days ago by neel

Status: needs_revisionneeds_review

Made the change.

comment:9 Changed 8 days ago by teor

Cc: nickm added
Status: needs_reviewneeds_revision

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.

comment:10 Changed 8 days ago by neel

Status: needs_revisionneeds_review

comment:11 Changed 7 days ago by nickm

Keywords: 043-can added

tag all currently needs_review tickets with 043-can. (Since there's code before the feature freeze, maybe we can take it.)

comment:12 Changed 3 days ago by dgoulet

Reviewer: dgouletdgoulet, nickm, teor

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.

Note: See TracTickets for help on using tickets.