Opened 10 years ago

Last modified 7 years ago

#1155 closed defect (Fixed)

memset on mem or memset on circ?

Reported by: arma Owned by:
Priority: Low Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: arma, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


ABCD> "memset(circ, 0xAA, memlen)" and "memset(conn, 0xAA, memlen)"
theoretically incorrect. It should be "memset (mem, 0xAA, memlen)" instead.

ABCD: ah. we're not poisoning all of it, because we're starting at the wrong

ABCD> no, it's right becaouse offset zero. but actually circ and or_circ not
the same

ah. yeah. i was just checking out TO_ORIGIN_CIRCUIT() to make sure the

offset is zero

so the suggestion is to change it to mem, in case one day we make the offset

not zero?
ABCD> yes, at least DOWNCAST(or_connection_t, c) exist for something.

explain that second part?

ABCD> as result it STRUCT_OFFSET(subtype, basemember) which zero if offset
strictly is zero.

we're probably screwed in plenty of other places if we make _base not the

first element of these structs.

also, why the heck are we poisoning the two types of structs with exactly

the same byte?

shouldn't one of them by AB or something?
0xCC would probably make us happier

ABCD> I found only those two cases, introduced by r13414.

ah. those are the commits we added when the cold boot attacks were first

known to us but hadn't been made public yet.

we made sure to scribble on anything that might be sensitive, when we're

done using it.

i do wonder how many compilers optimize it out, though.
in fact. are the compilers more likely to optimize it out when we memset mem

and then free mem?

whereas they'll leave it in if we memset circ and then free mem?

ABCD> compilers so smart this days, who knows.

So, three questions to ponder.

First: should we change circ and conn to mem in these two cases, to make it
clearer which struct we're clobbering? I would say yes.

Second: should we leave circ and conn alone, in hopes that the more complex
code is more likely to fool the compiler into not optimizing the code out?
I would say yes.

Third: should we change one of the AA's into CC, to reduce the chance that
one day we see AA in memory and realize we don't know which struct we're
looking at? I would say yes.

I think my 'yes' on #1 and #2 are incompatible, and I think yes for #1 should
take priority.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (4)

comment:1 Changed 10 years ago by nickm

on #1: yes, poison "mem", not "circ" or "conn".

on #2: I've not heard of a compiler optimizing out memset in this case. The GCC strict-aliasing

optimization might mess with us here, but we explicitly disable that. If we find a compiler that
does mess this up, we can deal with it then. If you are feeling extremely paranoid, we can assert
that the fields in circ/conn are munged right after we do the memset on mem.

On #3: Yup. We shouldn't use the same nonzero poison byte for two structures.

comment:2 Changed 10 years ago by arma

Ok. I've done #1 and #3. I'm going to close this bug as resolved.

comment:3 Changed 10 years ago by arma

flyspray2trac: bug closed.

comment:4 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.