Opened 3 weeks ago

Closed 2 weeks 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:


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 3 weeks ago by teor

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

comment:2 Changed 3 weeks ago by arma

Sure enough!

comment:3 Changed 3 weeks ago by nickm

  • Keywords 029-backport 030-backport added

comment:4 Changed 3 weeks ago by nickm

  • Actual Points set to 0
  • Keywords 024-backport 025-backport 026-backport 027-backport 028-backport added
  • Status changed from new to needs_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 3 weeks ago by teor

  • Status changed from needs_review to merge_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 3 weeks ago by nickm

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

comment:7 Changed 3 weeks ago by nickm

  • Milestone changed from Tor: 0.3.1.x-final to Tor: 0.2.4.x-final

Merged to maint-0.2.4 and forwards.

comment:8 Changed 2 weeks ago by teor

nickm, should this be closed?

comment:9 Changed 2 weeks ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed

indeed it should; sorry and thanks for catching it!

Note: See TracTickets for help on using tickets.