[bug bounty] The expanded AES key is leaked when a write to the output file fails #429

Summary

scryptenc_file() and scryptdec_file_copy() both leak the expanded AES key
if a write to the output file fails partway through the stream. The
crypto_aesctr stream is freed on that path, but the
struct crypto_aes_key * it was built from is not.

This is not a security issue, so I am reporting it here rather than by email,
per https://www.tarsnap.com/bugbounty.html.

The two sites

Both functions expand the encryption key, use it, and free it on the way out:

	if ((key_enc_exp = crypto_aes_key_expand(key_enc, 32)) == NULL) {
		rc = SCRYPT_EOPENSSL;
		goto err1;
	}
	if ((AES = crypto_aesctr_init(key_enc_exp, 0)) == NULL) {
		crypto_aes_key_free(key_enc_exp);	/* freed here */
		rc = SCRYPT_ENOMEM;
		goto err1;
	}

but the write-error exit inside the loop frees only the stream:

lib/scryptenc/scryptenc.c, in scryptenc_file():

		if (fwrite(buf, 1, readlen, outfile) < readlen) {
			crypto_aesctr_free(AES);
			rc = SCRYPT_EWRFILE;
			goto err1;
		}

and in scryptdec_file_copy():

		if (fwrite(buf, 1, buflen - 32, outfile) < buflen - 32) {
			crypto_aesctr_free(AES);
			rc = SCRYPT_EWRFILE;
			goto err0;
		}

err1 in scryptenc_file() only does insecure_memzero(dk, 64), and err0
in scryptdec_file_copy() only does return (rc). Neither calls
crypto_aes_key_free(), so key_enc_exp — the object holding the expansion of
the 256-bit encryption key — is never handed back to the function responsible
for disposing of it.

The _buf variants are not affected: scryptenc_buf() and scryptdec_buf()
have no fwrite(), so their only exits after the expansion already free it.

Reproducer

On Linux, /dev/full accepts an open() and then fails every write with
ENOSPC. The input has to be bigger than stdio's buffer so that the failure
lands inside the loop rather than at the final flush:

$ yes | head -c 1000000 > big.txt
$ echo hunter2 | scrypt enc -f --logN 10 -r 1 -p 1 \
      --passphrase dev:stdin-once big.txt /dev/full
scrypt: Error writing file: /dev/full: No space left on device
$ echo $?
1

Under valgrind --leak-check=full that run reaches the goto err1 above with
key_enc_exp still allocated. The same holds for the decrypt side:

$ echo hunter2 | scrypt enc -f --logN 10 -r 1 -p 1 \
      --passphrase dev:stdin-once big.txt big.enc
$ echo hunter2 | scrypt dec -f --passphrase dev:stdin-once big.enc /dev/full
scrypt: Error writing file: /dev/full: No space left on device

scrypt exits shortly afterwards, so the practical cost is small — but the
write-error path is the one path in these two loops that has no test coverage
at all, which is why make test USE_VALGRIND=1 does not currently notice.

Fix

PR to follow: add the missing crypto_aes_key_free(key_enc_exp) to both
write-error paths, matching the crypto_aesctr_init() failure path immediately
above each of them. It also adds tests/13-write-error.sh, which drives both
loops into a failing write via /dev/full (and skips itself where /dev/full
does not exist), so the project's existing valgrind CI covers these paths.

How this was found

Source review of lib/scryptenc/scryptenc.c at a71ae82, pairing every
crypto_aes_key_expand() against the frees on each exit from its scope.