Opened 7 weeks ago

Last modified 7 days ago

#30041 merge_ready defect

OOB access with huge buffers (src/lib/buf/buffers.c)

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: security-low, hackerone, bug-bounty, 029-backport, 034-backport, 035-backport, 040-backport, consider-backport-after-0405
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

Here is an out-of-bounds read bug found by paldium in hackerone. It's a low-severity bug because it can only be used for DoS, and requires transfer of more than INT_MAX bytes through a connection.

We should backport to 029 and forward anyhow.

# Summary
It is possible to trigger out of boundary accesses with buffers if their content exceeds INT_MAX bytes. See my first patch (0001) how to trigger the issue through unit tests, as this is the easiest way to see what happens. It will result in an out of boundary read. A 64 bit system with at least 5 GB is required for this unit test though!

# Explanation
A buffer consists of one or multiple chunks, which actually contain the data. A chunk contains a memory region and a data pointer. The data pointer points somewhere into the memory, where the actual user data is stored.

Even though a chunk is free to be larger than INT_MAX, it cannot be advised to use such chunks. Whenever a function performs searches or lookups, it is bound to "int" due to buf_pos_t. Many functions properly check that INT_MAX is not exceeded and throw assertions, but unfortunately not all...

Keeping that in mind, I was able to perform a sequence of actions that in fact create chunks with a data length greater than INT_MAX. The int variable "pos" will eventually overflow and access memory far ahead from reserved user data, effectively performing an out of boundary access.

# Exploitation
Generally this is a defensive coding measure to make sure that buffers are safe.

It should be possible to trigger a huge buffer in src/core/mainloop/connection.c. In function connection_buf_read_from_socket a linked connection (directory authentication, as far as I can tell) is joined into the connection buffer with buf_move_to_buf.

The return value of buf_move_to_buf is not properly checked, which means that excessively large data returned from the linked connection can slowly increase the value of "max_to_read" which means that more and more data can be stored in the connection. 

Should it eventually overflow INT_MAX (granted, this takes a LOOONG time), the integer calculation will corrupt the buffer, leading to out of boundary operations.

# Patch
To prevent this issue, both patches (0002 to fix buffers and 0003 to check for error return value) must be applied.

## Impact

Heap data is corrupted and out of boundary read access occur. It will be very hard to extract data with that, because it would be a blind memory check -- and will most likely directly cause a segmentation fault.

Most likely attack vector is therefore denial of service.

Child Tickets

Change History (13)

comment:1 Changed 7 weeks ago by asn

attached patches supplied by paldium

comment:2 Changed 7 weeks ago by asn

Keywords: 040-must added

comment:3 Changed 7 weeks ago by teor

Keywords: security-low 040-backport added; security removed
Milestone: Tor: 0.2.9.x-finalTor: 0.4.1.x-final

Adjust tags

comment:4 Changed 6 weeks ago by nickm

Status: newneeds_review

comment:5 Changed 6 weeks ago by asn

Reviewer: nickm

comment:6 Changed 6 weeks ago by nickm

I've made backport branches as bug30041_029 and bug30041_034; the latter should merge forward cleanly into subsequent branches. I've created pull requests as https://github.com/torproject/tor/pull/921 and https://github.com/torproject/tor/pull/920 respectively. Let's make sure the CI passes.

comment:7 Changed 6 weeks ago by nickm

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

Ready for merge to 0.4.0 and forward; let's check a little more before we take this in earlier versions.

comment:8 Changed 6 weeks ago by teor

Keywords: consider-backport-after-0405-alpha added
Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final
Version: Tor: unspecified

Allocating this backport one alpha's worth of testing: it seems like a low-risk fix. (And we'd also like coverity to run on it.)

Merged to 0.4.0 and merged forward.
Merged #29922 with #30041.

Please remember to fill in the points and actual points!

comment:9 Changed 5 weeks ago by teor

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

Drop the -alpha from backport tags

comment:10 Changed 7 days ago by nickm

Keywords: 040-must removed

Remove 040-must from tickets already solved in 0.4.0 and marked for backport.

Note: See TracTickets for help on using tickets.