Opened 6 years ago

Closed 6 years ago

#9928 closed defect (fixed)

off-by-one in buffer length check in format_helper_exit_status

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 023-backport tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I think there is a small buffer overflow (off by one) in the current stable
version of tor.

The function in question is format_helper_exit_status, which returns a
formatted hex string. It is in common/util.c, starting at line 3270.
The function has a comment header that explains how it works. It
specifically says it returns a string that is not null terminated, but
instead terminates with a newline.

The code checks periodically throughout the function whether it has written
more bytes than it should. If it does, it errors out and writes a null
character in the current character positions. This by itself can lead to a
buffer overflow, but I believe the checks ensure that it almost never
writes over the buffer size - except in one case.

After it has finished everything, it then checks again if there are more
than 0 bytes left in the buffer. If there are, it writes two characters - a
newline and a null terminator (lines 3342 to 3347).

The problem here is if the buffer only has one byte left, an off by one
overflow occurs. These usually are very hard to exploit, but can be a
security issue nonetheless.

However given that I am not familiar with the tor codebase I might be
wrong? I also did a quick security audit on the rest of the tor code and
couldn't find anything else. I was inspire because of the recent events...

Child Tickets

Change History (5)

comment:1 Changed 6 years ago by nickm

Status: newneeds_review

Branch "bug9928" in my public repository has a simple belt-and-suspenders fix. The commit is over here. It's against 0.2.3, but I suspect it should apply cleanly to later versions too.

comment:2 Changed 6 years ago by asn

Looks good to me. Maybe mention the name of the bug finder in the changelog?

comment:3 Changed 6 years ago by nickm

Done. Any other reviews before I merge?

comment:4 Changed 6 years ago by sysrqb

Looks good to me.

comment:5 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged to 0.2.3 and forward. Thanks, all!

Note: See TracTickets for help on using tickets.