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

[0x04e] Add check for JWT Claim #1939

Merged
merged 6 commits into from
Oct 27, 2021
Merged

Conversation

Saket-taneja
Copy link
Contributor

Reference LInk : https://bugsbunnyy1107.medium.com/is-your-jwt-secure-edef27f304fc

Thank you for submitting a Pull Request to the Mobile Security Testing Guide. Please make sure that:

  • Your contribution is written in the 2nd person (e.g. you)
  • Your contribution is written in an active present form for as much as possible.
  • You have made sure that the reference section is up to date (e.g. please add sources you have used, make sure that the references to MITRE/MASVS/etc. are up to date)
  • Your contribution has proper formatted markdown and/or code
  • Any references to website have been formatted as [TEXT](URL “NAME”)
  • You verified/tested the effectiveness of your contribution (e.g.: is the code really an effective remediation? Please verify it works!)

If your PR is related to an issue. Please end your PR test with the following line:
This PR closes #< insert number here >.

@Saket-taneja
Copy link
Contributor Author

Saket-taneja commented Oct 4, 2021

@cpholguera Can you please tell why this has failed ?

@cpholguera
Copy link
Collaborator

Hi @Saket-taneja, markdown-link-check gives the following output:

Document/0x04e-Testing-Authentication-and-Session-Management.md: 431: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
Document/0x04e-Testing-Authentication-and-Session-Management.md: 435: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
Document/0x04e-Testing-Authentication-and-Session-Management.md: 446: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
Document/0x04e-Testing-Authentication-and-Session-Management.md: 458: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
Document/0x04e-Testing-Authentication-and-Session-Management.md: 477: MD005/list-indent Inconsistent indentation for list items at the same level [Expected: 0; Actual: 1]

Please go thought your changes and fix those spots. In order to avoid this in the future we strongly recommend using a markdown linter on your IDE.

You can ignore the link checker for now, it is not completely reliable. But the markdown check pass is mandatory. Thank you in advance!

@cpholguera
Copy link
Collaborator

Thanks for the fixes, there are two more:

Document/0x04e-Testing-Authentication-and-Session-Management.md: 435: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
Document/0x04e-Testing-Authentication-and-Session-Management.md: 475: MD005/list-indent Inconsistent indentation for list items at the same level [Expected: 0; Actual: 1]

@cpholguera cpholguera changed the title Update 0x04e-Testing-Authentication-and-Session-Management.md 0x04e - About KID Misconfigurations Oct 9, 2021
@Saket-taneja
Copy link
Contributor Author

Hey @cpholguera Updated the fixes , Can you check the link problem

@Saket-taneja
Copy link
Contributor Author

@cpholguera Hey Man i have not added this link https://securitylearn.net/wp-content/uploads/tools/iOS/BinaryCookieReader.py which is causing the problem, Please check if this is not related anywhere we can then remove it so it won't cause problems for us

@cpholguera
Copy link
Collaborator

Hi @Saket-taneja, please take a look at the comment I sent you as part of your other PR: #1937 (comment)

You can safely ignore that one but please focus on the LINT CHECK. In this case it's fine 👍

@TheDauntless
Copy link
Collaborator

Thanks for contributing to the project, @Saket-taneja !

Unfortunately, I don't think we should include this in the MSTG for a few reasons:

  • With the refactor of the MASVS (and thus MSTG), one of the main goals is to remove the overlap between the MASVS and the ASVS. In practice, this means that multiple controls from chapter 4 (auth & session management) will probably removed.
  • The main angle of this PR is about the injection part, which is very specific and too in-depth for what we want in the MSTG.
  • This is covered under MSTG-PLATFORM-2, which covers injection attacks for "All inputs from external sources"'

That being said, I do think that lines 378 and 475 could be added, as they do compliment the current lists.

In any case, if you would like to continue to contribute (which I surely hope you do!) it's always best to open a ticket first to make sure the PR will be accepted. This is described in our Contributing Guide, though maybe we should make this even clearer :).

@cpholguera what do you think?

Copy link
Collaborator

@cpholguera cpholguera left a comment

Choose a reason for hiding this comment

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

I agree with @TheDauntless but I also found that one line was already included. I've put all the fixes as suggestions which should be applied before merging. Thanks @Saket-taneja!

Copy link
Collaborator

@cpholguera cpholguera left a comment

Choose a reason for hiding this comment

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

Thanks for the new content @Saket-taneja! We highly encourage you to keep contributing, please take a look at our current Issues or create new ones :)

@cpholguera cpholguera merged commit 47602b0 into OWASP:master Oct 27, 2021
@cpholguera cpholguera changed the title 0x04e - About KID Misconfigurations 0x04e - Add JWT Token aud Claim Check Oct 27, 2021
@Saket-taneja
Copy link
Contributor Author

Hi @TheDauntless I disagree over here , as if a person is reading something from the MSTG , it would be great if we can provide all the knowledge and misconfigurations in our guide , In this case the reader doesn't has to move from one document to other. What are your thoughts @cpholguera

@cpholguera
Copy link
Collaborator

Hi @Saket-taneja, we're narrowing the scope of the MASVS and the MSTG. We prefer to link to other well maintained resources and standard whenever possible. It's not realistic that we can cover every possible scenario.

We'll try our best to cover the app scenarios (whatever could be fixed in the app-side). Server-side scenarios will be from now on out of scope and will be a task for e.g. the ASVS and the WSTG.

The KID configuration is something that needs a fix in the server-side. Therefore, we consider it to be out of scope here unfortunately. Have you checked the ASVS/WSTG? Maybe this could be added over there.

@cpholguera cpholguera changed the title 0x04e - Add JWT Token aud Claim Check [0x04e] Add JWT Token aun Claim Check Dec 23, 2021
@cpholguera cpholguera changed the title [0x04e] Add JWT Token aun Claim Check [0x04e] Add check for JWT Claim Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants