aes: ensure output buffer is valid, share validation code, add tests
What the developer wrote
Authored by Jon Griffiths
aes: ensure output buffer is valid, share validation code, add tests
Reported-by: Jordan Mecom (@jmecom) via https://github.com/jmecom/odo
What changed, and why it matters
This commit tightens input validation in libwally-core's AES encryption helpers. It makes sure callers pass a valid output buffer and length, and it centralizes the checks so all AES functions behave consistently. The change also adds tests for bad inputs such as missing keys, missing IVs, and NULL output buffers. It appears to fix a bug where a caller could pass an invalid output buffer and not get a clear error.
Treat as a security-hardening fix with possible denial-of-service or undefined-behavior implications. Review whether prior releases allowed callers to trigger out-of-bounds writes or information leaks through the unvalidated output buffer. Apply the patch and run the new test suite. Consider issuing an advisory if the old behavior is exploitable beyond a simple API misuse error.
Security signals we found
Missing or invalid output-buffer validation in cryptographic API
Centralized argument validation to reduce inconsistent checks
New test coverage for NULL/empty key, IV, input, and output buffer cases
Reported-by line crediting external reporter with associated disclosure URL
Evidence from the diff
The patch refactors src/aes.c so that argument validation is shared via are_valid_args and a new are_valid_aes_cbc_args helper. Key changes: (1) are_valid_args now requires a non-NULL written pointer and uses BYTES_INVALID to reject invalid buffer/length combinations; (2) wally_aes and wally_aes_cbc now reject NULL or zero-length output buffers up front; (3) wally_aes_cbc_get_maximum_length accepts a NULL output buffer (since it only computes a size) by passing NULL/0 to the shared validator; (4) wally_aes_cbc no longer silently accepts a NULL bytes_out when len is non-zero. The Python tests exercise these invalid-argument paths. The commit message and Reported-by line credit Jordan Mecom via an external project, suggesting this was a third-party report.
Changed components
src/aes.cwally_aeswally_aes_lenwally_aes_cbcwally_aes_cbc_get_maximum_lengthInspect captured patch +83 / −35
diff --git a/src/aes.c b/src/aes.c
index 3a66042..f4ea03e 100644
--- a/src/aes.c
+++ b/src/aes.c
@@ -5,8 +5,6 @@
#include "ctaes/ctaes.h"
#include "ctaes/ctaes.c"
-#define ALL_OPS (AES_FLAG_ENCRYPT | AES_FLAG_DECRYPT)
-
static bool is_valid_key_len(size_t key_len)
{
return key_len == AES_KEY_LEN_128 || key_len == AES_KEY_LEN_192 ||
@@ -14,11 +12,15 @@ static bool is_valid_key_len(size_t key_len)
}
static bool are_valid_args(const unsigned char *key, size_t key_len,
- const unsigned char *bytes, size_t bytes_len, uint32_t flags)
+ const unsigned char *bytes, size_t bytes_len,
+ uint32_t flags, size_t *written)
{
- return key && is_valid_key_len(key_len) &&
- (bytes != NULL || (bytes == NULL && bytes_len == 0 && (flags & AES_FLAG_ENCRYPT))) &&
- (flags == AES_FLAG_ENCRYPT || flags == AES_FLAG_DECRYPT);
+ if (written)
+ *written = 0;
+ if (!key || !is_valid_key_len(key_len) || BYTES_INVALID(bytes, bytes_len) ||
+ (flags != AES_FLAG_ENCRYPT && flags != AES_FLAG_DECRYPT) || !written)
+ return false;
+ return true;
}
static void aes_enc(AES256_ctx *ctx,
@@ -75,10 +77,8 @@ int wally_aes_len(const unsigned char *key, size_t key_len,
const unsigned char *bytes, size_t bytes_len,
uint32_t flags, size_t *written)
{
- if (written)
- *written = 0;
- if (!are_valid_args(key, key_len, bytes, bytes_len, flags) ||
- !bytes_len || bytes_len % AES_BLOCK_LEN || !written)
+ if (!are_valid_args(key, key_len, bytes, bytes_len, flags, written) ||
+ !bytes_len || bytes_len % AES_BLOCK_LEN)
return WALLY_EINVAL;
*written = bytes_len;
return WALLY_OK;
@@ -90,8 +90,9 @@ int wally_aes(const unsigned char *key, size_t key_len,
unsigned char *bytes_out, size_t len)
{
AES256_ctx ctx;
+ size_t written;
- if (!are_valid_args(key, key_len, bytes, bytes_len, flags) ||
+ if (!are_valid_args(key, key_len, bytes, bytes_len, flags, &written) ||
len % AES_BLOCK_LEN || !bytes_len || bytes_len % AES_BLOCK_LEN ||
!bytes_out || !len)
return WALLY_EINVAL;
@@ -105,43 +106,52 @@ int wally_aes(const unsigned char *key, size_t key_len,
return WALLY_OK;
}
+static bool are_valid_aes_cbc_args(const unsigned char *key, size_t key_len,
+ const unsigned char *iv, size_t iv_len,
+ const unsigned char *bytes, size_t bytes_len,
+ uint32_t flags,
+ unsigned char *bytes_out, size_t len,
+ size_t *written)
+{
+ if (!are_valid_args(key, key_len, bytes, bytes_len, flags, written) ||
+ !iv || iv_len != AES_BLOCK_LEN || BYTES_INVALID(bytes_out, len))
+ return false;
+
+ if (flags & AES_FLAG_ENCRYPT) {
+ if (len % AES_BLOCK_LEN)
+ return false; /* Output must be a block length multiple if given */
+ } else {
+ if (bytes_len % AES_BLOCK_LEN)
+ return false; /* Input must be a block length multiple if given */
+ }
+ return true;
+}
+
int wally_aes_cbc_get_maximum_length(const unsigned char *key, size_t key_len,
const unsigned char *iv, size_t iv_len,
const unsigned char *bytes, size_t bytes_len,
uint32_t flags,
size_t *written)
{
- if (written)
- *written = 0;
-
- if (!are_valid_args(key, key_len, bytes, bytes_len, flags) ||
- ((flags & AES_FLAG_DECRYPT) && (bytes_len % AES_BLOCK_LEN)) ||
- !iv || iv_len != AES_BLOCK_LEN || !written)
+ if (!are_valid_aes_cbc_args(key, key_len, iv, iv_len, bytes, bytes_len,
+ flags, NULL, 0, written))
return WALLY_EINVAL;
-
*written = ((bytes_len / AES_BLOCK_LEN) + 1) * AES_BLOCK_LEN;
return WALLY_OK;
}
int wally_aes_cbc(const unsigned char *key, size_t key_len,
const unsigned char *iv, size_t iv_len,
- const unsigned char *bytes, size_t bytes_len,
- uint32_t flags,
- unsigned char *bytes_out, size_t len,
- size_t *written)
+ const unsigned char *bytes, size_t bytes_len, uint32_t flags,
+ unsigned char *bytes_out, size_t len, size_t *written)
{
unsigned char buf[AES_BLOCK_LEN];
AES256_ctx ctx;
size_t i, n, blocks;
unsigned char remainder;
- if (written)
- *written = 0;
-
- if (!are_valid_args(key, key_len, bytes, bytes_len, flags) ||
- ((flags & AES_FLAG_ENCRYPT) && (len % AES_BLOCK_LEN)) ||
- ((flags & AES_FLAG_DECRYPT) && (bytes_len % AES_BLOCK_LEN)) ||
- !iv || iv_len != AES_BLOCK_LEN || !written)
+ if (!are_valid_aes_cbc_args(key, key_len, iv, iv_len, bytes, bytes_len,
+ flags, bytes_out, len, written))
return WALLY_EINVAL;
blocks = bytes_len / AES_BLOCK_LEN;
@@ -173,6 +183,7 @@ int wally_aes_cbc(const unsigned char *key, size_t key_len,
goto finish; /* Inform caller how much space is needed */
if (!bytes_out) {
+ *written = 0;
wally_clear_2(buf, sizeof(buf), &ctx, sizeof(ctx));
return WALLY_EINVAL;
}
diff --git a/src/test/test_aes.py b/src/test/test_aes.py
index ff09b5a..577c4c4 100755
--- a/src/test/test_aes.py
+++ b/src/test/test_aes.py
@@ -80,17 +80,54 @@ class AESTests(unittest.TestCase):
return [lines[x:x+4] for x in range(0, len(lines), 4)]
def test_aes_cbc(self):
+ out_buf, out_len = make_cbuffer('00' * 80)
+ E, D = self.ENCRYPT, self.DECRYPT
+ # Encryption/decryption cases
for c in self.get_cbc_cases():
plain, key, iv, cypher = [make_cbuffer(s)[0] for s in c]
- for p, f, o in [(plain, self.ENCRYPT, cypher),
- (cypher, self.DECRYPT, plain)]:
-
- out_buf, out_len = make_cbuffer('00' * len(o))
+ for p, f, o in [(plain, E, cypher), (cypher, D, plain)]:
ret, written = wally_aes_cbc(key, len(key), iv, len(iv),
- p, len(p), f, out_buf, out_len)
+ p or None, len(p), f, out_buf, out_len)
self.assertEqual((ret, written), (0, len(o)))
- self.assertEqual(h(out_buf), h(o))
+ self.assertEqual(h(out_buf[:written]), h(o))
+ # Passing a NULL output buffer with zero length returns the
+ # number of bytes required for encrypted/decrypted output
+ ret, written = wally_aes_cbc(key, len(key), iv, len(iv),
+ p or None, len(p), f, None, 0)
+ self.assertEqual((ret, written), (0, len(o)))
+ # wally_aes_cbc_get_maximum_length returns tha maximum
+ # number of bytes required.
+ ret, max_len = wally_aes_cbc_get_maximum_length(key, len(key), iv, len(iv),
+ p or None, len(p), f)
+ self.assertEqual(ret, 0)
+ self.assertTrue(max_len >= written and max_len % 16 == 0)
+
+ # Invalid args
+ invalid_cases = [
+ # NULL key
+ (None, len(key), iv, len(iv), cypher, len(cypher), D, out_buf, out_len),
+ # Empty key
+ (key, 0, iv, len(iv), cypher, len(cypher), D, out_buf, out_len),
+ # NULL IV
+ (key, len(key), None, len(iv), cypher, len(cypher), D, out_buf, out_len),
+ # Empty IV
+ (key, len(key), iv, 0, cypher, len(cypher), D, out_buf, out_len),
+ # NULL cyphertext
+ (key, len(key), iv, len(iv), None, len(cypher), D, out_buf, out_len),
+ # Empty cyphertext
+ (key, len(key), iv, len(iv), cypher, 0, D, out_buf, out_len),
+ # Invalid flags
+ (key, len(key), iv, len(iv), cypher, len(cypher), 3, out_buf, out_len),
+ # NULL out_buf
+ (key, len(key), iv, len(iv), cypher, len(cypher), D, None, out_len),
+ ]
+ for c in invalid_cases:
+ if c[-1] == out_len and c[-2] == out_buf:
+ ret, written = wally_aes_cbc_get_maximum_length(*c[:-2])
+ self.assertEqual((ret, written), (WALLY_EINVAL, 0))
+ ret, written = wally_aes_cbc(*c)
+ self.assertEqual((ret, written), (WALLY_EINVAL, 0))
def test_aes_cbc_with_ecdh_key(self):
ENCRYPT, DECRYPT, _ = 1, 2, True
Why this scored 46/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.