floodyberry / chacha-opt

Optimized block functions for the ChaCha stream cipher
47 stars 16 forks source link

Several fixes to the reference implementation and alignment #2

Open vstakhov opened 9 years ago

vstakhov commented 9 years ago

I have found that using chacha-opt with avx2 is not possible unless chacha_state structure is aligned on 64 bytes boundary. Therefore, I have added explicit alignment for this structure that has fixed the problem. Moreover, the reference implementation leaves session key in the stack variable named j after chacha_blocks function. I suppose, cleaning is a desired behaviour in this case.

vstakhov commented 9 years ago

I have also found that chacha_final is completely broken, since memset(state, 0, sizeof(state)) occurs prior to returning of state->leftover. Optimizing compiler might fix that but it is totally incorrect anyway.

floodyberry commented 9 years ago

The state shouldn't have an alignment requirement, so it's an overlook in the avx2 (and xop) implementation. The test state should be intentionally misaligned to catch this.

The j array.. hmm. I'm not actually wiping the stack in any blocks functions, just in the one-shot functions and on final. I should fix that starting with your patch.

And oh gosh yeah, and I'm not even testing the return value for chacha_final. I'm not a big fan of the incremental api, but have no idea how else it should work (or if anyone even encrypts incrementally vs working on fixed size blocks if things get too large).

bobbradley commented 9 years ago

Just an FYI, but I use the incremental API. I'm not working with large messages, but I use it for cases where I have an HTTP header and body and I want to avoid needing to malloc a new buffer and copy into it to use the one-shot API. The incremental API lets me do this in-place.

vstakhov commented 9 years ago

I definitely observed crash in avx2_blocks unless I applied this patch http://git.io/nN-5gA to align chacha states explicitly. It didn't crash for sse/avx/reference versions.

DemiMarie commented 8 years ago

Is this problem fixed?