Skip to content

Conversation

tdykstra
Copy link
Contributor

@dotnet-bot dotnet-bot added this to the July 2020 milestone Jul 18, 2020
@tdykstra tdykstra changed the title WIP: Replace MD5 and SHA1 Replace MD5 and SHA1 Jul 22, 2020
@tdykstra tdykstra marked this pull request as ready for review July 22, 2020 23:20
@tdykstra tdykstra requested a review from bartonjs July 22, 2020 23:21
@tdykstra tdykstra requested review from bartonjs and removed request for bartonjs August 3, 2020 18:03
// Set the hash algoritm to 'SHA1'.
myAssemblyName->HashAlgorithm = AssemblyHashAlgorithm::SHA1;
// Set the hash algorithm to 'SHA256'.
myAssemblyName->HashAlgorithm = AssemblyHashAlgorithm::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 don't know that SHA256 actually works here.

// Set the hash algoritm to 'SHA1'.
myAssemblyName.HashAlgorithm = AssemblyHashAlgorithm.SHA1;
// Set the hash algorithm to 'SHA256'.
myAssemblyName.HashAlgorithm = AssemblyHashAlgorithm.SHA256;
Copy link
Member

Choose a reason for hiding this comment

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

Another triplet of AssemblyName.HashAlgorithm being updated. It might work, I just know that some parts of this system are effectively limited to MD5 and SHA1... and I don't know if this is one of them or not. (We could, of course, update them, and if we get feedback it doesn't work, then change them back)

Byte[] updHash = Sha1.ComputeHash(Encoding.UTF8.GetBytes("username" + "password" + "domain"));
SHA256 Sha256 = SHA256.Create();
Byte[] updHash = Sha256.ComputeHash(Encoding.UTF8.GetBytes("username" + "password" + "domain"));
String secureGroupName = Encoding.Default.GetString(updHash);
Copy link
Member

Choose a reason for hiding this comment

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

Not really the problem at hand, but: I don't know that this is going to do anything useful. The hash may contain a zero-byte, which will make the string effectively terminate early.

It's also weird that the "secure group name" is a hash of static data. Might want to get networking to review this sample to see what it's supposed to be doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for HttpWebRequest, which has an Important alert box warning that "We don't recommend that you use HttpWebRequest for new development. Instead, use the System.Net.Http.HttpClient class." The example is for ConnectionGroupName property. It may not be worth someone's time to investigate and I should just revert the changes on these files(?)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, reverting the edit and changing it to the "don't use SHA1" comment may be the short-term best.

@tdykstra tdykstra merged commit 0ab1ab3 into dotnet:master Aug 3, 2020
@eiriktsarpalis eiriktsarpalis added area-System.Runtime area-System.Security Issues related to security practices for .NET developers. and removed area-System.Runtime labels Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Security Issues related to security practices for .NET developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

**MD5 and SHA1 Hash algorithm replacement**
4 participants