tor_inet_aton parsing of IPv4 literals is too lax

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.

Trac:
Username: liberat

changed milestone to %Tor: 0.4.4.x-final

added BugSmashFund component::core tor/tor extra-review milestone::Tor: 0.4.4.x-final owner::neel points::0.2 priority::medium reporter::liberat resolution::fixed reviewer::nickm severity::normal status::closed type::enhancement labels

Trac:
Milestone: N/A to Tor: 0.4.3.x-final

Trac:
Status: new to assigned
Cc: N/A to neel
Owner: N/A to neel

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

Trac:
Status: assigned to needs_review

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]

Trac:
Status: needs_review to needs_revision

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

New PR since the old one has merge conflicts.

Trac:
Status: needs_revision to needs_review

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

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

Trac:
Reviewer: N/A to dgoulet
Status: needs_review to needs_revision

Made the change.

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision
Cc: neel to neel, nickm

Trac:
Status: needs_revision to needs_review

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

Trac:
Keywords: N/A deleted, 043-can added

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.

Trac:
Reviewer: dgoulet to dgoulet, nickm, teor

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 ?

Trac:
Reviewer: dgoulet, nickm, teor to nickm
Points: N/A to 0.2
Version: Tor: 0.4.1.6 to N/A
Keywords: 043-can deleted, BugSmashFund added
Type: defect to enhancement
Milestone: Tor: 0.4.3.x-final to Tor: 0.4.4.x-final

Trac:
Keywords: N/A deleted, extra-review added

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?

Replying to 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.

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

Trac:
Status: needs_review to merge_ready

Squashed and merged to master.

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

closed

changed time estimate to 1h 36m

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