[PATCH] Fix SASLprep validation after NFKC normalization

Hi,

I noticed that pg_saslprep() in src/common/saslprep.c performs the prohibited code point and bidirectional checks on input_chars rather than output_chars, even though NFKC normalization has already been performed.

Since NFKC normalization can change both the code point values and the number of code points, this can lead to incorrect results. The existing comments in pg_saslprep() explicitly describe checking the output string after normalization, suggesting that the current behavior is unintentional.

I found two examples that demonstrate the problem:

1. U+0061 followed by U+0340 normalizes to U+00E0, which is valid. The current implementation returns SASLPREP_PROHIBITED instead of SASLPREP_SUCCESS.

2. U+FB1D normalizes to U+05D9 followed by U+05B4. The normalized string violates the bidirectional rules because it does not end with a RandALCat character. The current implementation incorrectly returns SASLPREP_SUCCESS.

While investigating this issue, I also verified all the code point tables used by pg_saslprep() against RFC 3454 and found no discrepancies.

After building and installing the test_saslprep module from src/test/modules/test_saslprep, the issue can be reproduced in a running Postgres instance using:

CREATE EXTENSION IF NOT EXISTS test_saslprep;

WITH cases (name, src, expected_status, expected_res) AS (
VALUES
('step3', decode('61cd80', 'hex'), 'SUCCESS', decode('c3a0', 'hex')),
('step4', decode('efac9d', 'hex'), 'PROHIBITED', NULL::bytea)
)
SELECT name, status, encode(res, 'hex') AS result,
status = expected_status AND res IS NOT DISTINCT FROM expected_res AS passed
FROM cases
CROSS JOIN LATERAL test_saslprep(src);

On the unmodified master branch, both cases return false in the "passed" column. With the patch applied, both return true.

The attached patch changes the prohibited code point and bidirectional checks to operate on the normalized output, rather than the pre-normalization input. It also rejects an empty result after normalization.

The patch includes regression tests for both cases. These tests fail on the unmodified master branch and pass with the patch applied.

Testing was performed with a clean build configured with --enable-tap-tests, --enable-cassert, and --enable-debug. All applicable check-world tests passed with PG_TEST_EXTRA=saslprep, including the additional SASLprep TAP tests and the authentication tests.

The change is intended to correct the existing SASLprep implementation according to RFC 3454 and RFC 4013.

Since Postgres falls back to the original password when SASLprep fails, this change may affect the input to SCRAM key derivation for some passwords. This could cause compatibility issues with existing SCRAM verifiers or between different Postgres versions.

If retaining the current behavior is preferred for compatibility reasons, I think it would be useful to document this explicitly in the code, since the current implementation appears to contradict the processing order specified by RFC 3454.

I would appreciate your review and thoughts on the proposed change, particularly regarding the potential compatibility implications.

Regards,
Robert I Nilsson