Security: SAML XSW auth bypass, reactor UAF, request-size DoS, authz + crypto hardening#5
Merged
Merged
Conversation
…panic
`validate` verified *a* signature via xmlsec1 but then read identity
from the first `<Assertion>` in document order — with no link between
the two. An attacker could keep a legitimately-signed assertion where
xmlsec1 is happy and inject a forged assertion first, which
`parseResponse` consumes: classic XML Signature Wrapping = full
authentication bypass.
`checkNoWrapping` now enforces structural invariants a minimal string
parser can guarantee: exactly one `<Assertion>` in the document, and a
`<Signature>` whose `Reference URI` is `#<AssertionID>`. Since xmlsec1
checks signatures against the IdP cert only, its success now means the
single assertion we parse is the one the IdP signed.
Also in this file:
* verifySignature used a fixed `/tmp/saml_response.xml` — two
concurrent validations clobbered each other's file (could verify
one user's response while parsing another's) and it invited a
symlink pre-creation attack. Now `IO.FS.withTempDir` (unique,
auto-removed).
* decodeSAMLResponse used `String.fromUTF8!` on attacker-controlled
bytes — a malformed sequence panicked the server (DoS). Now
`fromUTF8?` → rejected request.
Adds 7 XSW unit tests (single signed assertion accepted; two-assertion
wrapping, wrong-reference, and unsigned payloads all rejected).
Use-after-free (reactor, remotely reachable — binds INADDR_ANY): when one fd was both readable and writable in a single kevent()/epoll_wait() batch, on_readable could conn_close()+free the connection and then on_writable() dereferenced the freed pointer. conn_close now defers the free: it marks the conn `closing`, parks it on a per-reactor dead list, and reactor_reap() frees the list once per batch after every event is dispatched. Both loops skip a conn already closing. Verified: 1.3M requests under wrk -c256, no crash. Unbounded-memory DoS (all three backends): no cap on Content-Length or header accumulation, so a giant Content-Length or a client that never sends the header terminator (slowloris) grew the read buffer until OOM. Added 64 KB header / 8 MB request caps enforced in each read loop (reactor C, FastServer, libuv Server). parse_content_length also saturated to kill a signed-overflow UB. Verified: 20 MB body dropped, server survives, on both FFI backends. The Http.lean doc comment claimed a "1 MB cap" that was never implemented — corrected to the real, now-enforced limits.
Proof/AuthRoute enforce authorization on the routes you choose to wrap, but nothing stopped an app from routing /admin through a plain Handler that never mentions a Proof. The mistake isn't a wrong capability; it's a route added with no capability at all. SecureRoute forces the decision: every entry is either `.needs c` (proof enforced) or `.anyone` (explicitly public). No unstated third case, so a forgotten guard doesn't typecheck and a deliberately-open endpoint is a greppable `.anyone`. dispatchSecure routes the list.
JWT RS256 and Passkey ECDSA verification (openssl shell-out fallback,
when the native libcrypto backend is off) wrote to predictable
`/tmp/jwt_*_{nowSec}` / fixed `/tmp/passkey_*` paths: the epoch-second
suffix collides under concurrent verifies, the fixed names race
outright, and both invite symlink pre-creation (CWE-377/367); the JWT
data file also carried the payload at 0644. Both now use
IO.FS.withTempDir (unique, auto-removed). Also dropped a pointless
attacker-data write in the Passkey registration stub.
Password: raised defaultIterations 20000 → 100000 (20k was well under
OWASP's floor) and documented the native-backend path for reaching
600000 without a pure-Lean CPU cost.
* "Eight primitives" → nine (CSP / LeanTea.Net.Csp was shipped but
unlisted); added its table row.
* "one inlined JS file / Web Speech API is the only external browser
API" → the runtime JS is served separately and fetch/History/DOM
are used too; stated accurately.
* "Typed RPC (Servant-style)" → "Shared-endpoint RPC
(Servant-inspired)", with an honest note that a Handler is still
`List String → IO String` (positional, runtime-checked) and typed
`Endpoint α β` is roadmap. Removed the reference to a nonexistent
examples/Sheet/Api.lean.
* Documented SecureRoute under the authorization primitive.
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.
Security review of the current branch turned up several issues across the auth layer and the (this-session-new) FFI HTTP backends. This PR fixes them, in the reviewed priority order, and adds regression tests. All changes build clean;
security_spec82/82,pure_spec36/36, and 7 new SAML XSW tests pass.🔴 SAML XML Signature Wrapping → authentication bypass
Auth/Saml.lean—validateverified a signature withxmlsec1but then read identity from the first<Assertion>in document order, with nothing tying the two together. An attacker keeps the legitimately-signed assertion where xmlsec1 is happy and injects a forged assertion first;parseResponseconsumes the forgery → full auth bypass (the classic XSW class: Google/SAML 2012, Duo 2018).Fix:
checkNoWrappingenforces invariants a minimal string parser can guarantee — exactly one<Assertion>in the document, and a<Signature>whoseReference URIis#<AssertionID>. Since xmlsec1 checks against the IdP cert only, success now means the single assertion we parse is the one the IdP signed. 7 unit tests added (wrapping / wrong-reference / unsigned all rejected; legit single signed assertion accepted).Same file: fixed-name
/tmp/saml_response.xml→IO.FS.withTempDir(concurrent validations were clobbering each other + symlink vector);String.fromUTF8!→fromUTF8?(malformed input panicked the server = DoS).🔴 Reactor use-after-free (remotely reachable)
c/leantea_reactor.c— bindsINADDR_ANY. When one fd was both readable and writable in a singlekevent()/epoll_wait()batch,on_readablecouldconn_close()+free the connection and thenon_writable()dereferenced freed memory. Fix: deferred free —conn_closemarks the connclosingand parks it on a per-reactor dead list;reactor_reap()frees once per batch after all events dispatch; both loops skip aclosingconn. Verified 1.3M requests underwrk -c256, no crash.🔴 Unbounded-memory DoS (all three backends)
No cap on Content-Length or header accumulation → giant Content-Length or a never-terminating header stream (slowloris) grew the read buffer to OOM. The
Http.lean"1 MB cap" doc comment described a limit that was never implemented. Fix: 64 KB header / 8 MB request caps enforced in each read loop (reactor C,FastServer, libuvServer);parse_content_lengthsaturated to kill a signed-overflow UB. Verified: 20 MB body dropped, server survives, on both FFI backends. Doc corrected to the real limits.🟠 Authorization was opt-in →
SecureRouteAuth/Proof.lean—Proof/AuthRouteenforce authz on routes you choose to wrap, but a route added through a plainHandlersimply had no check. Fix:SecureRouteforces every entry to be.needs c(proof enforced) or.anyone(explicitly public) — no unstated third case, so a forgotten guard doesn't typecheck and an open endpoint is a greppable.anyone.dispatchSecureroutes the list. (Doesn't force an app to use it — a convention — but removes the silent-omission failure mode within the router.)🟠 Predictable temp files in openssl shell-out
Crypto/Jwt.lean,Auth/Passkey.lean— the RS256/ECDSA fallback path (native libcrypto backend off) wrote to/tmp/jwt_*_{nowSec}/ fixed/tmp/passkey_*: epoch-second suffix collides under concurrency, fixed names race outright, both invite symlink pre-creation (CWE-377/367); JWT data file carried the payload at 0644. Fix:IO.FS.withTempDireverywhere; dropped a pointless attacker-data write in the Passkey registration stub.🟡 PBKDF2 default too low
Crypto/Password.lean— 20 000 iterations is well under OWASP's floor. Raised to 100 000 (pure-Lean-verifiable in login-acceptable time) and documented the nativeLEANTEA_CRYPTO=1path for reaching 600 000 without a pure-Lean CPU cost.README accuracy
"Eight primitives" → nine (CSP was shipped but unlisted); "one inlined JS file / Web Speech API only" corrected (runtime JS served separately, fetch/History/DOM used); "Typed RPC (Servant-style)" → "Shared-endpoint RPC (Servant-inspired)" with an honest note that a
Handleris stillList String → IO Stringand typedEndpoint α βis roadmap; removed a reference to a nonexistentexamples/Sheet/Api.lean.Test plan
lake buildclean (176 targets)security_spec82/82pure_spec36/36 (Password/JWT)auth_specSAML group incl. 7 new XSW tests pass (the 5 failing entries are the pre-existing OAuth2-round-trip tests that need a live IdP subprocess + curl — unrelated to this change)wrk -t8 -c256, no crashNot addressed here (follow-ups)
resolveRole : Session → Capabilityremains an app-supplied trust boundary by design.