Opened 6 months ago

Closed 7 weeks ago

#31001 closed defect (fixed)

Undefined behavior in tor_vasprintf()

Reported by: asn Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: hackerone, bug-bounty, security-low, unlikely-crash, 029-backport, 035-backport, 040-backport, 041-backport, dgoulet-merge, consider-backport-after-0421
Cc: Actual Points: .1
Parent ID: Points: 0.5
Reviewer: catalyst Sponsor:

Description

Overflowing a signed integer in C is an undefined behaviour.
It is possible to trigger this undefined behaviour in tor_asprintf on
Windows or systems lacking vasprintf.

On these systems, eiter _vscprintf or vsnprintf is called to retrieve
the required amount of bytes to hold the string. These functions can
return INT_MAX. The easiest way to recreate this is the use of a
specially crafted configuration file, e.g. containing the line:

FirewallPorts AAAAA<in total 2147483610 As>

This line triggers the needed tor_asprintf call which eventually
leads to an INT_MAX return value from _vscprintf or vsnprintf.

The needed byte for \0 is added to the result, triggering the
overflow and therefore the undefined behaviour.

Casting the value to size_t before addition fixes the behaviour.

Child Tickets

Change History (12)

comment:1 Changed 6 months ago by asn

Status: newneeds_review

Bug found by Tobias Stoeckmann.

Patch by Tobias can be found here: https://github.com/torproject/tor/pull/1144

comment:2 Changed 6 months ago by teor

Keywords: security-low unlikely-crash 029-backport 035-backport 040-backport 041-backport added
Points: 0.5
Status: needs_reviewneeds_revision

This patch makes sense to me, and it passes CI.

I'm marking it as security-low, because most common compilers don't aggressively optimise signed overflow in this context.
(If they did, this code could introduce some nasty bugs in tor.)

So the negative value will be converted to size_t by adding SIZE_T_MAX.
On 32-bit systems, that's the correct value, on 64-bit systems, that's UINT64_MAX - INT32_MIN, which will fail to malloc and crash.
Fortunately, most of Tor's parsers have document size limits that are much lower than 2GB.

But we still need to backport this fix to compact.c in 0.2.9, and then merge forward.

comment:3 Changed 5 months ago by nickm

Owner: set to nickm
Status: needs_revisionaccepted

I'll do the backport

comment:4 Changed 5 months ago by nickm

Actual Points: .1
Status: acceptedneeds_review

I have two branches here: bug31001_029 for maint-0.2.9, and bug31001_035 for maint-0.3.5 and later.

PR for 0.2.9: https://github.com/torproject/tor/pull/1178
PR for 0.3.5: https://github.com/torproject/tor/pull/1179
PR for master (for CI purposes): https://github.com/torproject/tor/pull/1180

comment:5 Changed 5 months ago by dgoulet

Reviewer: catalyst

comment:6 in reply to:  4 Changed 5 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

I have two branches here: bug31001_029 for maint-0.2.9, and bug31001_035 for maint-0.3.5 and later.

PR for 0.2.9: https://github.com/torproject/tor/pull/1178
PR for 0.3.5: https://github.com/torproject/tor/pull/1179
PR for master (for CI purposes): https://github.com/torproject/tor/pull/1180

Thanks! These look good to me.

comment:7 Changed 5 months ago by nickm

Keywords: dgoulet-merge added

Please merge to 0.4.0 and forward, and mark for backport?

comment:8 Changed 5 months ago by dgoulet

Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final

Merged in 041 and forward! Moving to 040 for backport.

comment:9 Changed 4 months ago by teor

Keywords: consider-backport-after-0416 added

This PR was merged after 0.4.1.4, so we will test it until 0.4.1.6 is released, then merge.

comment:10 Changed 4 months ago by nickm

Keywords: 041-must removed

comment:11 Changed 4 months ago by teor

Keywords: consider-backport-after-0421 added; consider-backport-after-0416 removed

0.4.1.5 was stable, so these get backported after the next alpha

comment:12 Changed 7 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to 0.2.9 and later.
Merged #32106, #31807, #31001, #23818, #12399, and #31372 together.

Note: See TracTickets for help on using tickets.