Opened 4 months ago

Closed 4 months ago

#22490 closed defect (fixed)

Stop using GeoIP country after buf has gone out of scope

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: memory-safety 024-backport 025-backport 026-backport 027-backport 028-backport 029-backport 030-backport
Cc: Actual Points: 0
Parent ID: Points: 0.2
Reviewer: Sponsor:

Description

In geoip_parse_entry() (IPv6 case), we find country in buf, let buf go out of scope, then pass country to geoip_add_entry().

Let's stop doing that.

Child Tickets

Change History (9)

comment:1 Changed 4 months ago by teor

(Discovered using clang-5.0's AddressSanitizer stack-use-after-scope.)

comment:2 Changed 4 months ago by arma

Sure enough!

comment:3 Changed 4 months ago by nickm

Keywords: 029-backport 030-backport added

comment:4 Changed 4 months ago by nickm

Actual Points: 0
Keywords: 024-backport 025-backport 026-backport 027-backport 028-backport added
Status: newneeds_review

Looks like this was introduced in a refactoring patch at 6a241ff3ffe7dc1.

Branch bug22490_024 is a minimal fix.

Shall we backport this all the way to 0.2.4? It _is_ undefined behavior, and the fix _does_ seem pretty simple.

(This is not IMO sufficient to force new releases)

comment:5 Changed 4 months ago by teor

Status: needs_reviewmerge_ready

This fix looks good, and I think it should be backported to 0.2.4.

I also don't think it needs a new release, because we're not actually overwriting country with the geoip_add_entry() stack. But we get within 6 bytes of overwriting it.

If the IPv6 addresses were both short, then the geoip_add_entry() stack could overwrite country, and leak pointer values: it allocates 3 pointers on the stack (24 bytes on x86_64, 12 bytes on i386).

But the shortest line is 32 characters (did we have shorter lines in earlier files?), which means that the earliest country could start is at buf+30.

Ah, but wait: pointers must be aligned on word boundaries, and we've added a call to geoip_add_entry() to the stack, as well as any registers we've saved, and any stack smashing protection, any any other compiler bookkeeping.

But I haven't been able to reproduce it locally using clang, macOS, x86_64, and my diagnostic branch assert-country with the command src/or/tor DisableNetwork 1 GeoIPv6File src/config/geoip6, even with two 2-character IPv6 addresses.

So I think this could trigger in rare circumstances, but common configs are safe.

But remember those weird countries we were getting in some relay descriptors?
Did we ever track that down?
This could be it.

comment:6 Changed 4 months ago by nickm

Oh, fine point! Yes, I think this indeed might be the cause of that.

comment:7 Changed 4 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.2.4.x-final

Merged to maint-0.2.4 and forwards.

comment:8 Changed 4 months ago by teor

nickm, should this be closed?

comment:9 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

indeed it should; sorry and thanks for catching it!

Note: See TracTickets for help on using tickets.