[release/10.0] Fix EncodeUnsignedInteger for test DSA signer#128005
Open
vcsjones wants to merge 1 commit intodotnet:release/10.0from
Open
[release/10.0] Fix EncodeUnsignedInteger for test DSA signer#128005vcsjones wants to merge 1 commit intodotnet:release/10.0from
vcsjones wants to merge 1 commit intodotnet:release/10.0from
Conversation
`EncodeUnsignedInteger` did not encode ASN.1 integers correctly when there was a leading zero and the second octet had its high bit set, requiring a leading zero to preserve positive sign. For example, encoding the unsigned integer `[0x00, 0xFF]` would get encoded to the ASN.1 `0201FF`, or -1. It should be encoded to `020200FF`. Conscrypt is picky about the ASN.1 encoding. This test had a ~0.46% failure chance (because it would fail if either r or s were encoded incorrectly). The first commit to this PR fixes `EncodeUnsignedInteger`. However all of the hand-rolled ASN.1 encoding is unnecessary. We can 1. Use `AsnWriter` to do ASN.1 encoding. Let `BigInteger` and `AsnWriter` do the correct encoding for us. 2. Use `DSASignatureFormat.Rfc3279DerSequence` so we don't have to convert from IEEE. Finally, this removed the ActiveIssue attribute.
vcsjones
added
area-System.Security
test-bug
Contributor
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
Contributor
There was a problem hiding this comment.
Pull request overview
Backports a test-only fix to stabilize DSA-related certificate/CRL tests on Android by avoiding hand-rolled ASN.1 integer/signature encoding that could intermittently produce invalid DER.
Changes:
- Switch DSA signatures to
DSASignatureFormat.Rfc3279DerSequenceto avoid manual IEEE-to-DER conversion. - Replace custom ASN.1 INTEGER/length encoding with
AsnWriter+BigIntegerto produce correct DER for DSA parameters and public key value.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography/tests/X509Certificates/CertificateCreation/DSAX509SignatureGenerator.cs | Removes manual ASN.1 encoding and uses AsnWriter/BigInteger plus RFC3279 DER signatures to prevent invalid DER that can fail on Android/Conscrypt. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
This was referenced May 10, 2026
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
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.
Backport of #127979 to release/10.0
Customer Impact
No customer impact. This change fixes a test bug that would cause intermittent failures of
CrlBuilderTests.DsaNotDirectlySupportedon Android.Regression
Testing
Verified test does not fail after many runs on Android.
Risk
None, test only change.