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

cannot impersonate (sign in as) #683

Closed
Pyrolistical opened this issue Mar 1, 2018 · 17 comments · Fixed by #689
Closed

cannot impersonate (sign in as) #683

Pyrolistical opened this issue Mar 1, 2018 · 17 comments · Fixed by #689

Comments

@Pyrolistical
Copy link

Pyrolistical commented Mar 1, 2018

when "sign in as" is used from user management, it triggers the application configured callback:
http://localhost:9000/auth0-callback#access_token=some-access-token&id_token=some-id-token&scope=openid&expires_in=86400&token_type=Bearer
notice, there is no state.

but in auth0.js v9 we have:

WebAuth.prototype.validateAuthenticationResponse = function(options, parsedHash, cb) {
var _this = this;
var state = parsedHash.state;
var transaction = this.transactionManager.getStoredTransaction(state);
var transactionState = options.state || (transaction && transaction.state) || null;
var transactionStateMatchesState = transactionState === state;
if (!state || !transactionStateMatchesState) {
return cb({
error: 'invalid_token',
errorDescription: '`state` does not match.'
});
}

it always trips "state does not match"

@jimmyn
Copy link

jimmyn commented Mar 2, 2018

I have the same problem. The only solution I've found so far is to use v9.2.3. after this version parseHash method will always check state and if it does not exist return this error.

It is still unclear on how to do impersonation in a new version. I have a suspicion they are going to deprecate this feature.

@Pyrolistical
Copy link
Author

Found it @jimmyn v9.2.3...v9.3.1#diff-2f11667c8f671044416e38e4ceb9d7c8R186

You could argue this breaks semver, but you could argue they fixed a bug

@luisrudge
Copy link
Contributor

Hi folks! We're aware of the issue and working in a solution. Thanks for the patience 🎉

@luisrudge
Copy link
Contributor

We're adding a new option so you can enable impersonation again: #689

@Pyrolistical
Copy link
Author

The bug fix breaks semver: https://auth0.com/docs/support/how-auth0-versions-software

@luisrudge
Copy link
Contributor

Yes. We're sorry about that. But we're closing an attack vector which is way more important. There will be docs about the trade-offs of using __enableImpersonation and what kind of attacks it leaves your users vulnerable to.

@luisrudge
Copy link
Contributor

luisrudge commented Mar 9, 2018

hey @Pyrolistical, do you want to test the new version with the flag so we make sure this works in your scenario? I already tested locally, but it'd be good to have an extra pair of hands working on this one! If you want to test it, here's the build file from this PR: https://770-13125982-gh.circle-artifacts.com/0/home/ubuntu/auth0.js/build/auth0.js

you'll need to add the __enableImpersonation flag to your parseHash method, like this:

webAuth.parseHash({__enableImpersonation: true}, function(err, authResult){
  
});

@Pyrolistical
Copy link
Author

@luisrudge You are fixing this with an __enableImpersonation flag, but I have a way to fix this that doesn't break semver.

  1. When making an impersonation call, add a bypass-state parameter in the hash
  2. the bypass-state value is a timestamp on when the impersonation call was made + RS256 signature
  3. when auth0.js parseHash see bypass-state, it downloads the well known RS256 public key. it then verifies the RS256 signature of bypass-state and checks the timestamp is within the last 60 seconds

this allows impersonation to just work. the RS256 signature ensures only auth0 can ever set a valid bypass-state parameter. the timestamp in bypass-state prevents CSRF after 60 seconds

@luisrudge
Copy link
Contributor

This is a great idea but, unfortunately, a product decision that I cannot make. You can ask reach out to our support team in https://support.auth0.com and add your suggestion there. I'll personally send this to the appropriate team and we'll see how it goes 🎉

@johnlim
Copy link

johnlim commented Apr 11, 2018

@luisrudge @Pyrolistical On my end, what I've done is to call checkSession when parseHash (and hence validateAuthenticationResponse) returns error. I'm not sure if that is the original design intent of checkSession but using checkSession negates the need for the additional __enableImpersonation flag. As an example,

        this.auth0.parseHash(function (err, authResult) {
          if (authResult && authResult.accessToken && authResult.idToken) {
            //handle success 
            return
          } else {
            this.auth0.checkSession({
              scope: this.options.auth.scope
            }, function (err, result) {
              if (err || !result || !result.idToken || !result.accessToken) {
                // regular login
                return
              } else {
                //handle success
              }
            }

Would this then nullify the CSRF attack vector associated with __enableImpersonation?

@luisrudge
Copy link
Contributor

checkSession returns the current session in the authorization server (auth0). If impersonation sets a session in the server, then it works.

@johnlim
Copy link

johnlim commented Apr 12, 2018

@luisrudge Yes it works on my end and so it seems that impersonation does indeed set a session in the server. Thus moving forward, shouldn't that be the default method to use for impersonation rather than setting the __enableImpersonation which then exposes the client to CSRF attacks?

@luisrudge
Copy link
Contributor

luisrudge commented Apr 12, 2018

I'm not sure we want to recommend people calling checkSession if the parseHash fails. I'll talk with the team internally and see what they think. Thanks for pinging me with this info!

@johnlim
Copy link

johnlim commented Apr 13, 2018

@luisrudge One point to note that is that in the checkSession() method that I pointed out above, the impersonated & impersonator properties will never be set in the userInfo object.

@luisrudge
Copy link
Contributor

@johnlim After reviewing your specific workaround which uses checkSession when the hash fails to be parsed, both our product and security teams are advising against it because this behavior will certainly change in the future.

Moving forward, we want developers to think about the implications of enabling the __enableIdPInitiatedLogin flag and issues they might face if they chose to do so.

@johnlim
Copy link

johnlim commented May 1, 2018

@luisrudge Thanks! Will Auth0 provide more information on the implications and how we can mitigate them in the near future? At the moment, when I read the documentation, it seems to be flashing a big red flag that says "Use at your own risk!".

@luisrudge
Copy link
Contributor

This is in our docs: https://auth0.com/docs/user-profile/user-impersonation#enable-impersonation, it's not a flashing warning, but a warning anyway 😝
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants