aead is non deterministic w/ certain parameters

[kilpatty]

I'm just wrapping my head around what is going on here, so I can provide some more details in the morning with a working gist, but here is what I understand so far.

There appears to be a function inside of AEAD that is static. If I create two separate instances of CipherState or Brontide from here: https://github.com/handshake-org/hsd/blob/master/lib/net/brontide.js I believe that the function that is causing this issue is here: https://github.com/bcoin-org/bcrypto/blob/master/src/aead/aead.c#L130-L141

What I have gathered is that if a "ad" or "plaintext/ciphertext" is passed to aead encrypt/decrypt that is not a multiple of 16, then this function is called. This function changes the result of the encryption and decryption function such that it is different on an odd/even basis.

I have tested this similar functionality in a Rust library, and it seems to keep its determinism (https://github.com/HandshakeAlliance/rust-brontide/blob/master/src/cipher_state.rs#L214) - that is, if I create two separate ciphers and call encrypt they will return the same tag. If I do the same thing with the Bcrypto library, they return different tags (provided that they have parameters that are not divisible by 16).

I have a few tests written to show this which I can upload tomorrow when I wake up, but I think the easiest way to replicate is to import CipherState into this test: https://github.com/handshake-org/hsd/blob/master/test/brontide-test.js and call a single cipher.encrypt with the constant Hello before the tests on line 109. This will cause the odd/even difference to occur and the iteration test on line 109 will fail.

I suspect this is the issue causing many bad tag issues being printed in hsd logs. If one peer gets onto the "odd" track, and another on the "even" then attempting to decrypt their messages will always return false.

[chjj]

The padding is specified here: https://tools.ietf.org/html/rfc7539#section-2.8.1

I believe it's implemented properly. If there is a problem here, it probably has something to do with the lazy computation of the aad padding (the C code is something like a port of a port of a port of my original bip151 code, so it could probably use some maintenance).

Anyway, code to reproduce this would be very helpful.

[kilpatty]

Apologies for my delayed response with the test. I cleaned up the tests I was running last night and compiled them into this file here: https://github.com/kilpatty/hsd/blob/brontide-cipher-test/test/brontide-test.js#L19

I think your implementation of the padding is correct, just something is lingering around to make the next tag returned from encryption different, even if it's a different object in JS with the same parameters.

I can dive into this more tonight and see if I can figure out what is causing this exactly.

[kilpatty]

Did some more digging on this today, I couldn't find anything in your implementation of AEAD that would cause this issue.

I think what might be happening is an issue with Buffer reuse in Node - when I reuse the const buffer HELLO, this issue occurs. When I instead create two new buffers, then it works as intended.

Attempting to reprint HELLO to string after the first encryption attempt shows that it no longer prints to it's normal text. I don't think this is actually a big issue because A. I didn't notice any corruption occurring if we replace HELLO with EMPTY, and B. we aren't sharing buffers that many places in the Brontide code.

I'm going to close this now on the basis that I think it's entirely on that Buffer reuse issue. I'm not sure if that's something we should investigate further, as I think it's just isolated to tests at the moment.

[chjj]

@kilpatty, ah, I should have mentioned: all of the stream ciphers in bcrypto encrypt the buffer in-place. This isn't typical of js apis, but it's done for extra speed. This might be the cause of the issue. Maybe this feature should be documented more clearly (?).

[kilpatty]

Ah my mistake -> not sure how I didn't see that in my testing earlier. Noted for the future, happy to work on some docs for that.