Remove insecure fallback session-ID generator; use Crypt::URandom always - #1804
Merged
cromedome merged 2 commits intoSep 15, 2026
Merged
Conversation
The default session engine previously picked between two ID-generation paths at runtime: - If both Math::Random::ISAAC::XS and Crypt::URandom were installed, IDs came from an ISAAC CSPRNG seeded with OS randomness. - Otherwise it fell back to sha1() over a hand-rolled seed made of rand(), __FILE__, a process-local counter, $$, the object string and a List::Util::shuffle of the base64url alphabet. That fallback was not cryptographically strong. rand() and shuffle both draw from the same drand48 stream, so the shuffle contributes no independent entropy; the real seed state is ~48 bits plus values an attacker can often narrow down (time, $$, a compile-time constant path). Recovering the stream makes subsequently generated session IDs predictable -- session prediction, not just fixation. Because both modules were optional, this weak path was actually the default for any install lacking them. Fixes: - generate_id() now always reads 20 bytes from the OS CSPRNG via Crypt::URandom (lib/Dancer2/Core/Role/SessionFactory.pm). The output keeps the exact same shape as before -- 4-byte big-endian timestamp + 20 bytes of entropy = 24 bytes = 32 base64url characters -- so existing sessions, validate_id(), and anything matching on ID length still work unchanged. The timestamp prefix keeps IDs roughly monotonic, which cleanup scripts rely on. - Crypt::URandom is now a hard runtime requirement (cpanfile and the generated app/tutorial cpanfiles) instead of a recommendation. Math::Random::ISAAC::XS is no longer needed and has been dropped from cpanfile and the skeleton cpanfiles. The lazy-seeding/fork-safety concerns that motivated the ISAAC closure are moot: every call fetches fresh entropy straight from the OS. Also hardens validate_id() in the same file: - Uses \A ... \z instead of ^ ... $, so IDs with a trailing newline no longer validate (previously "abc\n" passed; not directly exploitable because escape_filename encodes the newline, but wrong). - Rejects undef and any ID longer than 128 characters, so a malicious multi-megabyte cookie value is rejected cheaply instead of being stat()'d by the file session backend. POD for generate_id()/validate_id() updated to match, and t/session_object.t now asserts the new validate_id behaviour (trailing and embedded newlines, over-long IDs and undef are all rejected). Full test suite passes: 84 files, 977 tests.
…ions
The previous 128-character cap on session IDs broke Dancer2::Session::Cookie
(a third-party session engine), whose IDs are the cookie's encrypted,
serialized session data (Session::Storage::Secure: salt ~ expires ~
ciphertext ~ MAC ~ version). Even a tiny session ("{foo: bar}") produces a
134-character ID, so the cap caused every session retrieval to be rejected
and re-created -- breaking t/issues/gh-811.t on CI.
Raise the cap to 4096, the maximum size of an HTTP cookie (RFC 6265 /
browser limit). That is a natural ceiling for any legitimate cookie-based
session engine, while still rejecting the multi-megabyte values that were
the actual DoS concern (billed to stat()/fopen by the file backend).
t/session_object.t updated: a 4096-character ID now validates, 4097 is
rejected.
cromedome
approved these changes
Sep 11, 2026
cromedome
left a comment
cromedome
left a comment
Contributor
There was a problem hiding this comment.
I think it took longer for me to wrap my head around the first paragraph than it did the fix 😂
👍 from me. Bonus points for cleaning up the app skel and removing another dependency!
yanick
approved these changes
Sep 11, 2026
yanick
left a comment
yanick
left a comment
Contributor
There was a problem hiding this comment.
Looks good and straight-forward enough!
cromedome
deleted the
bigpresh/remove_fallback_insecure_session_id_generation
branch
September 15, 2026 10:52
Contributor
|
Merged, thanks! |
cromedome
added a commit
that referenced
this pull request
Sep 16, 2026
[ SECURITY ]
* PR #1836: Fix path traversal in Dancer2::Handler::File, which
served files from outside public_dir; see GHSA-6xw8-v24c-m783
(David Precious)
* PR #1836: A halting on_hook_exception handler no longer lets the
route the hook refused run anyway; see GHSA-v527-r4px-7vx7
(David Precious)
* GH #1822: Strip CR and LF from response header names, as was
already done for header values (David Precious)
* GH #1823: AutoPage no longer serves a layout as a page on
case-insensitive filesystems (David Precious)
[ BUG FIXES ]
* GH #1781: Fix directory detection heuristic (Jason A. Crome)
* GH #1784: Fix UTF-8 handling in Serializer::JSON for readonly
values (Russell @veryrusty Jenkins)
* GH #1790: Fix infinite recursion into blessed objects in JSON
Serializer (Russell @veryrusty Jenkins)
* PR #1791: Send correct error codes in send_file (Anton Lundin)
* PR #1797: Fix path()/dirname() DSL keywords dropping their first
argument (Mike Weisenborn)
* PR #1801: Fix failing CI (Jason A. Crome)
* PR #1804: Make session ID generation always use Crypt::URandom and
harden validate_id against invalid session IDs (David Precious)
* GH #1824: Encode each response content assignment on its own
merits, not just the first (David Precious)
* GH #1825: Serializer::Mutable now ignores content type parameters
such as charset when choosing a format (David Precious)
* GH #1826: uri_for_route accepts a route parameter of 0, and
refuses an empty one with a clearer message (David Precious)
* GH #1827: Hooks are compiled exactly once however many times
to_app is called (David Precious)
* GH #1828: A NUL byte in a static file request no longer warns once
per request (David Precious)
* GH #1829: dancer2 gen -g (and -r) no longer dies after writing the
application (David Precious)
* GH #1830: dancer2 gen names the application directory after the
dashed distribution name (David Precious)
* GH #1831: dancer2 gen appends a relative, matchable pattern to
MANIFEST.SKIP (David Precious)
[ ENHANCEMENTS ]
* None
[ DOCUMENTATION ]
* GH #1832: Document Serializer::Mutable's actual header precedence
in each direction (David Precious)
* GH #1833: Remove %D from the documented log_format characters; it
was never implemented (David Precious)
[ DEPRECATED ]
* PR #1821: Remove Data::Dumper serializer from Dancer2 core, along
with from_dumper/to_dumper keywords, tests (David Precious)
[ MISC ]
* GH #1834: Rename share/.gitignore so git stops applying it to this
distribution's own share/ tree (David Precious)
* GH #1835: Add a characterization test suite under t/unit,
t/integration and t/e2e (David Precious)
* With thanks to Curtis "Ovid" Poe, whose generated test suite from
PAAD for Dancer2 found the defects fixed above
pull Bot
pushed a commit
to AKJUS/Dancer2
that referenced
this pull request
Sep 16, 2026
Brings in the CI fixes, the app-root detection work, the Crypt::URandom session-ID change and the removal of the Dumper serializer from core. Two conflicts, both in files this branch also touched: Changes - both sides added entries to the same release block. Kept both; main's [ REMOVALS ] section follows our [ SECURITY ] one, and its PR PerlDancer#1804 bug-fix entry sits with the other upstream entries. Serializer/Mutable.pm - both sides rewrote overlapping parts of the POD. Our code change (normalising the header before the mapping lookup) and main's (gating Dumper behind enable_dumper) do not touch the same subs and merged cleanly; only the prose collided. Taken together rather than by side: our two-direction description of the header precedence, main's removal of Dumper from the mapping tables and its new Dumper section, and our dropped "the keys of the mapping are the content-types" clause, which the paragraph we added above it now states more precisely. t/integration/serializer/mutable.t needed a change the merge could not flag: it asserted that Accept: text/x-data-dumper serializes with Dumper, which is no longer in the default mapping. The case moves to the list of types that fall back to JSON, where it now pins something worth pinning - that an app which has not set enable_dumper does not reach the Dumper serializer, and so does not eval a request body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AYumxw4bfLvz6e4Cbjtrj8
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
3 participants
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
The default session engine (
Dancer2::Core::Role::SessionFactory) previously chose between two session-ID generation paths at runtime:Math::Random::ISAAC::XSandCrypt::URandomwere installed, IDs came from an ISAAC CSPRNG seeded with OS randomness.sha1()over a hand-rolled seed made ofrand(),__FILE__, a process-local counter,$$, the object string and aList::Util::shuffleof the base64url alphabet.That fallback was not cryptographically strong.
rand()andshuffleboth draw from the same drand48 stream, so the shuffle contributes no independent entropy; the real seed state is ~48 bits plus values an attacker can often narrow (time,$$, a compile-time constant path). Recovering the stream makes subsequently generated session IDs predictable — session prediction, not just fixation. Because both modules were optional, this weak path was in fact the default for any install lacking them.Changes
generate_id()now always reads 20 bytes from the OS CSPRNG viaCrypt::URandom(lib/Dancer2/Core/Role/SessionFactory.pm). Output keeps the exact same shape — 4-byte big-endian timestamp + 20 bytes of entropy = 24 bytes = 32 base64url characters — so existing sessions,validate_id()and anything matching on ID length work unchanged. The timestamp prefix keeps IDs roughly monotonic (used by cleanup scripts).Crypt::URandomis now a hard runtime requirement (cpanfile + generated app/tutorial cpanfiles) instead of a recommendation.Math::Random::ISAAC::XSis no longer needed and was dropped from all cpanfiles. The lazy-seeding/fork-safety concerns behind the ISAAC closure are moot: every call fetches fresh entropy straight from the OS.validate_id()in the same file:\A ... \zinstead of^ ... $, so IDs with a trailing newline no longer validate (previously"abc\n"passed).undefand IDs longer than 128 characters, so a malicious multi-megabyte cookie value is rejected cheaply instead of beingstat()'d by the file session backend.t/session_object.tasserts the newvalidate_idbehaviour (trailing/embedded newlines, over-long IDs,undef).Testing
Full test suite passes: 84 files, 977 tests.
Related security review: [Claude-Bug-Security-Sweep.md] section 5 "Session-ID fallback PRNG is not cryptographically strong".