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

Fix security weakness for compilation in .NET 7 #765

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

yihezkel
Copy link
Contributor

The Rfc2898DeriveBytes constructor used is called out as insecure in NET 7, because it’s insecure to use defaults for the number of iterations and hashing algorithm. The fix is to pass those (secure) values in the versions of .NET that support it.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

….NET 7, because it’s insecure to use defaults for the number of iterations and hashing algorithm. The fix is to pass those (secure) values in the versions of .NET that support it.
@yihezkel
Copy link
Contributor Author

From https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rfc2898derivebytes.-ctor?view=net-7.0#system-security-cryptography-rfc2898derivebytes-ctor(system-string-system-int32-system-int32)

"The default hash algorithm and iteration counts in Rfc2898DeriveBytes constructors are outdated and insecure. Use a constructor that accepts the hash algorithm and the number of iterations.", DiagnosticId="SYSLIB0041", UrlFormat="https://aka.ms/dotnet-warnings/{0}"

@piksel
Copy link
Member

piksel commented Aug 10, 2022

Well, the "insecure default" value for HashAlgorithmName is SHA1, which is what we have to use for the format. So, this would not increase security in any way, but it would replace a deprecated version with a current one. And also, it's generally better to be explicit rather than relying on defaults.

@yihezkel
Copy link
Contributor Author

Exactly. The intent here is to be explicit, which is a better approach, but also satisfies .NET 7 compilation requirements :-)

If in the future there's interest to upgrade the hashing algorithm, this change should make that a tiny bit easier.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #765 (a46758f) into master (b5b1b07) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #765   +/-   ##
=======================================
  Coverage   74.10%   74.10%           
=======================================
  Files          71       71           
  Lines        8441     8441           
=======================================
  Hits         6255     6255           
  Misses       2186     2186           
Impacted Files Coverage Δ
...harpCode.SharpZipLib/Encryption/ZipAESTransform.cs 89.09% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@piksel piksel merged commit a41e066 into icsharpcode:master Aug 11, 2022
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.

2 participants