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
Status: | new → needs_review |
---|
comment:2 Changed 6 months ago by
Keywords: | security-low unlikely-crash 029-backport 035-backport 040-backport 041-backport added |
---|---|
Points: | → 0.5 |
Status: | needs_review → needs_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
Owner: | set to nickm |
---|---|
Status: | needs_revision → accepted |
I'll do the backport
comment:4 follow-up: 6 Changed 5 months ago by
Actual Points: | → .1 |
---|---|
Status: | accepted → needs_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
Reviewer: | → catalyst |
---|
comment:6 Changed 5 months ago by
Status: | needs_review → merge_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
Keywords: | dgoulet-merge added |
---|
Please merge to 0.4.0 and forward, and mark for backport?
comment:8 Changed 5 months ago by
Milestone: | Tor: 0.4.1.x-final → Tor: 0.4.0.x-final |
---|
Merged in 041 and forward! Moving to 040 for backport.
comment:9 Changed 4 months ago by
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
Keywords: | 041-must removed |
---|
comment:11 Changed 4 months ago by
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
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
Bug found by Tobias Stoeckmann.
Patch by Tobias can be found here: https://github.com/torproject/tor/pull/1144