Opened 4 months ago

Closed 6 weeks 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 4 months ago by nickm

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

comment:2 Changed 3 months ago by nickm

  • Keywords tor-03-unspecified-201612 added
  • Milestone changed from Tor: 0.3.??? to Tor: unspecified

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

comment:3 Changed 7 weeks ago by chadmiller

  • Cc chadmiller added

comment:4 Changed 7 weeks ago by nickm

  • Milestone changed from Tor: unspecified to Tor: 0.3.0.x-final

comment:5 Changed 6 weeks ago by nickm

  • Owner set to nickm
  • Status changed from new to assigned

comment:6 Changed 6 weeks ago by nickm

  • Summary changed from Fix known instance of TROVE-2016-10-001 to Resolve 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 6 weeks ago by nickm

  • Priority changed from Medium to High
  • Status changed from assigned to needs_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 6 weeks ago by teor

  • Status changed from needs_review to needs_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 6 weeks 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 6 weeks ago by nickm

  • Status changed from needs_revision to needs_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 6 weeks ago by teor

  • Status changed from needs_review to merge_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 6 weeks ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed

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

Note: See TracTickets for help on using tickets.