What the developer wrote
Authored by M1nd3r
fix(crypto): minor fixes in `noise_kk1`
- Mark `split` as static in `noise_kk1`.
- Add missing memzero to `noise_kk1`.
- Return false on encryption failure in `noise_kk1`.
- Prevent using memcpy with NULL in noise.
- Using `memcpy` to copy data from NULL buffer results in potentially undefined behavior. This raises a runtime error when using UB sanitizer.
[no changelog]
What changed, and why it matters
This commit fixes several minor issues in a cryptographic module used for secure device communication. The changes include preventing undefined behavior when copying from NULL pointers, zeroing out sensitive key material after use, and properly returning errors when encryption fails. These are defensive hardening fixes rather than a clear, exploitable vulnerability.
Treat as a hardening patch. Review whether any callers rely on the previously unchecked encrypt() behavior or pass NULL plaintext/ciphertext intentionally. Ensure the NULL-guarded memcpy paths do not silently produce incorrect ciphertext/plaintext when non-NULL buffers are expected. No urgent incident response is indicated absent additional evidence of exploitability.
Security signals we found
NULL pointer guard added around memcpy in encrypt/decrypt
Missing memzero of sensitive key material added on error paths
Return value of encrypt() now checked in handshake request handler
Function made static to limit scope
Key size constant corrected in split()
Commit message describes UB sanitizer warning and undefined behavior
Evidence from the diff
The patch modifies crypto/noise_kk1.c, which implements the Noise KK1 handshake protocol. Key changes: (1) split() is now static and uses the correct key size constant NOISE_KK1_KEY_SIZE instead of NOISE_KEY_SIZE. (2) encrypt() and decrypt() now guard memcpy with NULL checks to avoid undefined behavior detected by AddressSanitizer/UBSan. (3) noise_kk1_handle_handshake_request() now checks the return value of encrypt() and calls memzero() on kauth before returning false on failure. (4) noise_kk1_handle_handshake_response_multiple_keys() now zeroizes ephemeral_key_backup before returning false. The commit message frames these as minor fixes and explicitly notes they suppress sanitizer warnings.
Changed components
crypto/noise_kk1.cNoise KK1 handshake implementationGCM encrypt/decrypt wrappersInspect captured patch +15 / −6
diff --git a/crypto/noise_kk1.c b/crypto/noise_kk1.c
index c8191656..b7b00c1d 100644
--- a/crypto/noise_kk1.c
+++ b/crypto/noise_kk1.c
@@ -44,7 +44,9 @@ static bool encrypt(const uint8_t key[NOISE_KK1_KEY_SIZE],
return false;
}
- memcpy(ciphertext, plaintext, plaintext_length);
+ if (ciphertext != NULL && plaintext != NULL) { // to suppress asan warning
+ memcpy(ciphertext, plaintext, plaintext_length);
+ }
if (gcm_encrypt_message(nonce, NOISE_KK1_NONCE_SIZE, associated_data,
associated_data_length, ciphertext, plaintext_length,
@@ -75,7 +77,9 @@ static bool decrypt(const uint8_t key[NOISE_KK1_KEY_SIZE],
return false;
}
- memcpy(plaintext, ciphertext, plaintext_length);
+ if (plaintext != NULL && ciphertext != NULL) { // to suppress asan warning
+ memcpy(plaintext, ciphertext, plaintext_length);
+ }
if (gcm_decrypt_message(nonce, NOISE_KK1_NONCE_SIZE, associated_data,
associated_data_length, plaintext, plaintext_length,
@@ -133,8 +137,9 @@ static void mix_key(uint8_t chaining_key[SHA256_DIGEST_LENGTH],
"output_key must be truncated to NOISE_KK1_KEY_SIZE");
}
-void split(uint8_t chaining_key[SHA256_DIGEST_LENGTH],
- uint8_t output1[NOISE_KEY_SIZE], uint8_t output2[NOISE_KEY_SIZE]) {
+static void split(uint8_t chaining_key[SHA256_DIGEST_LENGTH],
+ uint8_t output1[NOISE_KK1_KEY_SIZE],
+ uint8_t output2[NOISE_KK1_KEY_SIZE]) {
// output1 || output2 =
// HKDF(salt=chaining_key, key=b"", output_length=2*NOISE_KK1_KEY_SIZE)
hkdf(chaining_key, SHA256_DIGEST_LENGTH, NULL, 0, output1, output2);
@@ -217,8 +222,11 @@ bool noise_kk1_handle_handshake_request(
memcpy(response, responder_ephemeral_public_key, sizeof(curve25519_key));
uint8_t zero_nonce[NOISE_KK1_NONCE_SIZE] = {0};
- encrypt(kauth, zero_nonce, handshake_hash, sizeof(handshake_hash), NULL, 0,
- response->tag);
+ if (!encrypt(kauth, zero_nonce, handshake_hash, sizeof(handshake_hash), NULL,
+ 0, response->tag)) {
+ memzero(kauth, sizeof(kauth));
+ return false;
+ }
memzero(kauth, sizeof(kauth));
// This is unnecessary, as the handshake hash is no longer used.
@@ -355,5 +363,6 @@ bool noise_kk1_handle_handshake_response_multiple_keys(
return true;
}
}
+ memzero(ephemeral_key_backup, sizeof(ephemeral_key_backup));
return false;
}
Why this scored 31/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.