Found while a sibling project vendored this pair as its crypto baseline and ran it under the undefined-behavior sanitizer for the first time; confirmed by a cold read against the pair at fc473eb and the public headers of libsodium 1.0.18 through 1.0.22. Two annotation defects in sodium/sodium.h, opposite in direction, neither touching crypto arithmetic.
The first is a stale over-strict annotation. sodium/sodium.h declares ten functions with a bare __attribute__ ((nonnull)), which means every pointer parameter, among them crypto_onetimeauth_poly1305_update at line 529 and crypto_stream_chacha20_ietf_xor_ic at line 148. Upstream 1.0.22 declares these as nonnull(1) and nonnull(1, 4, 6) respectively, and deliberately does not declare the message or additional-data pointers non-null, because a NULL buffer with length zero is a valid call. netcode's own use trips it: netcode.c:1397 and netcode.c:1415 encrypt and decrypt the challenge token with netcode_encrypt_aead( buffer, NETCODE_CHALLENGE_TOKEN_BYTES - NETCODE_MAC_BYTES, NULL, 0, nonce, key ), additional data NULL with length zero, which reaches crypto_onetimeauth_poly1305_update(&state, ad, adlen) at sodium/sodium.c:5976 inside the detached encrypt and at :6134 on the decrypt side with a null second argument. Compiling the unmodified pair with clang -O1 -g -fsanitize=undefined -fno-sanitize-recover=all and making exactly that call aborts with "null pointer passed as argument 2, which is declared to never be null" at sodium/sodium.c:5976, citing sodium/sodium.h:532. So any sanitized build of netcode goes red on the challenge-token path, which runs on every connection. It is a false diagnostic rather than a memory-safety bug, since the callee never dereferences a zero-length buffer, though the attribute is also a license for the compiler to assume the pointer non-null and delete a later null check; there is no such check in this code today, so the practical effect is the sanitizer abort. The fix is a mechanical refresh: copy the attribute text from 1.0.22's public headers for those ten declarations. The parameter lists are identical for all ten, so the index lists transfer unchanged.
The second is a typo, and the more important of the two. sodium/sodium.h:429 declares crypto_aead_xchacha20poly1305_ietf_decrypt_detached with __attribute__ ((nonnull(3, 5, 9, 9))): parameter 9 is listed twice and parameter 8, the nonce, is missing. Upstream says nonnull(3, 5, 8, 9) in 1.0.18, 1.0.19, 1.0.20 and 1.0.22 alike, and this file's own non-X crypto_aead_chacha20poly1305_ietf_decrypt_detached carries (3, 5, 8, 9) correctly, so the typo is local to this amalgamation and not inherited from any upstream release. The effect is a silently lost check: a NULL nonce passed to the XChaCha20-Poly1305 detached decrypt is no longer diagnosed at compile time or by the sanitizer. netcode itself does not call the detached form, so nothing here misses today; it matters for yojimbo, which carries this pair byte for byte under its parity workflow, and for any other consumer. Of the two this one is worth fixing first, because a noisy false positive announces itself and a missing check on a nonce does not.
Both repairs are one regeneration of the pair from the pinned upstream headers per sodium/NOTES.md's own procedure, with the review log gaining a line; yojimbo follows by re-vendoring, as its parity check requires.
Found while a sibling project vendored this pair as its crypto baseline and ran it under the undefined-behavior sanitizer for the first time; confirmed by a cold read against the pair at fc473eb and the public headers of libsodium 1.0.18 through 1.0.22. Two annotation defects in sodium/sodium.h, opposite in direction, neither touching crypto arithmetic.
The first is a stale over-strict annotation. sodium/sodium.h declares ten functions with a bare
__attribute__ ((nonnull)), which means every pointer parameter, among themcrypto_onetimeauth_poly1305_updateat line 529 andcrypto_stream_chacha20_ietf_xor_icat line 148. Upstream 1.0.22 declares these asnonnull(1)andnonnull(1, 4, 6)respectively, and deliberately does not declare the message or additional-data pointers non-null, because a NULL buffer with length zero is a valid call. netcode's own use trips it: netcode.c:1397 and netcode.c:1415 encrypt and decrypt the challenge token withnetcode_encrypt_aead( buffer, NETCODE_CHALLENGE_TOKEN_BYTES - NETCODE_MAC_BYTES, NULL, 0, nonce, key ), additional data NULL with length zero, which reachescrypto_onetimeauth_poly1305_update(&state, ad, adlen)at sodium/sodium.c:5976 inside the detached encrypt and at :6134 on the decrypt side with a null second argument. Compiling the unmodified pair withclang -O1 -g -fsanitize=undefined -fno-sanitize-recover=alland making exactly that call aborts with "null pointer passed as argument 2, which is declared to never be null" at sodium/sodium.c:5976, citing sodium/sodium.h:532. So any sanitized build of netcode goes red on the challenge-token path, which runs on every connection. It is a false diagnostic rather than a memory-safety bug, since the callee never dereferences a zero-length buffer, though the attribute is also a license for the compiler to assume the pointer non-null and delete a later null check; there is no such check in this code today, so the practical effect is the sanitizer abort. The fix is a mechanical refresh: copy the attribute text from 1.0.22's public headers for those ten declarations. The parameter lists are identical for all ten, so the index lists transfer unchanged.The second is a typo, and the more important of the two. sodium/sodium.h:429 declares
crypto_aead_xchacha20poly1305_ietf_decrypt_detachedwith__attribute__ ((nonnull(3, 5, 9, 9))): parameter 9 is listed twice and parameter 8, the nonce, is missing. Upstream saysnonnull(3, 5, 8, 9)in 1.0.18, 1.0.19, 1.0.20 and 1.0.22 alike, and this file's own non-Xcrypto_aead_chacha20poly1305_ietf_decrypt_detachedcarries(3, 5, 8, 9)correctly, so the typo is local to this amalgamation and not inherited from any upstream release. The effect is a silently lost check: a NULL nonce passed to the XChaCha20-Poly1305 detached decrypt is no longer diagnosed at compile time or by the sanitizer. netcode itself does not call the detached form, so nothing here misses today; it matters for yojimbo, which carries this pair byte for byte under its parity workflow, and for any other consumer. Of the two this one is worth fixing first, because a noisy false positive announces itself and a missing check on a nonce does not.Both repairs are one regeneration of the pair from the pinned upstream headers per sodium/NOTES.md's own procedure, with the review log gaining a line; yojimbo follows by re-vendoring, as its parity check requires.