Add Square Credentials Dynamic Validator by Bpendragon · Pull Request #515 · microsoft/sarif-pattern-matcher

Bpendragon · 2021-07-29T18:54:59Z

Adds a Dynamic Stripe Validator
Adds a DynamicStripeValidatopr_Test class as well.

Ran BuildAndTest.cmd

eddynaka · 2021-07-29T18:56:31Z

+
+            const string uri = "https://connect.squareup.com/oauth2/token";
+            const string CodeNotFoundMessage = "Authorization code not found";
+            const string CodeForRequest = "123";


order by string length

Code* starts with uppercase and uri lowercase. Make everything lowercase, pls #Closed

Fixed in iteration 3

eddynaka · 2021-07-29T18:56:51Z

+                dict.Add("grant_type", "authorization_code");
+                dict.Add("code", CodeForRequest); // Needs some value for call to work.
+                dict.Add("client_id", id);
+                dict.Add("client_secret", secret);


you can auto-initialize and order by string length #Closed

Fixed in iteration 3

eddynaka · 2021-07-29T18:57:33Z

+                    .GetAwaiter()
+                    .GetResult();
+
+                string res = response.Content.ReadAsStringAsync().Result; // Actually runs synchronously when "Result" is called.


Result

getawaiter().getresult()

also, change to content instead of res #Closed

Fixed in iteration 3

eddynaka · 2021-07-29T18:57:55Z

