ASVS icon indicating copy to clipboard operation
ASVS copied to clipboard

Add OAuth2 requirements

Open csfreak92 opened this issue 3 years ago • 59 comments

Ed note: This issue was originally called "v3.5 Token-based Session Management addition"


Ed note 2: If you are just joining us, the draft chapter is currently here: V51 OAuth 2.0 Protocol

Before you review it, read through this issue thread to collect the context and then feel free to open separate issues if you have specific questions or suggestions.


Original text starts here:

I think we need to add a control that validates the incoming HTTP request in an API endpoint which issues new access and refresh tokens. Usually this is with the refresh endpoint, but in some weirdly built applications, there were some implementations that every response would contain the new tokens that could be used for the next authenticated and authorized calls to the API. The requirement would be for the refresh token endpoints to have a validation of the requests, URL and request body must be validated before issuing the new set of tokens.

The rationale about this thought came to me when I was testing a few recent applications that either have refresh endpoints or a few ones that do not have refresh endpoints but the next authenticated tokens are coming from the HTTP responses. This is a bit of an old way of doing it as I understood and a bit uncommon. However, I noticed a pattern of accepting the request even if it was malformed or missing the object id or any reference for a resource (e.g. http://www.example.com/12345 but I the request was http://www.example.com/ralph; second URL is invalid and should be rejected). The application returns no valuable data except the new set of tokens which can be used for the succeeding calls to the API. I think it was a bad design, but for defense in depth purposes and preventing an attacker for prolonging their usage of the compromised account (which would be cut short had the application been following proper OIDC flow), the application should check for incoming HTTP requests to be correct in format, parameters, etc. and if anything is wrong then reject the request and never send back new tokens.

I would place this under v3.5 Token-based Session Management. I don't know whether this is still an acceptable thing to worry about and whether the threat model is too low to bother about this control, but I think it just seems like a basic thing that developers should have placed some sort of validation for requests before allowing the scenario described to happen. If this is too low risk, then we could scrap it.

csfreak92 avatar May 12 '21 07:05 csfreak92

I like this, but do we need a new OAuth2 section that covers https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-18 ?

jmanico avatar Sep 24 '21 03:09 jmanico

That is a great question though.. In wishful thinking I think we should add something OAuth2 best practices as well. I believe a lot of developers are not aware how to properly implement this on their applications.

csfreak92 avatar Oct 18 '21 06:10 csfreak92

OAuth2 is a special protocol for just delegation and is often misused. But a dedicated section for OAuth in 5.0 is a great idea!

jmanico avatar Oct 18 '21 14:10 jmanico

@jmanico, would you like me to draft a proposal for OAuth2 dedicated section? I believe that also deals with the original issue I opened here.

csfreak92 avatar Nov 10 '21 14:11 csfreak92

Mr. @csfreak92 absolutely yes. And we can a get few of the OAuth2 security group folks to review. You can extract a lot from https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics to help you

jmanico avatar Nov 10 '21 16:11 jmanico

Sounds good, let me thoroughly read it and then draft something soon (might be a bit later). :)

csfreak92 avatar Nov 12 '21 05:11 csfreak92

Just an update. I'm still actively working on this over spare time, please do not close the issue.

csfreak92 avatar Apr 29 '22 08:04 csfreak92

Hi @csfreak92, any update on this, would like to get this prepared for 5.0

@set-reminder 1 month consider closing this issue

tghosth avatar Dec 07 '22 15:12 tghosth

Reminder Saturday, January 7, 2023 12:00 AM (GMT+01:00)

consider closing this issue

octo-reminder[bot] avatar Dec 07 '22 15:12 octo-reminder[bot]

@tghosth, @jmanico, as promised here is my initial draft for the OAuth requirements (with some references to OpenID Connect) as I have derived from the RFC: (https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics).

I still need to add the CWE mappings and more details for the explanatory paragraphs about each of them. I have two questions how we wish to present these new to be added requirements:

  1. I propose a section 3.8.x to keep all OAuth requirements separate and easier for anyone using ASVS for security audits. If that is good for you guys, then shall I keep it like this where there's a separating subsection as per the security threats presented RFC that I followed? Also, shall I add separate explanatory paragraphs each or do you prefer all those explanations about Authorization Servers, Clients, Resource Servers, PKCE, etc. all in one huge chunk of paragraphs right after 3.8.x section title? Which one do you think is better?
  2. If having a separate section for OAuth requirements seem quite too much and redundant (as we have token-based session management requirements), then I will need further help with splitting them to other 3.x sections.

