corrupt SecretBox ciphertext

[warner]

We're tracking down a problem in which secretbox produces ciphertexts that cannot be decrypted (with @Natim's workaround in mozilla-services/msisdn-gateway#105). This appears to happen about once out of every 80000 encryptions. I'm still tracking down the problem, but the following test program (which bypasses the SecretBox object and calls the low-level binding.crypto_secretbox directly) shows the problem:

var key = Buffer("0123456701234567012345670123456701234567012345670123456701234567", "hex");

var binding = require("sodium/build/Release/sodium");
var Nonce = require("sodium/lib/nonces/secretbox-nonce");
var SecretBoxKey = require("sodium/lib/keys/secretbox-key");
var nonce = new Nonce(Buffer("001122334455667788990011223344556677889900112233", "hex"));
var buf = Buffer("data");
var boxKey = new SecretBoxKey(key);
var expected_ct = binding.crypto_secretbox(buf, nonce.get(), boxKey.get())
      .toString("hex");
console.log("1st:", expected_ct);
for (var i = 0; i < 10*1000*1000; i++) {
  var ct = binding.crypto_secretbox(buf, nonce.get(), boxKey.get())
        .toString("hex");
  if (expected_ct !== ct) {
    console.log("got:", ct);
  }
}

crypto_secretbox is supposed to be deterministic (same key+message+nonce gives you the same output), but this program emits a couple lines of differences within the first few seconds. So far, the differences are in the poly1305 MAC portion of the output, not the encrypted data, so I suspect memory corruption or stack overflow or something funny happening in that part of the code, rather than the xsalsa keystream.

(Also, I was surprised to see that the SecretBox output includes the 16 bytes of zero padding that the NaCl C API imposes: most of the other nacl/libsodium bindings I've seen strip that out)

I'll try to trace this down more thoroughly tomorrow.

[jedisct1]

You should consider crypto_secretbox_easy() (and the _box counterpart): http://doc.libsodium.org/secret-key_cryptography/authenticated_encryption.html
http://doc.libsodium.org/public-key_cryptography/authenticated_encryption.html

It is way easier than juggling with zero padding and memcpy().

[warner]

I'm still digging, but so far I've learned that the input message inside sodium.cc's bind_crypto_secretbox() is getting corrupted. If I check the input plaintext here:

    //Copy the message to the new buffer
    memcpy((void*) (pmb_ptr + crypto_secretbox_ZEROBYTES), (void *) message, message_size);
    message_size += crypto_secretbox_ZEROBYTES;

    NEW_BUFFER_AND_PTR(ctxt, message_size);

    // <-- ADD CHECK HERE
    if( crypto_secretbox(ctxt_ptr, pmb_ptr, message_size, nonce, key) == 0) {

Then on specific iterations of my script above (always i=2285, 12915, 16956, 20981, 41321, for some reason), I see the buffer being passed into crypto_secretbox() change from:

000000000000000000000000000000000000000000000000000000000000000064617461

to:

000000000000005000000000000000500300000000000000000000000000000064617461

On other runs, the corruption happens on the same iterations, but has different bits flipped.

The glitch in the first 16 bytes causes the poly1305 key to be wrong (it flips a couple of bits in the auth key), so the emitted tag is wrong.

[warner]

Looks like the corruption is happening during the NEW_BUFFER_AND_PTR() macro: things look ok just before that call, but are sometimes corrupted just afterwards. This call uses Buffer::New() to allocate memory. The consistent-but-sorta-random recurrence of the corruption (always on those specific iterations) makes me think that the memory allocator is behaving differently during those calls, maybe it runs out of freelist entries and has to request an extra page, or GC is taking place. Memory allocators aren't supposed to corrupt random memory, so there's something deeply weird taking place here.

[warner]

If I move the NEW_BUFFER_AND_PTR(ctxt, message_size) call to earlier in the function (just after the other NEW_BUFFER_AND_PTR), then I get either a segfault or a malloc warning:

node(11795,0x7fff76d9b180) malloc: *** error for object 0x100812388: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug

which looks like a dead giveaway. I don't know enough about how Node (or webkit, or C++) does memory management to see what's wrong with bind_crypto_secretbox(), but obviously there's some use-after-free problem going on. Do Buffer objects get destructed automatically when they go out of scope?

[rvagg]

@warner are you sure you're getting the right size when you move that NEW_BUFFER_AND_PTR up? if you do that you have to NEW_BUFFER_AND_PTR(ctxt, message_size + crypto_secretbox_ZEROBYTES) to get the same behaviour

[warner]

Oh, good catch. I completely forgot that message_size changes.

With your fix, I still get the same malloc error.

[rvagg]

fixed in #20, see explanation there

[rvagg]

I'd also recommend moving to nan asap for 0.11+ support

[dannycoates]

@rvagg GOOOOOOOOOOOOOOOOOOOOOOOAL!

[warner]

sweet! thanks!