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

Update the crypto rules strings and help links #360

Merged
merged 9 commits into from
Nov 23, 2015

Conversation

qinxgit
Copy link
Contributor

@qinxgit qinxgit commented Nov 20, 2015

@genlu @srivatsn
This is the pull request to update the crypto rules' (CA5350, CA5351) message strings so that all algorithms have a same user-facing string pattern.

And also updated help link of CA2153.
See https://microsoft.sharepoint.com/teams/CESecEngineering/SecDevTools/_layouts/15/WopiFrame2.aspx?sourcedoc={3b7b7554-5f75-4db3-8a52-fa37d9a3713b}&action=edit&wd=target%28Crypto%2Eone%7C9ACEC449-841F-4EF2-A460-F4A5E6760B69%2F%29

@@ -118,48 +118,36 @@
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="DoNotCatchCorruptedStateExceptions" xml:space="preserve">
<value>Do not catch corrupted state exceptions in general handlers.</value>
<value>Do Not Catch Corrupted State Exceptions</value>
</data>
<data name="DoNotCatchCorruptedStateExceptionsDescription" xml:space="preserve">
<value>Do not author general catch handlers in code that receives corrupted state exceptions. Code that receives and intends to handle corrupted state exceptions should author distinct handlers for each exception type.</value>
</data>
<data name="DoNotCatchCorruptedStateExceptionsMessage" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

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

Messages should be small - they need to be able to show up in a single line. You can put a lot of this detail in the description.I would change the message to be simply "{0} is catching a corrupted state exception.".
The description can be: "Catching corrupted state exceptions could mask errors (such as access violations), resulting in inconsistent state of execution or making it easier for attackers to compromise system. Instead, catch and handle a more specific set of exception type(s) or re-throw the exception."

This way the most relevant information Is shown and the user can expand the error in the error list to see the details.


cur = pNode;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only find a method GetEnclosingSymbol(), but it doesn't work for

await Task.Run(()=>new RC2CryptoServiceProvider());

The name of the symbol is empty here, which makes the message looks stupid.
I still need to have a while to walk up the tree. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I'll think about this but for now we can leave this as is.

@qinxgit
Copy link
Contributor Author

qinxgit commented Nov 20, 2015

@srivatsn addressed your comments with one 1 left. See changes.

<data name="DoNotUseWeakCryptographicAlgorithmsDescription" xml:space="preserve">
<value>Cryptographic algorithms degrade over time as attacks become for advances to attacker get access to more computation. Depending on the type and application of this cryptographic algorithm, further degradation of the cryptographic strength of it may allow attackers to read enciphered messages, tamper with enciphered  messages, forge digital signatures, tamper with hashed content, or otherwise compromise any cryptosystem based on this algorithm.

HOW: Replace encryption uses with the AES algorithm (AES-256, AES-192 and AES-128 are acceptable) with a key length greater than or equal to 128 bits. Replace hashing uses with a hashing function in the SHA-2 family, such as SHA-2 512, SHA-2 384, or SHA-2 256.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

We agreed to drop the word "HOW:" in caps right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. Let me remove it.

s_localizableDoNotUseMD5Description);
s_localizableDoNotUseBrokenAlgorithmsTitle,
s_localizableDoNotUseBrokenAlgorithmsMessage,
s_localizableDoNotUseBrokenAlgorithmsDescription);

internal static DiagnosticDescriptor DoNotUseDESSpecificRule = CreateDiagnosticDescriptor(DoNotUseBrokenCryptographicRuleId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have all these descriptions if they have the same content? Why not just one descriptor?

@qinxgit
Copy link
Contributor Author

qinxgit commented Nov 23, 2015

@srivatsn For last 2 comments, they are fixed.

@srivatsn
Copy link
Contributor

👍 Thanks, I'll merge this is in now.

srivatsn added a commit that referenced this pull request Nov 23, 2015
Update the crypto rules strings and help links
@srivatsn srivatsn merged commit 5f363f5 into dotnet:master Nov 23, 2015
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.

3 participants