fix: correct three latent error-path bugs#233
Open
perfgao wants to merge 6 commits into
Open
Conversation
- x509/store.lua: anchor chain_dup across X509_verify_cert to prevent use-after-free (chain_dup was block-local; its ffi_gc finalizer could release the stack while X509_STORE_CTX still referenced it). - pkey.lua: use builtin tostring (not the file-local __tostring serializer) and preserve the original padding value in the "invalid padding" error message. - bn.lua: fix copy-paste mistakes in error reporting — mod_sqr now tags check_args as "mod_sqr" (was "mod_sub"), and __shr reports "BN_rshift() failed" (was "BN_lshift() failed"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- pkey.t: assert pkey:encrypt(s, "bad_pad") returns the preserved "invalid padding: bad_pad" message (regression for the __tostring shadowing bug). - bn.t: assert bn.mod_sqr with a non-bignum argument reports the "mod_sqr" op name in its error (regression for the mod_sub copy-paste). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes several bugs across the OpenSSL Lua library, including correcting argument check names in bn.lua, fixing a variable name typo (type to typ) and padding handling in pkey.lua, and hoisting chain_dup in store.lua to prevent premature garbage collection. The review comments identify two important improvements: ensuring chain_dup is not aggressively garbage collected by LuaJIT before X509_verify_cert executes, and correcting a typo ("explictly") in an error message in pkey.lua.
…y_cert Hoisting chain_dup to function scope is not enough: LuaJIT collects locals at last-read, not end-of-scope. Since chain_dup's last read would otherwise be chain_dup.ctx at init time, its ffi_gc finalizer (OPENSSL_sk_pop_free) could still fire before X509_verify_cert runs. Add an explicit `local _ = chain_dup` read after X509_verify_cert to extend its liveness past the verify call. Per-review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Multi-line anchored regex didn't match Test::Nginx's response body framing. Use a simple substring match for "mod_sqr" instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
|
Thank you @perfgao I'm undergoing a CI refresh, will get back to this PR once that's ready. |
There was a problem hiding this comment.
Pull request overview
This PR fixes several latent error-path issues and a GC/lifetime hazard in the OpenSSL bindings, improving both correctness and the quality of error reporting in edge cases.
Changes:
- Fix potential GC-related use-after-free in
x509.store:verify()when passing an untrusted chain intoX509_STORE_CTX_init(). - Correct RSA padding error handling to preserve the user-supplied padding value and avoid calling the wrong
__tostring. - Fix copy/paste mistakes in BN error reporting and add regression tests for the updated error messages.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| t/openssl/pkey.t | Adds regression coverage for the “invalid padding” error message preserving user input. |
| t/openssl/bn.t | Adds regression coverage that mod_sqr error messages name the correct operation. |
| lib/resty/openssl/x509/store.lua | Adjusts untrusted-chain handling to avoid premature GC/free while verification is running. |
| lib/resty/openssl/pkey.lua | Fixes JWK type check typo and corrects RSA padding error-path logic/message. |
| lib/resty/openssl/bn.lua | Fixes incorrect operation tagging and wrong function name in rshift failure error text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
216
to
+220
| local code = C.X509_verify_cert(ctx) | ||
| -- LuaJIT collects locals at last-read, not end-of-scope. The read below | ||
| -- extends chain_dup's liveness past X509_verify_cert so its ffi_gc | ||
| -- finalizer (OPENSSL_sk_pop_free) can't free the stack the ctx points at. | ||
| local _ = chain_dup |
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.
Summary
chain_dupacrossX509_verify_certto avoid a use-after-free. The wrapper was block-local; itsffi_gc(OPENSSL_sk_pop_free)finalizer could fire betweenX509_STORE_CTX_initandX509_verify_cert, freeing the stack while the ctx still held the raw pointer.__tostring(self, is_priv, fmt, is_pkcs1)serializer instead of builtintostring, and stringified the already-nilledpadding. Now uses builtintostringand preserves the original value, sopkey:encrypt(s, 'bad_pad')returnsinvalid padding: bad_padinstead ofattempt to index a string value.mod_sqrtaggedcheck_argsas"mod_sub", and__shrreportedBN_lshift() failedon rshift failure. Both only fire on error paths, so CI happy-path didn't catch them.Tests
t/openssl/pkey.t— assertspubkey:encrypt(s, "bad_pad")returnsinvalid padding: bad_pad.t/openssl/bn.t— assertsbn.mod_sqr(a, "not-a-bn", 5)error message names themod_sqrop.🤖 Generated with Claude Code