I am sure I might be missing a few more things, but I just pushed my initial draft so that I don't keep holding this issue from moving forward and start the year right. :)

csfreak92 avatar Jan 03 '23 07:01 csfreak92

I have not yet went into details, but first impression is - 18 requirements on one topic is a bit too much and maybe we need some abstraction.

I don't think we need all related OAuth requirements in one category if the same point is covered in general somewhere else. To have easy to follow "checklist" is maybe more testing guide approach.

Maybe we should have discussion in the issue first, not discussion over PR's. From PR's it's hard to follow later, why some requirements got in or why some changes were made.

Really nice input to work on!

elarlang avatar Jan 03 '23 09:01 elarlang

@elarlang, yeah I do agree that 18 requirements for one topic is quite too much, honestly. I have this dilemma too, but there's another dilemma if I will cut them down.

An example is if you look at 3.8.12 - 3.8.14 in that draft, those all pertain to IDORs and ACLs which are covered somewhere in ASVS. However, OAuth-specific ones kind of make sense if our main objective in this issue is to teach developers and engineers how to properly use OAuth. This is one of my dilemmas which had been paralyzing me for a while what to do with it. I'd like to hear it from the community so that I can finally get that off my head. :)

All of these 18 are derived from the OAuth RFC which is quite comprehensive. Great yeah, having a discussion about the issue would be very helpful too, at least in my view for trimming down the PR.

csfreak92 avatar Jan 03 '23 10:01 csfreak92

OAuth is complex. 18 requirements are critical. https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics has many more and is recent...

jmanico avatar Jan 03 '23 17:01 jmanico

I love this work! Steallar!

jmanico avatar Jan 03 '23 17:01 jmanico

Another topic is category - I don't think session management is the best place for covering OAuth. If it is 18 requirements (or even 10+) with splitting to different topics, it may be worth separate main category.

elarlang avatar Jan 04 '23 09:01 elarlang

OAuth is so important for _access delegation and API access that I do think it's critical for a new category.

Once we have the initial batch I can loop in some top tier experts to review.

And please be sure to include the requirements from the OAuth 2.0 BCP (Best Current Practice) for the most up to date requirements.

jmanico avatar Jan 04 '23 17:01 jmanico

🔔 @tghosth

consider closing this issue

octo-reminder[bot] avatar Jan 06 '23 23:01 octo-reminder[bot]

@jmanico, almost those 18 requirements I have drafted there were straight from the RFC's best current practice. I may have left some out, which I will do another few passes to ensure at least the critical ones are included.

Also, if this will be a new category, what number shall I assign to it? Should it be next to Session Management category so that it stays relative?

csfreak92 avatar Jan 07 '23 10:01 csfreak92

Hi @csfreak92,

This is really fantastic work, I can see how much effort you have put into it.

However, I am a little nervous that the level of detail is significantly more that we should probably have in the ASVS. The level of detail feels more like an OWASP cheatsheet. (I think this point could also be accurately made about parts of V2 but that is a different discussion for another day).

@jmanico what do you think?

@csfreak92 Do you think there is any way it could be trimmed down to key essentials?

For now I have created a dedicated branch for it.

tghosth avatar Jan 08 '23 16:01 tghosth

Thanks @tghosth, @jmanico and @elarlang for chiming in your thoughts and praises! It is still draft at first stages, but I had a lot of dilemmas presenting these requirements as I said earlier that some of them may have been covered by ASVS in other sections (in some ways or another).

@tghosth, I have the same worry too as the level of detail was taken straight from the RFC which makes it too strict of an implementation guideline including the Authorization Server, Resource Server and other components that make up an OAuth setup.

I'll see what I can do to trim them down to the key essentials and push another commit soon. I'll add the subsection explanatory paragraphs too to explain such terms and their meaning.

csfreak92 avatar Jan 09 '23 04:01 csfreak92

