Opened 12 months ago

Closed 9 months ago

#32363 closed enhancement (fixed)

tor_inet_aton parsing of IPv4 literals is too lax

Reported by: liberat Owned by: neel
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: BugSmashFund, extra-review
Cc: neel, nickm Actual Points:
Parent ID: Points: 0.2
Reviewer: nickm 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 (18)

comment:1 Changed 12 months ago by nickm

Milestone: Tor: 0.4.3.x-final

comment:2 Changed 11 months ago by neel

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

comment:3 Changed 10 months ago by neel

Status: assignedneeds_review

comment:4 Changed 10 months 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 10 months 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 10 months ago by dgoulet

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

comment:7 Changed 10 months 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 10 months ago by neel

Status: needs_revisionneeds_review

Made the change.

comment:9 Changed 10 months 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 10 months ago by neel

Status: needs_revisionneeds_review

comment:11 Changed 10 months 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 9 months 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.

comment:13 Changed 9 months ago by teor

Keywords: BugSmashFund added; 043-can removed
Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final
Points: 0.2
Reviewer: dgoulet, nickm, teornickm
Type: defectenhancement
Version: Tor: 0.4.1.6

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 ?

comment:14 Changed 9 months ago by teor

Keywords: extra-review added

comment:15 Changed 9 months ago 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?

comment:16 in reply to:  15 Changed 9 months ago by teor

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.

comment:17 Changed 9 months ago by nickm

Status: needs_reviewmerge_ready

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

comment:18 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged to master.

Note: See TracTickets for help on using tickets.