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

changed milestone to %Tor: 0.3.0.x-final

added component::core tor/tor milestone::Tor: 0.3.0.x-final owner::nickm points::0.5 priority::high resolution::fixed severity::normal status::closed tor-03-unspecified-201612 type::defect version::tor unspecified labels

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

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

Trac:
Keywords: N/A deleted, tor-03-unspecified-201612 added
Milestone: Tor: 0.3.??? to Tor: unspecified

Trac:
Username: chadmiller
Cc: N/A to chadmiller

Trac:
Milestone: Tor: unspecified to Tor: 0.3.0.x-final

Trac:
Owner: N/A to nickm
Status: new to assigned

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 (moved), 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.

Trac:
Summary: 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)

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?

Trac:
Priority: Medium to High
Status: assigned to needs_review

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/

Trac:
Status: needs_review to needs_revision

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).

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.

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to merge_ready

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

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 4h

mentioned in issue #21470 (moved)

moved to tpo/core/tor#20894 (closed)

mentioned in issue tpo/core/tor#21470 (closed)

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

Child items ...

Activity