+                {
+                    case HttpStatusCode.OK:
+                    {
+                        return ValidationState.Authorized; // This should theoretically never be hit, a "code" of 123 is much too short. But if we do get a 200, it's a valid Credential.


add the comments before the return and break in multiple lines #Closed

Fixed in iteration 3

eddynaka · 2021-07-29T18:58:21Z

+
+                        // Credential not valid.
+                        return ValidationState.Unauthorized;
+                    }


add a new line here #Closed

Fixed in iteration 3

eddynaka · 2021-07-29T18:58:27Z

+            {
+                return ReturnUnhandledException(ref message, e);
+            }
+


remove extra new line #Closed

eddynaka · 2021-07-29T18:59:02Z

+            var keyValuePairs = new Dictionary<string, string>();
+
+            SquareCredentialsValidator.IsValidDynamic(ref fingerprint,
+                                                          ref message,


[](http://example.com/codeflow?start=0&length=58)

align with the first argument #Resolved

Resolved in iteration 4

eddynaka · 2021-07-29T18:59:40Z

+                    .GetAwaiter()
+                    .GetResult();
+
+                string res = response.Content.ReadAsStringAsync().Result; // Actually runs synchronously when "Result" is called.


res

you can move this to the unauthorized section to prevent this being used in a 404 (for example) #Closed

Fixed in iteration 3

michaelcfanning · 2021-07-29T19:27:55Z

+
+            try
+            {
+                HttpClient client = CreateHttpClient();


CreateHttpClient

This code doesn't actually create a client, does it? i.e., we implemented a caching scheme underneath the covers?

If so, this is a bad name for an API. It should be CreateOrRetrieveCachedHttpClient or something... #Pending

let's rename in a new pr. but, yes, we should rename it :)

michaelcfanning · 2021-07-29T19:30:31Z

+                        // This should theoretically never be hit, a "code" of 123 is much too short.
+                        // But if we do get a 200, it's a valid Credential.
+                        return ValidationState.Authorized;
+                    }


You need to have the courage of your comment's convictions here. If this code should never be hit, you should be throwing an exception or throwing an error condition.

Since I agree with you this code shouldn't be hit, it seems like any execution through this code path, in fact, would be a bug and not flag an actual find.

In general, it doesn't help us to code defensively. Instead, aggressively fail unexpected code paths and our test process (if we are testing properly) should flush those out. #Resolved

My specific suggestion is to use unexpected response code for this code path. Because that's what it is, right?

Makes sense, just going to remove the block entirely and we can have it fall to 'Unexpected Response Code'

michaelcfanning · 2021-07-29T19:30:54Z

+                                                 .ReadAsStringAsync()
+                                                 .GetAwaiter()
+                                                 .GetResult();
+                        if (content.Contains(codeNotFoundMessage))


codeNotFoundMessage

please add a blank vertical line before this if condition. #Closed

michaelcfanning · 2021-07-29T19:34:36Z

+                                                                Dictionary<string, string> options,
+                                                                ref ResultLevelKind resultLevelKind)
+        {
+            string id = fingerprint.Id;


fingerprint

It looks like this code doesn't return any asset information, i.e., the identifier for the compromised thing. Please see other rules for examples of proper reporting. I think 'id' is the thing we should be surfacing that we are not.

Please make sure to review actual user-facing strings returned by this change.

@eddynaka, enough is enough: we need proper unit and functional testing that forces testing through the dynamic validation piece. :)

We need to report the id for this code path but can delay getting the additional testing out there. But the very next thing we should do is implement an approach for this.

Basically, for every dynamic validator, we need a way to exercise/validate all possible code paths. This will be tricky/likely require a mocked HTTP client/other things to accomplish.

Added asset information to retrun messages.

Can you provide an example of what the user-facing string looks like for authorized/unauthorized access? I think there's another overload for some helpers that allow you to provide a descriptor of the id, when helpful. to clarify the output. So looking at current example would help understand whether we need to do something like that.

@michaelcfanning

For successfully validating the response is

The compromised asset is 'a'

For a credential that doesn't validate it's

The provided secret is not authorized to access 'a'.

In both cases 'a' is replaced with the client_id of the Square app that was detected. (which normally looks like sq0idp-<alphanumeric-identifier>

…com/microsoft/sarif-pattern-matcher into users/bpendragon/AddStripeValidator

eddynaka

Bpendragon added 2 commits July 29, 2021 11:46

Add Square Credentials Dynamic Validator

a1869c7

Rebaseline tests

f77d069

Ran BuildAndTest.cmd

Bpendragon requested review from eddynaka and michaelcfanning as code owners July 29, 2021 18:54

eddynaka reviewed Jul 29, 2021

View reviewed changes

Bpendragon added 2 commits July 29, 2021 12:21

Changes for PR

b49c1f6

Rename file to match other naming scheme.

ed461fc

michaelcfanning reviewed Jul 29, 2021

View reviewed changes

Bpendragon and others added 7 commits July 29, 2021 12:55

Remove improper return statement.

d60807f

Adding SetHttpClient so we can inject and test httpclient requests

a53bfae

Merge branch 'users/bpendragon/AddStripeValidator' of https://github.…

09bfbfd

…com/microsoft/sarif-pattern-matcher into users/bpendragon/AddStripeValidator

Adding one test

4ab34e5

Added additional moq tests for Square API

862a260

Add messaging to test results.

f328cad

Minor updates based on Intellisense

e9e8487

eddynaka reviewed Jul 30, 2021

View reviewed changes

Comment thread Src/Plugins/Tests.Security/Tests.Security.csproj Outdated

eddynaka approved these changes Jul 30, 2021

View reviewed changes

Remove unused section from CSProj file.

2e988d6

Bpendragon enabled auto-merge (squash) July 30, 2021 20:25

Bpendragon merged commit 06ff25f into main Jul 30, 2021

Bpendragon deleted the users/bpendragon/AddStripeValidator branch July 30, 2021 20:32

Conversation

Bpendragon commented Jul 29, 2021

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited by Bpendragon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddynaka Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelcfanning Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelcfanning Jul 29, 2021 • edited by Bpendragon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelcfanning Jul 29, 2021 • edited by eddynaka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelcfanning Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eddynaka Jul 29, 2021 •

edited

Loading

eddynaka Jul 29, 2021 •

edited

Loading

eddynaka Jul 29, 2021 •

edited

Loading

eddynaka Jul 29, 2021 •

edited

Loading

eddynaka Jul 29, 2021 •

edited

Loading

eddynaka Jul 29, 2021 •

edited

Loading

eddynaka Jul 29, 2021 •

edited by Bpendragon

Loading

eddynaka Jul 29, 2021 •

edited

Loading

michaelcfanning Jul 29, 2021 •

edited

Loading

michaelcfanning Jul 29, 2021 •

edited by Bpendragon

Loading

michaelcfanning Jul 29, 2021 •

edited by eddynaka

Loading

michaelcfanning Jul 29, 2021 •

edited

Loading