Skip to content

Reducing unhandled exceptions for the certificate rules#544

Merged
eddynaka merged 7 commits into
mainfrom
users/ednakamu/improving-cert-handling
Aug 26, 2021
Merged

Reducing unhandled exceptions for the certificate rules#544
eddynaka merged 7 commits into
mainfrom
users/ednakamu/improving-cert-handling

Conversation

@eddynaka

@eddynaka eddynaka commented Aug 25, 2021

Copy link
Copy Markdown
Collaborator

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • ReleaseHistory.md updated for non-trivial changes
  • Added unit tests


$SEC101/026.MailgunApiCredentialsId=(?is)(?:[^0-9a-z]|^)(?P<id>[0-9a-z]+?)\.mailgun\.org(?:[^0-9a-z]|$)
$SEC101/026.MailgunApiCredentialsSecret=(?is)(?:[^0-9a-z]|^)(?P<secret>[0-9a-z]{32}-[0-9a-z]{8}-[0-9a-z]{8})(?:[^0-9a-z]|$)
$SEC101/026.MailgunApiCredentialsId=(?si)(?:[^0-9a-z]|^)(?P<id>[0-9a-z]+?)\.mailgun\.org(?:[^0-9a-z]|$)

@eddynaka eddynaka Aug 25, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

si

normalizing to "si" #ByDesign

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for making this consistent! nice.

$SEC101/013/Putty.CryptographicPrivateKey=(?is)PuTTY-User-Key-File-2.+?Private-Lines:\s*[0-9]+\s*(?P<secret>.+?)Private-MAC:\s[0-9a-z]+(?:[^0-9a-d]|$)
$SEC101/013/Pem.CryptographicPrivateKey=(?si)-{5}BEGIN (?:DSA|EC|OPENSSH|PGP|RSA|SSH2 ENCRYPTED)?\s*PRIVATE (?:KEY BLOCK|KEY)-{5}.*?(?:VERSION: [^\n]+\n)*\n?(?P<secret>[^:]*?)-{5}END (?:DSA|EC|OPENSSH|PGP|RSA|SSH2 ENCRYPTED)?\s*PRIVATE (?:KEY BLOCK|KEY)-{5}
$SEC101/013/Putty.CryptographicPrivateKey=(?si)PuTTY-User-Key-File-2.+?Private-Lines:\s*[0-9]+\s*(?P<secret>.+?)Private-MAC:\s[0-9a-z]+(?:[^0-9a-d]|$)
$SEC101/013/Pem.CryptographicPrivateKey=(?si)-{5}BEGIN (?:DSA|EC|OPENSSH|PGP|RSA|SSH2 ENCRYPTED)?\s*PRIVATE (?:KEY BLOCK|KEY)-{5}.*?(?:(?:VERSION|Proc-Type|DEK-Info): [^\n]+\n)*\n?(?P<secret>[^:]*?)-{5}END (?:DSA|EC|OPENSSH|PGP|RSA|SSH2 ENCRYPTED)?\s*PRIVATE (?:KEY BLOCK|KEY)-{5}

@eddynaka eddynaka Aug 25, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Proc-Type|DEK-Info

there are certificates that also show proc-type and dek-info. We should skip those just like version. #ByDesign

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this will prevent an error when we try to convert to byte[]

{
try
{
byte[] bytes = Convert.FromBase64String(content.Value);

@eddynaka eddynaka Aug 25, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

content.Value

if we receive a content.value which isn't a string, it's probably a base64. We will try to process it. #ByDesign

Comment thread Src/ReleaseHistory.md Outdated

- Plugin Improvement: Required properties will throw exception if they do not exist. [#539](https://github.com/microsoft/sarif-pattern-matcher/pull/539)
- Tool Improvement: Tool will emit fixes with comprehensive region properties. [#540](https://github.com/microsoft/sarif-pattern-matcher/pull/540)
- Rule Improvement: Improving certificate handling. [#544](https://github.com/microsoft/sarif-pattern-matcher/pull/544)

@michaelcfanning michaelcfanning Aug 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rule Improvement

Eddy, can we please start documenting changes by their actual impact at analysis time?

Does this change eliminate unhandled exceptions? Increase results (i.e., eliminates false negatives), decrease false positives?

This is the data we need in a change log. 'Rule improvement' doesn't tell us anything about how analysis is impacted. We use the change log to help explain deltas between test runs. #Closed

ref message,
ref resultLevelKind);
}
catch (Exception)

@michaelcfanning michaelcfanning Aug 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exception

Please don't every swallow general exceptions like this. Instead, find a more precise exception type. Do we have some showing up in our telemetry? #Closed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

changed to get formatexception that would generate a nomatch.

@michaelcfanning michaelcfanning left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🕐

@eddynaka eddynaka changed the title Improving certificate handling Reducing unhandled exceptions for the certificate rules Aug 25, 2021
@eddynaka eddynaka merged commit 136d470 into main Aug 26, 2021
@eddynaka eddynaka deleted the users/ednakamu/improving-cert-handling branch August 26, 2021 21:03
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