-
Notifications
You must be signed in to change notification settings - Fork 465
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
Conversation
@@ -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"> |
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.
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; | ||
} | ||
|
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.
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?
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.
I see. I'll think about this but for now we can leave this as is.
@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> |
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.
We agreed to drop the word "HOW:" in caps right?
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.
Ah, yes. Let me remove it.
s_localizableDoNotUseMD5Description); | ||
s_localizableDoNotUseBrokenAlgorithmsTitle, | ||
s_localizableDoNotUseBrokenAlgorithmsMessage, | ||
s_localizableDoNotUseBrokenAlgorithmsDescription); | ||
|
||
internal static DiagnosticDescriptor DoNotUseDESSpecificRule = CreateDiagnosticDescriptor(DoNotUseBrokenCryptographicRuleId, |
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.
Why do we have all these descriptions if they have the same content? Why not just one descriptor?
@srivatsn For last 2 comments, they are fixed. |
👍 Thanks, I'll merge this is in now. |
Update the crypto rules strings and help links
@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