Stop using GeoIP country after buf has gone out of scope

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.

changed milestone to %Tor: 0.2.4.x-final

added 024-backport 025-backport 026-backport 027-backport 028-backport 029-backport 030-backport actualpoints::0 component::core tor/tor memory-safety milestone::Tor: 0.2.4.x-final points::0.2 priority::medium resolution::fixed severity::normal status::closed type::defect labels

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

Sure enough!

Trac:
Keywords: memory-safety deleted, memory-safety 029-backport 030-backport added

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)

Trac:
Keywords: memory-safety 029-backport 030-backport deleted, memory-safety 024-backport 025-backport 026-backport 027-backport 028-backport 029-backport 030-backport added
Actualpoints: N/A to 0
Status: new to needs_review

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.

Trac:
Status: needs_review to merge_ready

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

Merged to maint-0.2.4 and forwards.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.2.4.x-final

nickm, should this be closed?

indeed it should; sorry and thanks for catching it!

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

closed

changed time estimate to 1h 36m

mentioned in issue #24821 (moved)

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