fix: Keep Data pointers alive across C crypto calls - #396
Merged
Conversation
Aes128Util and KeyUtil took raw pointers via `(data as NSData).bytes`. The bridged NSData is a temporary owned by nothing past the statement, so the pointer is only valid by accident: on Darwin the autorelease pool holds the object until the pool drains, and on Linux there is no pool, so the buffer is freed immediately. In Aes128Util the key pointer was captured and then read after further allocations (the IV bridge, the context, the output buffer) had a chance to reuse the freed memory, which is why AES-128-CTR returned wrong bytes against the NIST SP 800-38A vector on Linux and KeystoreUtil produced unreadable keystores on top of it. KeyUtil had the same construct for the private key and message pointers. It has not been observed to misbehave because those pointers are consumed immediately, but it is the same undefined behaviour over private key material, so fix it in the same pass. Take the pointers with withUnsafeBytes so their lifetime is bounded by the scope that uses them, matching what recoverPublicKey already did. Aes128Util now also holds AES_ctx as a zero-initialised value rather than uninitialised heap memory, and encrypts a copy of the input in place instead of hand-copying through a raw buffer. Fixes #395
Aes128Util only had the NIST SP 800-38A vector, which passes whole Data values with an IV. Add the paths the fix touches: slices, whose non-zero start index has to be honoured; a nil IV, which used to read whatever was in the freshly allocated context and is now a zero IV; and a round trip that also checks the input buffer is left alone. Expected values come from `openssl enc -aes-128-ctr`, which reproduces the existing NIST expectation exactly, so it is a real oracle rather than our own output. KeyUtil had no coverage of its failure paths, and adding it turned up an out-of-bounds read: secp256k1 reads a fixed 32 bytes through the pointer, so a shorter private key or unhashed message was read past the end of the buffer instead of being rejected, and a longer one was silently truncated. recoverPublicKey already guarded its message length; generatePublicKey and sign never did. Guard both. testEncryptAndStorePrivateKey and its multiple-key twin passed 256 random bytes as a private key and passed only because the first 32 were taken silently. Give them a real key.
DarthMike
marked this pull request as ready for review
August 11, 2026 08:50
benjamin-es-hall
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #395.
Root cause
Not the
aesdependency — the wrapper.Aes128UtilandKeyUtiltook raw pointers like this:The bridged
NSDatais a temporary that nothing owns past the end of the statement. The pointer is therefore only valid by accident: on Darwin the autorelease pool keeps the object alive until the pool drains, so it works; swift-corelibs-foundation has no autorelease pool, so the buffer is released immediately and the pointer dangles.In
Aes128Utilthe key pointer was captured and then read after three further allocations (the IV bridge,AES_ctx, the output buffer) had a chance to reuse the freed memory. That is why AES-128-CTR returns wrong bytes against the NIST SP 800-38A vector on Linux, and whyKeystoreUtil— which encrypts and decrypts through it — writes keystores that cannot be read back.Two other call sites corroborate the diagnosis:
KeccakExtensionsstores the bridge in a localletbefore using.bytes, andKeyUtil.recoverPublicKeyalready useswithUnsafeBytes. Both pass on Linux. Only the temporary-bridge form fails.Commits
1.
fix: Keep Data pointers alive across C crypto callsAes128Util.xcrypttakes key, IV and input pointers insidewithUnsafeBytesscopes.AES_ctxis now a zero-initialised value instead of uninitialised heap memory, and the input is encrypted in place on a copy rather than hand-copied through a raw buffer.KeyUtil.generatePublicKeyandKeyUtil.signget the same treatment. Neither had been observed to misbehave — their pointers are consumed immediately with nothing allocated in between — but it is the same undefined behaviour applied to private key material.KeccakExtensionsis left alone: it retains the bridge in a local, so it is not UB. Worth tidying separately.2.
test: Cover the AES and secp256k1 buffer handlingAes128Utilonly had the NIST vector, which passes wholeDatavalues with an IV — none of the paths the fix touches. Added: sliced inputs (a non-zero start index has to be honoured), a nil IV (previously read whatever was in the freshly allocated context, now a zero IV), and a round trip that also checks the caller's buffer is left alone. Expected values come fromopenssl enc -aes-128-ctr, which reproduces the existing NIST expectation exactly, so it is an independent oracle rather than our own output.KeyUtilhad no coverage of its failure paths, and writing it turned up a pre-existing out-of-bounds read. secp256k1 reads a fixed 32 bytes through the pointer, so a shorter private key or unhashed message was read past the end of the buffer instead of being rejected, and a longer one was silently truncated.recoverPublicKeyalready guarded its message length;generatePublicKeyandsignnever did. Both now guard.encryptAndStorePrivateKeywith a key that is not 32 bytes now throwsprivateKeyInvalidinstead of silently deriving an address from the first 32 bytes.testEncryptAndStorePrivateKeyand its multiple-key twin were passing 256 random bytes as a private key and only passed because of that truncation, so their fixtures are fixed here too. This is stricter than #395 strictly requires — happy to split it into its own PR if you would rather keep this one to the Linux fix.3.
tech: Block on the Linux job now that it passesReverts
continue-on-error: trueontest_linux, per the Status section of #395. Note this makes Linux blocking for every PR, and it takes ~9 minutes against macOS's ~30 seconds.Testing
EthereumAccountTests, 18 suites reachedThe fix commit was re-run three times before the Linux job was made blocking, to check it was not flaky. Results read from the job logs rather than the check status, since
continue-on-errorreports success either way.KeystoreUtilTestsis the coverage that matters most here and it already existed — it encodes and decodes the canonical Web3 Secret Storage vector, so it checks interoperability rather than just self-consistency. It had simply never run on Linux (see below).🤖 Generated with Claude Code