Skip to content

Fix native ChaCha nonce corruption during nested writes - #1523

Open
lzfxxx wants to merge 1 commit into
mscdex:masterfrom
lzfxxx:fix/chacha-native-reentrant-nonce
Open

lzfxxx wants to merge 1 commit into
mscdex:masterfrom
lzfxxx:fix/chacha-native-reentrant-nonce

Conversation

@lzfxxx

@lzfxxx lzfxxx commented Oct 13, 2026

Copy link
Copy Markdown

Fix native ChaCha20-Poly1305 nonce corruption during nested writes

Target repository: mscdex/ssh2. Draft for upstream review.
Baseline tested: ssh2 1.17.0, Node 22.20.0, macOS, optional native binding absent.
Updated 2026-10-12: master HEAD a3a39d7
still contains the pattern. GitHub connector reads verified the SHA and searched
open/closed issues and PRs. No equivalent fix was found within the documented
search coverage; see upstream-search.md for limits. Highest tag is v1.17.0;
GitHub Releases is empty, and npm registry publication state was not checked.

Summary

 ChaChaPolyCipherNative.encrypt(packet)
+  allocate a nonce buffer scoped to this invocation
   encrypt packet length
   invoke onWrite(length)
   encrypt payload using the same invocation's nonce

The native implementation uses a module-scoped mutable nonce buffer.
The synchronous onWrite callback can encrypt through another cipher, as
in nested SSH forwarding. That nested call changes the shared sequence
number before the original cipher encrypts its payload. The packet length
is valid, but the peer decrypts an invalid payload.

Use a zero-initialized 16-byte buffer scoped to encrypt(). This isolates
the nonce across different cipher instances in nested callbacks without
changing cipher negotiation or SSH access policy.

Evidence

  • Before: unmodified 1.17.0 passes standalone payload validation but fails
    nested service-request payload validation; regression exits 1.
  • After: the two-line patch passes standalone and nested payload validation;
    regression exits 0.
  • Fixtures use synthetic keys and sequence numbers. No network or credentials.
  • The original diagnostic fixture validates payload and packet length.
    The integrated test/test-protocol-chacha-reentrancy.js forces the native
    cipher and decipher, validates authenticated decryption and exact payload,
    and covers standalone/nested writes at sequence numbers 0, 3 and 0xFFFFFFFF.
    It fails on unpatched HEAD and passes all six cases with the patch.
  • The complete upstream node test/test.js entry point was rerun on
    2026-10-13 using Node 22.20.0 on macOS with normal approval for localhost
    and test sockets. It exited 1: 12 test files passed, including the native
    six-case regression and upstream crypto; three failed. The OpenSSH 10.3 RSA
    integration test reported client exit 255; server-hostkeys and SFTP tests
    reported localhost AggregateError ETIMEDOUT. These failures have not been
    diagnosed or confirmed against an unpatched baseline in this execution.
    One unsupported OpenSSL cipher case was skipped. This PR does not claim
    the full suite passes.
  • Runtime and test subtrees were verified by Git tree hash against HEAD before
    patching. Existing local dependencies were copied; no software was installed.
    Compiled optional binding and Windows/Linux were not tested. Lint was not run
    because ESLint and the upstream config package are unavailable locally.

Merge Danger

Door: two-way. Revert the invocation-local allocation to undo the change.

Blast Radius: cryptography.

One small buffer allocation per packet on this native path. Benchmarking and
upstream review are appropriate. Optional compiled binding and other ciphers
are unchanged. This does not claim to make same-instance recursive encryption
or all module-scoped scratch storage generally reentrant.

@lzfxxx
lzfxxx marked this pull request as ready for review October 13, 2026 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant