-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ClickOnce] Fix nonce generation in timestamping of signed manifest. #9579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like the current version of the NuGet code is slightly different: https://github.com/NuGet/NuGet.Client/blame/c13cea4dfd50e7eba1b794e1fea979dd1d6af3bd/src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampProvider.cs#L245-L270 Does this need similar netcore-specific logic? |
I don't think we need that logic since we only support .NET FX. That logic is for Rfc3161TimestampRequest.CreateFromHash (based on the comment) which we don't call. The .NET FX version PInvokes to CryptRetrieveTimeStamp Win32 API. |
JanKrivanek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes
#9505
Summary
SecurityUtilities.SignFile function to sign ClickOnce manifest can fail occasionally during timestamping because the random bytes generated for the nonce can be invalid DER encodings for integer values
Changes Made
To ensure the encoding is not invalid, clear the nonce MSB's most significant bit and set the MSB's least significant bit.
This is a port of the fix made for the same issue in the NuGet client:
NuGet/NuGet.Client@3e55190
Customer Impact
Customers calling SignFile API through the Microsoft.Build.Tasks.Core NuGet package encounter occasional failures because the Nonce generated is invalid per DER encoding. The fix will address this issue of the SignFile API failing intermittently.
Regression?
No
Testing
Failing scenario validated + signing scenarios in ClickOnce validated for regression.
Risk
Low