Opened 8 years ago

Closed 7 years ago

#8037 closed defect (fixed)

Specialy crafter microdesc could trigger to flush up to 16MB uninited heap allocated memory to media

Reported by: cypherpunks Owned by:
Priority: Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

microdescs_parse_from_string() and so on func do not count string as null terminated and allows to process "string" with zero byte in middle. md->body = tor_strndup(cp, md->bodylen) duplicate only partial of such string. dump_microdescriptor() flushes all bodylen of md's body to disk. Specially crafted bytes append to valid md sent by directory cache could lead to flush up to 16MB of memory data to media. Tor tries to clear sensitive data on free(), but some non cleared memory still no need to write in plain text to media.

Child Tickets

Change History (12)

comment:1 Changed 8 years ago by cypherpunks

Component: - Select a componentTor
Milestone: Tor: 0.2.4.x-final

comment:2 Changed 8 years ago by cypherpunks

Resolution: invalid
Status: newclosed

oops I missed a "little" details like
crypto_digest256(md->digest, md->body, md->bodylen, DIGEST_SHA256)
and "Received non-requested microcdesc"
Sorry it was wrong.

comment:3 Changed 8 years ago by cypherpunks

Auths could generate such md, of course they could to do more worse things but it's not good to flush memory to media even if auths decide so. It would be better to change tor_strndup to memcpy.

comment:4 Changed 8 years ago by rransom

Resolution: invalid
Status: closedreopened

This is at least a quality-of-implementation issue.

comment:5 Changed 8 years ago by nickm

Keywords: tor-client easy added
Priority: normalminor

Agreed; this should be a memdup. Anybody want to make a little branch with a changes file and commit message?

comment:6 Changed 8 years ago by cypherpunks

    extrainfo->cache_info.signed_descriptor_body = tor_strndup(s, end-s);

Then need to fix it for consistency too. Non exploitable.

comment:7 Changed 8 years ago by nickm

Status: reopenedneeds_review

So, there's a possible fix in branch "bug8037" in my public repository. But maybe we should just check for NUL bytes and reject the descriptor if they're present.

comment:8 Changed 8 years ago by cypherpunks

But maybe we should just check for NUL bytes and reject the descriptor if they're present.

Not instead but together with it. Cache copying of every document should be consisted to one way, strndup or memdup. We need to think about binary document future right now.

tokenize_string could to check for NUL byte if const char *end present.

comment:9 Changed 8 years ago by cypherpunks

Or we could to change get_next_token() so it check every token line and object(if we'd assume openssl can accept such).

comment:10 Changed 7 years ago by nickm

tokenize_string could to check for NUL byte if const char *end present

bug8037 now does that too.

comment:11 Changed 7 years ago by andrea

This one looks okay to me.

comment:12 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Added a tweak to fix the memchr invocation argument order (oops), squashed it as bug8037_squashed, and merged. Thanks!

Note: See TracTickets for help on using tickets.