fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2#3669
Conversation
`pg`'s SCRAM-SHA-256 client passes the raw password into PBKDF2 with no normalization, while PostgreSQL's server (and libpq) apply SASLprep (B.1 mapping -> NFKC -> prohibition + bidi check) when computing the stored verifier. Passwords whose NFKC form differs from themselves (e.g. containing U+00A8 dieresis, U+2011 non-breaking hyphen, U+00BC vulgar one quarter, NBSP, soft hyphen) authenticate with psql/libpq but fail against pg with `28P01`. Wire `@mongodb-js/saslprep` (the maintained fork used by mongodb's official Node driver) into `continueSession` before `crypto.deriveKey`, with a try/catch fallback to the raw password on prohibited / bidi violations to match `libpq`'s `pg_saslprep` behavior. Also adds: - Unit tests covering the soft-hyphen B.1 mapping equivalence, the Roman-numeral-IX NFKC asymmetry, the prohibited-char fallback, and a deterministic snapshot for the original bug-report password. - A gated integration test block (SCRAM_TEST_PGUSER_UNICODE / SCRAM_TEST_PGPASSWORD_UNICODE) covering raw + NFKC-equivalent + wrong password. - A `scram_unicode_test` role (password `U&'IX-\2168'`) provisioned in CI plus matching env vars so the new integration tests run on every Node version. - A Cloudflare Workers regression guard that exercises `sasl.continueSession` to ensure `@mongodb-js/saslprep` resolves cleanly under workerd. - A `pg@8.21.0` CHANGELOG entry.
I'm not sure what you mean exactly by this, but if you're suggesting there's a way to avoid taking a 3rd party dependency into |
|
Yes, and I think dropping support for detecting/preserving perfect compatibility with prohibited passwords to simplify the implementation is a good trade: const saslprep = (s: string): string => s
.replace(/[\u00A0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u200B\u202F\u205F\u3000]/g, ' ')
.replace(/[\u00AD\u034F\u1806\u180B\u180C\u180D\u200B\u200C\u200D\u2060\uFE00\uFE01\uFE02\uFE03\uFE04\uFE05\uFE06\uFE07\uFE08\uFE09\uFE0A\uFE0B\uFE0C\uFE0D\uFE0E\uFE0F\uFEFF]/g, '')
.normalize('NFKC') |
|
also...if someone wanted could they install the saslprep library in their own code & run it? seems edge-case and not likely something that'd cause runtime errors after deployment or something. Happy to be proven wrong tho |
Per review feedback on brianc#3669: ship the SASLprep step as a small in-tree function instead of pulling a runtime dep with an unpinned transitive. The function performs only the three byte-changing steps from RFC 4013 (Table C.1.2 -> SPACE, Table B.1 -> empty, NFKC) and skips the prohibition (RFC 4013 section 2.3) and bidi (RFC 3454 section 6) checks, since libpq is forgiving on those paths and Postgres's own SASLprep is similarly lenient. Removes the try/catch fallback (no code path throws). The deterministic snapshot tests stay byte-for-byte valid because none of them touch U+200B, the only edge case where the inline impl diverges from `@mongodb-js/saslprep`. RFC 3454 places U+200B in Table B.1 (mapped to nothing); the dep maps it to SPACE. PostgreSQL's saslprep.c follows the RFC, so the inline impl matches libpq more closely on that codepoint. The B.1 unit-test rename ("passes ASCII control characters through normalization unchanged") keeps the same snapshot bytes since BEL is unchanged by all three steps. Co-authored-by: charmander <charmander@noreply.github.com>
12751f8 to
c8d2e51
Compare
now that the associated dependency isn’t being added.
normally added as part of the release process
Updated comments in sasl.js to clarify the password normalization process and removed redundant test cases from vitest-cf.test.ts, streamlining the codebase.
Confirmed in pull request comments that the “macOS/iOS” thing was an AI inventing an unneeded justification, and NFKC is already covered by another test.
As mentioned in the test comment, RFC 3454 defines appendix B for mapping tables and appendix C for prohibition tables. RFC 4013 SASLprep is probably misusing that list of non-ASCII spaces, and says nothing about the overlap. (At least it’s obsoleted.)
|
really nice work trimming this down to small, clean thing that workw! 🙌 |
* test: Ensure failure to throw at all doesn’t pass (brianc#3671) * Assorted test fixes and cleanup (brianc#3672) * cleanup: Remove duplicate test * cleanup: Remove nonsense test * cleanup: Simplify promise rejection test * test: Fix and tighten assertion that would always pass because of the `SELECTR` typo. * cleanup: Add missing `await`s when using `assert.rejects` in tests; remove unneeded function wrappers * ci: Node 26 followup (brianc#3670) * Revert unneeded pg-native→libpq dependency range adjustment This reverts part of commit 1025d12. * dev: Upgrade libpq/nan in lockfile for Node 26 compatibility * fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2 (brianc#3669) * fix: apply SASLprep (RFC 4013) to passwords before SCRAM-SHA-256 PBKDF2 `pg`'s SCRAM-SHA-256 client passes the raw password into PBKDF2 with no normalization, while PostgreSQL's server (and libpq) apply SASLprep (B.1 mapping -> NFKC -> prohibition + bidi check) when computing the stored verifier. Passwords whose NFKC form differs from themselves (e.g. containing U+00A8 dieresis, U+2011 non-breaking hyphen, U+00BC vulgar one quarter, NBSP, soft hyphen) authenticate with psql/libpq but fail against pg with `28P01`. Wire `@mongodb-js/saslprep` (the maintained fork used by mongodb's official Node driver) into `continueSession` before `crypto.deriveKey`, with a try/catch fallback to the raw password on prohibited / bidi violations to match `libpq`'s `pg_saslprep` behavior. Also adds: - Unit tests covering the soft-hyphen B.1 mapping equivalence, the Roman-numeral-IX NFKC asymmetry, the prohibited-char fallback, and a deterministic snapshot for the original bug-report password. - A gated integration test block (SCRAM_TEST_PGUSER_UNICODE / SCRAM_TEST_PGPASSWORD_UNICODE) covering raw + NFKC-equivalent + wrong password. - A `scram_unicode_test` role (password `U&'IX-\2168'`) provisioned in CI plus matching env vars so the new integration tests run on every Node version. - A Cloudflare Workers regression guard that exercises `sasl.continueSession` to ensure `@mongodb-js/saslprep` resolves cleanly under workerd. - A `pg@8.21.0` CHANGELOG entry. * fix: inline SASLprep, drop @mongodb-js/saslprep dependency Per review feedback on brianc#3669: ship the SASLprep step as a small in-tree function instead of pulling a runtime dep with an unpinned transitive. The function performs only the three byte-changing steps from RFC 4013 (Table C.1.2 -> SPACE, Table B.1 -> empty, NFKC) and skips the prohibition (RFC 4013 section 2.3) and bidi (RFC 3454 section 6) checks, since libpq is forgiving on those paths and Postgres's own SASLprep is similarly lenient. Removes the try/catch fallback (no code path throws). The deterministic snapshot tests stay byte-for-byte valid because none of them touch U+200B, the only edge case where the inline impl diverges from `@mongodb-js/saslprep`. RFC 3454 places U+200B in Table B.1 (mapped to nothing); the dep maps it to SPACE. PostgreSQL's saslprep.c follows the RFC, so the inline impl matches libpq more closely on that codepoint. The B.1 unit-test rename ("passes ASCII control characters through normalization unchanged") keeps the same snapshot bytes since BEL is unchanged by all three steps. Co-authored-by: charmander <charmander@noreply.github.com> * Revert unrelated no-op changes to yarn.lock now that the associated dependency isn’t being added. * cleanup: Allow Prettier to format some lines * cleanup: Remove changelog entry for unreleased pg version normally added as part of the release process * refactor: Simplify comments in sasl.js and remove unused test cases Updated comments in sasl.js to clarify the password normalization process and removed redundant test cases from vitest-cf.test.ts, streamlining the codebase. * Remove redundant NFKC-only SASLprep test Confirmed in pull request comments that the “macOS/iOS” thing was an AI inventing an unneeded justification, and NFKC is already covered by another test. * fix: SASLprep zero-width space the same way PostgreSQL does As mentioned in the test comment, RFC 3454 defines appendix B for mapping tables and appendix C for prohibition tables. RFC 4013 SASLprep is probably misusing that list of non-ASCII spaces, and says nothing about the overlap. (At least it’s obsoleted.) * cleanup: Simplify regex character classes with ranges --------- Co-authored-by: charmander <charmander@noreply.github.com> Co-authored-by: Charmander <~@charmander.me> --------- Co-authored-by: Charmander <~@charmander.me> Co-authored-by: charmander <charmander@noreply.github.com>
Summary
pg's SCRAM-SHA-256 client passes the raw password into PBKDF2 with no normalization, while PostgreSQL's server (andlibpq) apply SASLprep (B.1mapping →C.1.2mapping → NFKC → prohibition + bidi check) when computing the stored verifier. Passwords whose NFKC form differs from themselves authenticate againstpsql/libpqbut fail againstpgwith28P01.This bug bites real users: a password like
abcd123456789123456789¨‑¼###authenticates fine inpsqlbut fails in any consumer usingpg.Fix
Inline a minimal SASLprep helper in
packages/pg/lib/crypto/sasl.jsand call it incontinueSessionimmediately beforecrypto.deriveKey. The helper performs only the three RFC 4013 steps that change the byte content fed to PBKDF2:Prohibition (RFC 4013 §2.3) and bidi (RFC 3454 §6) checks are intentionally omitted:
libpq'spg_saslprepand Postgres's ownsaslprep.care forgiving on those paths for legacy roles, so omitting them keeps existing accounts working without adding complexity. Because the three remaining steps cannot throw on a string input, no try/catch fallback is needed.Why in-tree, no dependency
Updated in response to review feedback from @brianc and @charmander on this PR:
Why no behavior change for existing users
What changed
Commits
Happy to squash to one commit on merge if you prefer; I kept them separate so the diff in response to your feedback is easy to read.
Test plan