Skip to content

Url detection#611

Merged
michaelcfanning merged 12 commits into
mainfrom
url-detection
Apr 25, 2022
Merged

Url detection#611
michaelcfanning merged 12 commits into
mainfrom
url-detection

Conversation

@michaelcfanning

Copy link
Copy Markdown
Member

Changes

Add a simple URL analysis that's currently constrained to a small subset of code files. We could consider expanding this and adding a dynamic validator (that can do things like provide a broken link detection).

@suvamM

"MessageArguments": { "dataKind": "social security number" }
},
{
"Id": "SEC102/003",

@michaelcfanning michaelcfanning Apr 20, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Id

Note that this rule picks up some default information from the root location, such as the level of "note" #Closed

{
"Id": "SEC102/003",
"Name": "ReviewPotentiallySensitiveData/Url",
"FileNameAllowRegex": "(?i)(\\.cs|\\.cpp|\\.ini|\\.js|\\.ps1)$",

@michaelcfanning michaelcfanning Apr 20, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FileNameAllowRegex

This extensions list is a start to use when we run our investigative analysis. It's not clear this is what we'll want moving forward. Can discuss with you offline. #Closed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@michaelcfanning Is this comment with respect to the URL analysis or the certificate analysis?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

URL

Comment thread .gitignore
# SPAM configuration
.spam/
*.sarif No newline at end of file
.sarif/ No newline at end of file

@michaelcfanning michaelcfanning Apr 20, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sarif

The previous entry ignored newly added test files! Clearly not write. I think it was intended to ignore everything in the .sarif\ directory... #Closed

"artifacts": [
{
"location": {
"uri": "src/Plugins/Tests.Security/TestData/ReviewPotentiallySensitiveData/Inputs/SEC102_001.EmailAddress_with_email_addresses.txt",

@michaelcfanning michaelcfanning Apr 20, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

EmailAddress_with_email_addresses

Why did someone think suffixing this file with 'with_email_addresses' provided more information? I don't know. Seems redundant to me so I trimmed it. :) #Closed

{
"ValidatorsAssemblyName": "Security.dll",
"SharedStringsFileName": "Security.SharedStrings.txt",

@eddynaka eddynaka Apr 21, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: remove this empty line #Resolved

@eddynaka

eddynaka commented Apr 21, 2022

Copy link
Copy Markdown
Collaborator
        string candidateAddress = $"{id}.Value@{host?.Value}";

this looks strange.
I think it should be {id.Value}


In reply to: 1105351716


Refers to: Src/Plugins/Security/SEC102_001.EmailAddressValidator.cs:22 in 35b0fc7. [](commit_id = 35b0fc7, deletion_comment = False)

@@ -0,0 +1,24 @@
{

@eddynaka eddynaka Apr 21, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

something is wrong. we should see results in the SARIF #Closed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wrong extension on test file! Good catch, thank you.

@suvamM

suvamM commented Apr 22, 2022

Copy link
Copy Markdown
Collaborator

@michaelcfanning @eddynaka I am trying to understand this: I see the test files in place (expected inputs and outputs), but I don't see any files which drive the tests to use this new test data. Will the existing testing setup pick these up automatically?


In reply to: 1106972516

@michaelcfanning

Copy link
Copy Markdown
Member Author

I'd neglected to git add a file, sorry!


In reply to: 1106972516

@michaelcfanning michaelcfanning enabled auto-merge (squash) April 25, 2022 21:19
@eddynaka

Copy link
Copy Markdown
Collaborator

{

you can remove this entire file


Refers to: Src/Plugins/Tests.Security/TestData/ReviewPotentiallySensitiveData/ExpectedOutputs/SEC102_001.EmailAddress_with_email_addresses.sarif:1 in b9278ea. [](commit_id = b9278ea, deletion_comment = False)

@eddynaka

eddynaka commented Apr 25, 2022

Copy link
Copy Markdown
Collaborator

{

you can remove this entire file


In reply to: 1109053602


Refers to: Src/Plugins/Tests.Security/TestData/ReviewPotentiallySensitiveData/ExpectedOutputs/SEC102_002.SocialSecurityNumber_with_social_security_numbers.sarif:1 in b9278ea. [](commit_id = b9278ea, deletion_comment = False)

https://msn.com/test
https://msn.com/test/
https://msn.com/test?foo=bar
https://msn.com/test?foo=bar#bookmark No newline at end of file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the tests, I cannot see this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

that's by design, this is a bookmark, i.e., the fragment doesn't change the finding.

@michaelcfanning

Copy link
Copy Markdown
Member Author

{

hm, the baselining gesture in my non-clean build env re-added them, thanks.


In reply to: 1109053602


Refers to: Src/Plugins/Tests.Security/TestData/ReviewPotentiallySensitiveData/ExpectedOutputs/SEC102_002.SocialSecurityNumber_with_social_security_numbers.sarif:1 in b9278ea. [](commit_id = b9278ea, deletion_comment = False)

@eddynaka eddynaka left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:shipit:

@eddynaka eddynaka deleted the url-detection branch April 28, 2022 21:59
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