Opened 18 months ago

Closed 14 months ago

Last modified 13 months ago

#30041 closed defect (fixed)

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

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


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 (17)

comment:1 Changed 18 months ago by asn

attached patches supplied by paldium

comment:2 Changed 18 months ago by asn

Keywords: 040-must added

comment:3 Changed 18 months 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 18 months ago by nickm

Status: newneeds_review

comment:5 Changed 18 months ago by asn

Reviewer: nickm

comment:6 Changed 18 months 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 and respectively. Let's make sure the CI passes.

comment:7 Changed 18 months 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 18 months 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 17 months ago by teor

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

Drop the -alpha from backport tags

comment:10 Changed 16 months ago by nickm

Keywords: 040-must removed

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

comment:11 Changed 16 months ago by nickm

Keywords: 034-backport removed

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:12 Changed 14 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.2.9.x-final
Resolution: fixed
Status: merge_readyclosed

Merged to 0.2.9 and 0.3.5.
Created to reword the fixup commit.
Merged to 0.3.5 by deleting the old modified file.

comment:13 Changed 13 months ago by cypherpunks

i believe my issue is related. will this limit fix my bugs or should i open new ticket?

[warn] {BUG} Bug: Non-fatal assertion !(buf->datalen >= INT_MAX - at_most) failed in buf_read_from_tls at buffers_tls.c:73. (Stack trace not available) (on Tor )

comment:14 in reply to:  13 Changed 13 months ago by teor

Replying to cypherpunks:

i believe my issue is related. will this limit fix my bugs or should i open new ticket?

[warn] {BUG} Bug: Non-fatal assertion !(buf->datalen >= INT_MAX - at_most) failed in buf_read_from_tls at buffers_tls.c:73. (Stack trace not available) (on Tor )

Looks like #25957, I will add your logs there.
If you can answer the questions on that ticket, we might be able to make progress,

Note: See TracTickets for help on using tickets.