Opened 11 months ago

Closed 8 months ago

#20894 closed defect (fixed)

Resolve read-off-end-of-buffer on atoi in fetch_from_buf_http (TROVE-2016-10-001)

Reported by: teor Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-03-unspecified-201612
Cc: chadmiller Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

Since we're releasing the fuzzing code (#20893) that reveals the underlying bug in #20384, we should also fix that bug.

It's entirely safe to fix the bug in 0.3.0, because the mitigation applied in #20384 works.

When we fix it, we should credit:

Discovered by fuzzing using afl: http://lcamtuf.coredump.cx/afl/

It would be nice to email the maintainer with this ticket number and let them know, so they can add it to their gallery.

Child Tickets

Change History (12)

comment:1 Changed 11 months ago by nickm

+1 on credit, and +1 on letting the maintainer know.

comment:2 Changed 10 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:3 Changed 9 months ago by chadmiller

Cc: chadmiller added

comment:4 Changed 9 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.0.x-final

comment:5 Changed 9 months ago by nickm

Owner: set to nickm
Status: newassigned

comment:6 Changed 9 months ago by nickm

Summary: Fix known instance of TROVE-2016-10-001Resolve read-off-end-of-buffer on atoi in fetch_from_buf_http (TROVE-2016-10-001)

The problem was the atoi() in fetch_from_buf_http: it's entirely too happy to read off the end of a buf if there is no subsequent '\n' in the same chunk as the "content-length".

We fixed this with the patch for #20384, where we made sure that every buf chunk was NUL-terminated, but we really ought to fix the underlying issue too.

I have an overcomplicated patch in bug20894_024. Probably it should use tor_parse_uint64 instead of atoi. But the part that I really dislike is the hunt-for-the-newline-and-copy-the-header part -- it's overcomplicated and more than a little bit zany.

comment:7 Changed 8 months ago by nickm

Priority: MediumHigh
Status: assignedneeds_review

My branch bug20894_024_v2 is now updated to use a separate function here, and to include a unit test. It's written a little bit oddly for backward compatibility with 0.2.4's compiler flags and coding style, but it's fairly clean. Please review?

comment:8 in reply to:  description Changed 8 months ago by teor

Status: needs_reviewneeds_revision

Can headers+headerlen can wrap here?

If so, we also need:
tor_assert(headers < SIZE_T_MAX - headerlen);
Before every time we do headers+headerlen.
(And before:
p = (char*) tor_memstr(headers, headerlen, CONTENT_LENGTH);
which effectively does headers+headerlen.)

Please credit AFL in the changes file:

Discovered by fuzzing using AFL: http://lcamtuf.coredump.cx/afl/

Replying to teor:

It would be nice to email the maintainer with this ticket number and let them know, so they can add it to their gallery.

I emailed the AFL maintainer today and CC'd tor-team.
This bug is now linked from http://lcamtuf.coredump.cx/afl/

comment:9 Changed 8 months ago by arma

Nickm tells me he's confident that the sentinel patch (already applied back through 0.2.4) has resolved the security issue. So this is just to clean up the code to make things better for our future? That sounds like a great thing to put into Tor 0.3.0 (like the body of this ticket suggests).

comment:10 Changed 8 months ago by nickm

Status: needs_revisionneeds_review

Can headers+headerlen can wrap here?

I believe it can't, since headers is a pointer to a place in a buffer, and headerlen is an amount of memory that's readable at that point.

I've forward-ported to 0.2.9, moved the unit test, added a correct use of STATIC, and credited AFL in a branch bug20894_029_v3. I'm fine taking this in 0.3.0 or 0.2.9.

comment:11 Changed 8 months ago by teor

Status: needs_reviewmerge_ready

Looks good!
make check test-network-all runs for me on macOS 10.12 x86_64 and i386.

I can't see any reason to take this in 0.2.9, given that this changes quite a bit of code for a stable release.

comment:12 Changed 8 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

hm, okay. Taking it in 0.3.0, with no backport planned.

Note: See TracTickets for help on using tickets.