thanks, I think it deserves it's own section and I think it should obsolete 3.5.1

tghosth avatar Jan 12 '23 15:01 tghosth

Still doing some clean-up and trimming it down. Any number for the new section that I should use, @tghosth? I agree that the current 3.5.1 should be obsolete after this new section.

csfreak92 avatar Jan 30 '23 18:01 csfreak92

Also, combing through the RFCs and related documents together with searching the net for any CWEs that I can map the new OAuth 2.0 requirements and could not piece them together. Any ideas how else I can do that? Or shall we leave them blank for now and add those later?

csfreak92 avatar Jan 30 '23 19:01 csfreak92

I think it does not make sense to waste time for CWE mapping at the moment as there is quite big chance that we will remove this mapping.

https://github.com/OWASP/ASVS/issues/1481

elarlang avatar Jan 31 '23 06:01 elarlang

Agree that there is no need for a CWE mapping right now.

I remain concerned by the number and complexity of the requirements and still think it needs cutting down. I would also urge you to create a cheatsheet because I think the information you have provided is super valuable, just too in-depth for this use case.

Can we cut this down to a few requirements that an OAuth2 client needs to consider and a few requirements that a Resource Server needs to consider. Presumably implementing an Authorization Server is a much more niche situation.

What do you think @csfreak92?

tghosth avatar Apr 03 '23 16:04 tghosth

@tghosth, I did not add anymore CWE mapping for the draft pull request.

Regarding the complexity of requirements, I agree with you that maybe we should just focus on OAuth2 client needs as the implementation of an Authorization Server is a niche situation. Although, just out of curiosity, what are the chances and scenarios where an organization would implement their own Authorization Server?

About the cheatsheet, for sure I can do that. Where shall I place those? I presume appendix somewhere?

csfreak92 avatar Jul 10 '23 15:07 csfreak92

Hey @csfreak92, good to see you 😀👋

Regarding the complexity of requirements, I agree with you that maybe we should just focus on OAuth2 client needs as the implementation of an Authorization Server is a niche situation.

Would be great if you could do this, yes :)

Although, just out of curiosity, what are the chances and scenarios where an organization would implement their own Authorization Server?

I am not sure to be honest but I think it is a little unusual.

About the cheatsheet, for sure I can do that. Where shall I place those? I presume appendix somewhere?

Suggest you put it into the cheatsheets project. I would suggest you open an issue and ask them whether you should add it to the existing cheatsheet or create a new one.

tghosth avatar Jul 10 '23 15:07 tghosth

@tghosth, apologies for being MIA. Conferences and work travel has taken over the past few months. If we have an instant messaging channel/system we use, please add me there too :)

Ok, will rework my pull request and submit an update before I fly again in 4 days.

If the implementation of an Authorization Server is quite unusual, then yeah I agree we have to trim those down from the OAuth 2.0 requirements for ASVS, but I'll retain these on the cheatsheet.

Oh, ok let me do that cheatsheet project separately. For a second I thought you wish to add that in ASVS, good to know I clarified.

csfreak92 avatar Jul 10 '23 16:07 csfreak92

apologies for being MIA. Conferences and work travel has taken over the past few months.

Didn't mean that :) Just pleased to see you, I was worried we had scared you off 😂

If we have an instant messaging channel/system we use, please add me there too :)

We will probably speak further about this, is LinkedIn messaging the best way to get in contact with you?

Ok, will rework my pull request and submit an update before I fly again in 4 days.

Fantastic, thanks!

Oh, ok let me do that cheatsheet project separately. For a second I thought you wish to add that in ASVS, good to know I clarified.

Excellent :)

tghosth avatar Jul 10 '23 17:07 tghosth

No scare at all! Just got flying to different places and cities for work recently and got a breather.

Yes please, for now we can use LinkedIn messaging until the leaders and core team agrees on an instant comms channel. I look forward to your message! :)

@tghosth, quick question: won't it be overwhelming for folks out there that there's bunch of OWASP cheatsheets we have? I checked it out and there's a lot for different things which I would argue most are covered either by ASVS, OTG or Top 10. Anyway, that's not very important, just curious about it.

csfreak92 avatar Jul 10 '23 18:07 csfreak92