Skip to content

Comments

Add __msan_unpoison for X25519 assembly output buffers#30021

Open
alexey-milovidov wants to merge 1 commit intoopenssl:masterfrom
ClickHouse:msan-x25519-unpoison
Open

Add __msan_unpoison for X25519 assembly output buffers#30021
alexey-milovidov wants to merge 1 commit intoopenssl:masterfrom
ClickHouse:msan-x25519-unpoison

Conversation

@alexey-milovidov
Copy link

x25519_scalar_mult and fe_tobytes may dispatch to assembly
implementations (e.g. x25519-x86_64.s) that MemorySanitizer
cannot instrument. As a result, output buffers written by the
assembly appear uninitialized to MSan, causing false positives
when subsequent code reads them (e.g. CRYPTO_memcmp in ossl_x25519).

This adds __msan_unpoison annotations after assembly writes in
ossl_x25519 and ossl_x25519_public_from_private, following the
same pattern already used in sha3.c, bn_intern.c, and
eng_rdrand.c.

The issue was discovered while testing ClickHouse's SSH protocol
support with MSan: the libssh library calls EVP_PKEY_derive for
X25519 key exchange, and the shared secret buffer produced by the
assembly implementation triggers a false use-of-uninitialized-value
report in BN_bin2bn.

x25519_scalar_mult and fe_tobytes may dispatch to assembly
implementations (e.g. x25519-x86_64.s) that MemorySanitizer
cannot instrument. As a result, output buffers written by the
assembly appear uninitialized to MSan, causing false positives
when subsequent code reads them (e.g. CRYPTO_memcmp in ossl_x25519).

Add __msan_unpoison annotations after assembly writes in ossl_x25519
and ossl_x25519_public_from_private, following the same pattern
already used in sha3.c, bn_intern.c, and eng_rdrand.c.
@bernd-edlinger
Copy link
Member

the shared secret buffer produced by the
assembly implementation triggers a false use-of-uninitialized-value
report in BN_bin2bn

Note: this means usually you use a secret value in a non-constant time function,
which is a security issue. Do you really have to use BN_bin2bn which strips leading zero
and is therefore not constant time?

@paulidale paulidale added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug labels Feb 16, 2026
@alexey-milovidov
Copy link
Author

This usage is in libssh, I'd also doubt it, but I didn't look closely enough...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: review pending This pull request needs review by a committer branch: master Applies to master branch hold: cla required The contributor needs to submit a license agreement triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants