BUG: Fix missing error code to error description mappings for CryptoError
#969
Conversation
…ecognized crypto HRESULT: 0x80096011` for check `BA2022.SignSecurely` when the signature is malformed, by adding missing error code to error description mappings.
| @@ -130,10 +130,11 @@ private static string CreateLibraryKey(ObjectModuleDetails objectModuleDetail) | |||
|
|
|||
| private static readonly Dictionary<CryptoError, string> s_cryptoErrorToDescriptionMap = BuildCryptoErrorDescriptions(); | |||
|
|
|||
| private static Dictionary<CryptoError, string> BuildCryptoErrorDescriptions() | |||
| public static Dictionary<CryptoError, string> BuildCryptoErrorDescriptions() | |||
There was a problem hiding this comment.
I don't know how nitty we are about class visibility modifiers, but I think making it internal and using 'InternalsVisibleTo' to specify that the test assembly can access would be ideal. If code signing is used, this requires sticking the public key in a class attribute. If there's no code signing then it's trivial.
My take: if you haven't taken a crack at this yet, I'd encourage giving it a shot. But not a hill to die on if you run into a hard time figuring out the signing stuff or just can't afford to spend a little bit of time trying to make it work.
src/BinSkim.Rules/CryptoErrors.cs
Outdated
| @@ -10,6 +10,8 @@ namespace Microsoft.CodeAnalysis.IL.Rules | |||
| /// </summary> | |||
| public enum CryptoError : uint | |||
| { | |||
| // Whenever this file is refreshed, | |||
There was a problem hiding this comment.
Nit: suggest "modified" instead of "refreshed." #Resolved
| Dictionary<CryptoError, string> dictionaryValues = RulesExtensionMethods.BuildCryptoErrorDescriptions(); | ||
| var missingValues = enumValues.Where(value => !dictionaryValues.ContainsKey(value)).ToList(); | ||
| missingValues.Should().HaveCount(0, "BuildCryptoErrorDescriptions() should contain all cases from CryptoError enum."); | ||
| } |
| { CryptoError.CERTSRV_E_ADMIN_DENIED_REQUEST, "The request was denied by a certificate manager or CA administrator." }, | ||
| { CryptoError.CERTSRV_E_ALIGNMENT_FAULT, "A memory reference caused a data alignment fault." }, | ||
| { CryptoError.CERTSRV_E_ARCHIVED_KEY_REQUIRED, "The request is missing a required private key for archival by the server." }, | ||
| { CryptoError.CERTSRV_E_ARCHIVED_KEY_UNEXPECTED, "The request includes a private key for archival by the server, but key archival is not enabled for the specified certificate template." }, |
There was a problem hiding this comment.
"for archiving by the server" scans a little better for me, but none of my public school English teachers were as excited about teaching grammar as discussing literature, so I couldn't explain exactly why. I might be in error. :) #Resolved
There was a problem hiding this comment.
Fixed, all the texts were from Windows SDK, your proposed change sounds good to me.
There was a problem hiding this comment.
Oh man, if I'd known you were using strings that were already public-facing I wouldn't have said anything. Thanks though!
| { CryptoError.CERTSRV_E_SUBJECT_ALT_NAME_REQUIRED, "The request is missing a required Subject Alternate name extension." }, | ||
| { CryptoError.CERTSRV_E_SUBJECT_DIRECTORY_GUID_REQUIRED, "The Active Directory GUID is unavailable and cannot be added to the Subject Alternate name." }, | ||
| { CryptoError.CERTSRV_E_SUBJECT_DNS_REQUIRED, "The DNS name is unavailable and cannot be added to the Subject Alternate name." }, | ||
| { CryptoError.CERTSRV_E_SUBJECT_EMAIL_REQUIRED, "The EMail name is unavailable and cannot be added to the Subject or Subject Alternate name." }, |
|
@michaelcfanning I think ready to merge. |
BUG: Fix
ERR998.ExceptionInAnalyze:InvalidOperationException: Unrecognized crypto HRESULT: 0x80096011for checkBA2022.SignSecurelywhen the signature is malformed, by adding missing error code to error description mappings.We have a mapping that is from the error code to description,
RulesExtensionMethods.BuildCryptoErrorDescriptions().Whenever
CryptoErroris refreshed,this mapping must be updated accordingly.