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 - About OTP Authentication Checks #1938

Merged
merged 4 commits into from
Jun 12, 2022
Merged

Conversation

Saket-taneja
Copy link
Contributor

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 >.

@cpholguera cpholguera changed the title Update 0x04e-Testing-Authentication-and-Session-Management.md About OTP Authentication Checks Oct 9, 2021
@cpholguera cpholguera changed the title About OTP Authentication Checks 0x04e - About OTP Authentication Checks Oct 9, 2021
@TheDauntless
Copy link
Collaborator

Thanks for the PR, @Saket-taneja

While I understand what you are trying to say, I don't know if this is the right place to do it. This section is about a backend enforcing 2FA, after which it will provide new information to the user (e.g. sensitive data). In your case, if the server simply returns true/false, it would mean that the sensitive information is already stored locally, or that there is client-side validation; both of which are already covered in the MASVS/MSTG.

Your recommendation also wouldn't fix the problem. If you say "> The application should always pass user token or some dynamic information related to the user to prevent the attack", then the backend could still simply return true / false and you would still be 'vulnerable' or at least have the same issue.

As I mentioned in #1939, it's best to first open an issue so that we can have a discussion on the merit of the addition.

@cpholguera
Copy link
Collaborator

I agree with @TheDauntless, and please don't hesitate to raise issues whenever you feel that something's missing or incomplete. If something is already covered or not applicable we can save you the effort of working on PRs. Thanks @Saket-taneja :)

@cpholguera cpholguera closed this Oct 27, 2021
@Saket-taneja
Copy link
Contributor Author

@TheDauntless The case which i have enlisted over here also entails that in many of the cases , the application is configured in a way that server returns only true/false when 2fa is enabled.There are numerous reports on hackerone and bugcrowd you can refer that.The solution to almost all of them is that they all were using static responses and should be avoided.

CC : @cpholguera

@cpholguera
Copy link
Collaborator

Hi @Saket-taneja, could you please provide a couple of links to those reports? We'll take a look and if needed we'll open an issue for this.

@cpholguera cpholguera reopened this Oct 28, 2021
@Saket-taneja
Copy link
Contributor Author

@cpholguera Hey Buddy any updates on this ?

@cpholguera
Copy link
Collaborator

Hi @Saket-taneja, thanks for reaching out! We'll address more PRs in the upcoming year. As you might know we're in the middle of a big refactoring for the MASVS which will extend to the MSTG. We were also improving our pipelines so that we can release more often. Thanks for your patience and congratulations for making it to our list of New Contributors! 🎉

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.

Hi @Saket-taneja, thanks a lot for your PR and sorry for the late response from our side. We had to give more priority to the MASVS refactoring process and we are still working on it.

Here are a couple of suggested changes, if you like them you can click on the button "Commit suggestion" and they'll be integrated. Thanks a lot for the PR and for your patience!

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 a lot @Saket-taneja for your contribution and your patience. We're ready to go!

@cpholguera cpholguera merged commit 4146340 into OWASP:master Jun 12, 2022
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