search

jaredhanson / passport-oauth2

OAuth 2.0 authentication strategy for Passport and Node.js.
https://www.passportjs.org/packages/passport-oauth2/?utm_source=github&utm_medium=referral&utm_campaign=passport-oauth2&utm_content=about
MIT License
607 stars 343 forks source link

Include check that access token was actually successfully returned #144

Closed ben12385 closed 3 years ago

ben12385 commented 3 years ago

getOAuthAccessToken returns an error only if it is a HTTP error, if the identity provider does not follow OAuth2 standard and instead returns a status 200 indicating it is an error, authentication can still happen. I have therefore included a check to ensure some value for accessToken must have been returned. It is done here instead of the OAuth library because I saw that the OAuth library was last updated in 2017.

Do let me know if you have any concerns (not sure if will impact passport-facebook) and thank you for creating Passport and the various modules.

Separately, I have also raised a CVE for this so that everyone is more aware of this since if the identity provider returns unsuccessful but is a status 200, an empty authorization token to the callback URL will deem the user authenticated to the application.

jaredhanson commented 3 years ago

I don't actually believe this constitutes a security vulnerability. That being said, there is clearly no harm in this so I've merged and published passport-oauth2@1.6.1 incorporating this patch.

As a result of this PR being opened, Snyk also contacted me asking for an assessment. Since this is public at this point, I've written up my assessment on the blog: https://medium.com/passportjs/no-access-token-no-service-7fb017c9e262

ben12385 commented 3 years ago

Hi Jared, just read your post and wanted to clarify on why I thought that this is a security vulnerability. The reason why this was found is because I was testing this library for authentication to a on-prem COTs IdP which actually lead me to discover that I am able to authenticate with an empty token. I can show a demo separately if you would like.

There are multiple caveats to this though:

  1. IdP does not follow OAuth2 protocol and return 200 instead of 400 for invalid access token.
  2. No further use of the access token or no error check when using the access token

The reason why I decided to bring this up as a vulnerability here instead of at the IdP level is more of an easy fix compared to checking and ensure that the IdP follows OAuth2 standard correctly. Apologies if this has caused any trouble for you.

Mitre had assigned it CVE-2021-41580.