Description

When calling aes_gcm_encrypt with a plaintext buffer whose length is less than the AES block size (i.e. less than 16 bytes), a bug (whose cause and fix is described below) causes OTCRYPTO_RECOV_ERR to be returned when the call should return OTCRYPTO_SUCCESS. In the case of aes_gcm_decrypt this bug does not cause OTCRYPTO_RECOV_ERR to be returned, but rather the call succeeds yet the plaintext results buffer remains unwritten. The root cause is the same.

Cause

Internally, when aes_gcm_gctr is called, it exits when there is not enough data to encrypt a full block, and the output_len output parameter's value is not set. output_len is only set when there is at least one full block, which as you will see is why this bug only occurs when there is not a full block.

if (input_len < kAesBlockNumBytes - partial_len) {
  // Not enough data for a full block; copy into the partial block.
  unsigned char *partial_bytes = (unsigned char *)partial->data;
  memcpy(partial_bytes + partial_len, input, input_len);
} else {
  ...
  *output_len = kAesBlockNumBytes;
  ...

aes_gcm_update_encrypted_data calls aes_gcm_gctr and then uses the value pointed to by output_len:

// Process any full blocks of input with GCTR to generate more ciphertext.
size_t partial_aes_block_len = ctx->input_len % kAesBlockNumBytes;
HARDENED_TRY(aes_gcm_gctr(ctx->key, &ctx->gctr_iv, partial_aes_block_len,
                          &ctx->partial_aes_block, input_len, input,
                          output_len, output));

// Accumulate any new ciphertext to the GHASH context. The ciphertext is the
// output for encryption, and the input for decryption.
if (ctx->is_encrypt == kHardenedBoolTrue) {
  // Since we only generate ciphertext in full-block increments, no partial
  // blocks are possible in this case.
  if (*output_len % kGhashBlockNumBytes != 0) {
    return OTCRYPTO_RECOV_ERR;
  }

...

And aes_gcm_encrypt calls aes_gcm_update_encrypted_data like this:

size_t ciphertext_bytes_written;
HARDENED_TRY(aes_gcm_update_encrypted_data(
    &ctx, plaintext_len, plaintext, &ciphertext_bytes_written, ciphertext));

...

Therefore, if plaintext_len is less than 16, *output_len yields whatever uninitialized stack memory is occupied by ciphertext_bytes_written because it is never set in aes_gcm_gctr, and the evaluation of *output_len % kGhashBlockNumBytes != 0 results in non-deterministic undefined behavior.

Solution

Option 1

aes_gcm_gctr should set output_len:

if (input_len < kAesBlockNumBytes - partial_len) {
  // Not enough data for a full block; copy into the partial block.
  unsigned char *partial_bytes = (unsigned char *)partial->data;
  memcpy(partial_bytes + partial_len, input, input_len);
   *output_len = 0; // this fixes this bug
}

...

Option 2

If Option 1 breaks the current expected functionality of aes_gcm_gctr (existing tests still work but I might be missing something) another solution would be to simply initialize ciphertext_bytes_written to 0 in aes_gcm_encrypt and plaintext_bytes_written to 0 in aes_gcm_decrypt.

PR

Let me know which of these two options (or another option entirely) would be preferable to implement, and I'll make a PR for it.