Opened 7 months ago

Closed 3 months ago

#30040 closed defect (fixed)

Double-free bug on huge bandwidth file in some platforms

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.5.1-alpha
Severity: Normal Keywords: teor-merge, security-low?, bw-auth, double-free, hackerone, bug-bounty, 040-backport, 035-backport, consider-backport-after-0405
Cc: Actual Points: 0.4
Parent ID: Points: 0.3
Reviewer: asn Sponsor:

Description

Here is a very situational double-free bug reported in hackerone from bug hunter paldium. It's a low-severity item since bandwidth files are considered trusted input, and anyone who controls a bandwidth file can cause worse disasters than double-frees. Also it only applies on very specific platforms that none of our dirauths use.

Details:
The function compat_getdelim_ is used for tor_getline if tor is compiled
on a system that lacks getline and getdelim. These systems should be
very rare, considering that getdelim is POSIX.

If this system is further a 32 bit architecture, it is possible to
trigger a double free with huge files.

If bufsiz has been already increased to 2 GB, the next chunk would
be 4 GB in size, which wraps around to 0 due to 32 bit limitations.

A realloc(*buf, 0) could be imagined as "free(*buf); return malloc(0);"
which therefore could return NULL. The code in question considers
that an error, but will keep the value of *buf pointing to already
freed memory.

The caller of tor_getline() would free the pointer again, therefore
leading to a double free.

This code can only be triggered in dirserv_read_measured_bandwidths
with a huge measured bandwith list file on a system that actually
allows to reach 2 GB of space through realloc.

It is not possible to trigger this on Linux with glibc or other major
*BSD systems even on unit tests, because these systems cannot reach
so much memory due to memory fragmentation.

This patch is effectively based on the penetration test report of
cure53 for curl available at https://cure53.de/pentest-report_curl.pdf
and explained under section "CRL-01-007 Double-free in aprintf() via
unsafe size_t multiplication (Medium)".

## Impact

Successfully triggering a double free can corrupt the heap
which might allow more sophisticated attacks within the
tor application.

Child Tickets

Attachments (1)

0001-Prevent-double-free-on-huge-files-with-32-bit.patch (2.1 KB) - added by asn 7 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 months ago by asn

attaching patch supplied by bug reporter

comment:2 Changed 7 months ago by asn

Keywords: 040-must added

comment:3 Changed 7 months ago by teor

Keywords: security-low? 040-backport added

comment:4 Changed 7 months ago by nickm

Status: newneeds_review

comment:5 Changed 7 months ago by asn

Reviewer: asn

comment:6 Changed 7 months ago by asn

Status: needs_reviewmerge_ready

Patch LGTM. The commit msg was really useful in understanding this.
Please see https://github.com/torproject/tor/pull/918 for a PR that also contains a changes file.

comment:7 Changed 7 months ago by nickm

Keywords: 035-backport added
Status: merge_readyneeds_revision

Let's have this branch against 0.3.5 rather than master?

comment:8 Changed 7 months ago by asn

Status: needs_revisionneeds_review

comment:9 Changed 7 months ago by asn

Status: needs_reviewmerge_ready

comment:10 Changed 7 months ago by teor

Actual Points: 0.4
Keywords: consider-backport-after-0405-alpha added
Version: Tor: 0.3.5.1-alpha

We should test this fix in one alpha, then backport it.

comment:11 Changed 7 months ago by teor

Keywords: teor-merge added

I'll merge this in my morning.

comment:12 Changed 7 months ago by nickm

Keywords: 040-must removed
Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final

Merged to 0.4.0 and forward; let's backport if it works in 0.4.0.4.rc

comment:13 Changed 7 months ago by nickm

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

comment:14 Changed 7 months ago by teor

Keywords: consider-backport-after-0405 added; consider-backport-after-0405-alpha removed

Drop the -alpha from backport tags

comment:15 Changed 3 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Backported to 0.3.5.
Merged with the other 0.3.5 and 0.4.0 backports on 2019-08-12.

Note: See TracTickets for help on using tickets.