Skip to content
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

6732: Default to sha2 digest for clickonce manifest #6882

Merged
merged 3 commits into from
Sep 24, 2021

Conversation

sujitnayak
Copy link
Contributor

@sujitnayak sujitnayak commented Sep 22, 2021

Default to sha2 digest instead of sha1 for clickonce manifest signing when certificate signing algorithm is sha256/384/512

Fixes #6732

Context

When using a certificate signed with sha384/sha512 as signature algorithm, ClickOnce defaults to the sha1 algorithm for it's digest hash.

Changes Made

UseSha256Algorithm decides if we sign with sha1 or sha256 hash. The function has been updated to use sha256 hash when signature algorithm of the signing cert has sha256/sha384/sha512 signature algorithm.

Testing

CTI has tested signing scenario for forms and wpf apps for all 4 hash types.

Notes

@John-Hart John-Hart requested a review from ning51 September 22, 2021 23:10
@sujitnayak sujitnayak changed the base branch from main to vs17.0 September 23, 2021 00:43
@sujitnayak sujitnayak changed the base branch from vs17.0 to main September 23, 2021 00:51
@@ -572,7 +572,11 @@ public static void SignFile(string certPath, SecureString certPassword, Uri time
private static bool UseSha256Algorithm(X509Certificate2 cert)
{
Oid oid = cert.SignatureAlgorithm;
return string.Equals(oid.FriendlyName, "sha256RSA", StringComparison.OrdinalIgnoreCase);
// Issue 6732: Clickonce does support sha384/sha512 hash so we default to sha256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Issue 6732: Clickonce does support sha384/sha512 hash so we default to sha256
// Issue 6732: Clickonce does not support sha384/sha512 hash so we default to sha256

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this question makes sense but: Is it possible for a cert to have sha384/sha512 and not have 256?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the signature algorithm of the cert to decide what file digest algorithm to use for the clickonce manifest files. So for certs with sha1 signature, we choose sha1 file digest algorithm for clickonce manfest. With this change, we will use sha256 file digest algorithm for clickonce manifest if the signature algorithm in the cert is sha256 or sha384 or sha512.

The cert's signature algorithm is the algorithm used to create the signature of the cert and can be only 1 value like sha1/sha256/sha384/sha512.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so it's an independent signature/algorithm but it makes sense to use the strongest possible one based on the signal of the cert provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

@sujitnayak sujitnayak merged commit c144bfc into dotnet:main Sep 24, 2021
ClaytonJohnsonCashies added a commit to ClaytonJohnsonCashies/deployment-tools that referenced this pull request Jul 7, 2022
- Updated the Microsoft.Build.Tasks.Core nuget package to version 17.0.0 so that
  it includes this fix dotnet/msbuild#6882
ClaytonJohnsonCashies added a commit to ClaytonJohnsonCashies/deployment-tools that referenced this pull request Jul 7, 2022
- Updated the Microsoft.Build.Tasks.Core nuget package to version 17.0.0 so that
  it includes this fix dotnet/msbuild#6882
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.

SecurityUtilities chooses sha1 when signing using certificate signed with sha384
3 participants