The Wayback Machine - https://web.archive.org/web/20220707122901/https://github.com/github/codeql/pull/9693
Skip to content
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

Jump to bottom

Token validation #9693

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Token validation #9693

wants to merge 5 commits into from
+703 −0

Conversation

raulgarciamsft
Copy link
Contributor

@raulgarciamsft raulgarciamsft commented Jun 23, 2022

New queries to address some of the major security issues discussed at azure-activedirectory-identitymodel-extensions-for-dotnet ValidatingTokens wiki
@raulgarciamsft raulgarciamsft requested a review from as a code owner Jun 23, 2022
github-code-scanning[bot]
github-code-scanning bot found potential problems Jun 27, 2022
View changes
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
|
p.getAnAccess() = this and
c.getAProperty() = p and
p.getName() in ["IssuerValidator"]

Check warning on line 207

Code scanning / CodeQL

Singleton set literal Warning

Singleton set literal can be replaced by its member.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it.
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
/**
* Abstract PropertyWrite for `TokenValidationParameters`.
* Not really necessary anymore, but keeping it in case we want to extend the queries to check on other properties.
*/

Check warning on line 7

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it.
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
/**
* Dataflow from a `false` value to an to a write sensitive property for `TokenValidationParameters`.
*/

Check warning on line 29

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it.
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
/**
* Method `ValidateToken` for `Microsoft.IdentityModel.JsonWebTokens.JsonWebTokenHandler` or other Token handler that shares the same behavior characteristics
*/

Check warning on line 59

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it.
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
/**
* Read access for properties `IsValid` or `Exception` for `Microsoft.IdentityModel.JsonWebTokens.JsonWebTokenHandler.ValidateToken`
*/

Check warning on line 82

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
/**
* Property write for security-sensitive properties for `Microsoft.IdentityModel.Tokens.TokenValidationParameters`
*/

Check warning on line 120

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
/**
* Property Write for the `IssuerValidator` property for `Microsoft.IdentityModel.Tokens.TokenValidationParameters`
*/

Check warning on line 199

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link
Contributor Author

@raulgarciamsft raulgarciamsft Jul 1, 2022

Choose a reason for hiding this comment

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

Fixing it.
tamasvajk
tamasvajk requested changes Jun 27, 2022
View changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment •
edited

Thank you for the queries. I've done a first pass review on the PR. Also, I triggered the CI jobs, which reported some more issues in QL.
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll Outdated Show resolved Hide resolved
...rp/ql/src/experimental/Security Features/JsonWebTokenHandler/security-validation-disabled.ql Outdated Show resolved Hide resolved
csharp/ql/test/experimental/Security Features/JsonWebTokenHandler/delegation-test.cs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# documentation
2 participants
@raulgarciamsft @tamasvajk