Revoking token does not work for public clients

[ostinelli]

Hello,
I'm using grant implicit to allow authorizing an application from mobile applications, by initiating the flow as follows:

https://example.com/oauth2/authorize?response_type=token&client_id={CLIENT_ID}&redirect_uri={REDIRECT_URI}&scope={REQUESTED_SCOPES}&state={STATE}

After a successful oauth flow, the generated token is saved in the oauth_access_tokens table jointly with the application referenced by the original client_id value:

#<Doorkeeper::AccessToken id: 396, resource_owner_id: 2, application_id: 1, token: "49d9a5c6232d6880e9b8df1c9369edf6bfc00aceedd5bb8164...", refresh_token: nil, expires_in: 1209600, revoked_at: nil, created_at: "2016-10-05 12:37:11", scopes: "profile">

The oauth_access_grants table is empty, as expected since using implicit.

When I try to revoke this token using the POST /oauth/revoke API endpoint (the one at the bottom, which does not require client_id or client_secret to be passed), it does not get revoked.

Diving into the source code, I see in the tokens_controller.rb:

# OAuth 2.0 Section 2.1 defines two client types, "public" & "confidential".
# Public clients (as per RFC 7009) do not require authentication whereas
# confidential clients must be authenticated for their token revocation.
#
# Once a confidential client is authenticated, it must be authorized to
# revoke the provided access or refresh token. This ensures one client
# cannot revoke another's tokens.
#
# Doorkeeper determines the client type implicitly via the presence of the
# OAuth client associated with a given access or refresh token. Since public
# clients authenticate the resource owner via "password" or "implicit" grant
# types, they set the application_id as null (since the claim cannot be
# verified).
#
# https://tools.ietf.org/html/rfc6749#section-2.1
# https://tools.ietf.org/html/rfc7009

However, as per above the token does specify the application_id, despite having used the implicit grant type.

Hence, when I try to revoke this token I land in the controller's authorized? method:

def authorized?
  if token.present?
    # Client is confidential, therefore client authentication & authorization
    # is required
    if token.application_id?
      # We authorize client by checking token's application
      server.client && server.client.application == token.application
    else
      # Client is public, authentication unnecessary
      true
    end
  end
end

Where token is present and specifies an application_id, however server.client is nil hence authorized? returns false and the token does not get revoked.

I'm using doorkeeper 4.2.0.

Is this expected behavior or am I missing something?

Thank you.

[tute]

Looping in @f3ndot for context on this. Thank you both!

[f3ndot]

You very well may be correct @ostinelli.

From the RFC 6749, Sec 4.2 (emphasis added):

The implicit grant type is used to obtain access tokens (it does not
support the issuance of refresh tokens) and is optimized for public
clients known to operate a particular redirection URI.
...
The implicit grant type does not include client authentication, and
relies on the presence of the resource owner and the registration of
the redirection URI. Because the access token is encoded into the
redirection URI, it may be exposed to the resource owner and other
applications residing on the same device.

So you're correct in that:

1. implicit is a public client mode
2. It does not authenticate the OAuth client

What we have here @tute is a conflation of design with what application_id means. Unfortunately my previous work left it ambiguous. After my patch, TokensController#authorized? assumes that if application_id is set then the token was created for a confidential client that authenticated correctly. It further assumes that if it's not set then the token was created for a public client. Prior to fb93805 I believe Doorkeeper set application_id as statistical/informational and wasn't used as a key value for determining the "publicness" of a client. I even had the foresight to call it out in the commit message:

This patch assumes that all public clients issue tokens with a NULL application_id, which may or may not be completely correct.

There are two ways forward:

1. Set application_id as NULL for implicit grant tokens to be consistent with other public grants and thusly #authorized? will work properly (less preferred)
2. Refactor the design of Doorkeeper to always set application_id for tokens, regardless of the client's "publicness", for statistical purposes, and create a new column that #authorized? can rely on for determining whether that token was issued under a public or confidential means. This, however, may be a MAJOR-level bump? Semver is hard 😉

With Option 2, more research would have to be done with the RFCs on what a public/confidential means. Standing questions:

• Is a confidential/public client determined by an OAuth application's defined acceptable grant types?
• Can you issue tokens both publicly and confidentially for the same application? That is, one application can choose to authenticate publicly in one part of itself, getting a token, and then authenticate confidentially somewhere else during its lifetime and getting another token.
• Does the RFC enforce the notion that an OAuth application can only be confidential or only public?

Answering these questions would inform the design of where this new column of truth should live. For example if an application can only define one acceptable grant type:

1. oauth_applications should get a grant_type column stating the public or confidential grant it wishes to authenticate under
2. Every call requesting an access token for said application should verify the grant type is allowed, verify the client secrets (if confidential).
3. The revocation endpoint takes a token and determines whether or not to enforce client authentication by looking up the application via application_id and reading it's grant_type value.

[f3ndot]

It's also worth noting this is a security vulnerability akin to CVE-2016-6582 but restricted only the the implicit grant type.

[ostinelli]

@f3ndot, thank you for looking into this.

Maybe I'm missing a piece here, but to my understanding we still need to keep information on the application a token refers to (application_id seems like a good place for it). Otherwise I might have missed how you could link a token to the application that a user has authorized to access their account. Hence I can't see how option 1 could work?

[f3ndot]

@ostinelli It somewhat depends how you've written your business logic, but in general a token is tied to both a resource the user owns (resource_owner_id) and the application that granted the token application_id.

You get access from a token to a user's account via that resource_owner_id field, not necessarily the application.

But hey, maybe I'm wrong too! I'm not a super expert on the spec 😉

[f3ndot]

cough excuse me. I clearly need more ☕️

You are correct. Option 1 has less fidelity as you don't know which application that token belongs to, if using a public grant type. However, you cannot cryptographically guarantee that it belongs to that publicly auth'd application hence why it should be considered informational vs a truth.

This is why I don't entirely mind that it is set to NULL for public clients.

It may be too tempting for developers who don't fully understand OAuth 2.0's intent behind public and confidential grant types to rely on application_id when it can be very, very easily spoofed for public grants

[ostinelli]

You definitely have more grasp than me on the subject :)

My consideration mainly was: when a user authorizes an application, we clearly need to allow a developer know which user is authorized to access which application by a token. My understanding is that this is done with the application_id (application) and the resource_owner_id (user) fields. Hence I'm not understanding how application_id can be set to nil: given a token, we would know which user it belongs to, but not which application it authorizes...

I might just have missed a point though :)

[nbulaj]

Hi @f3ndot @ostinelli . Where we are on this issue?

[ostinelli]

We reverted to 3.x back then and patched it to our needs. I might come back to this and look what the situation is nowadays.

[nbulaj]

Hi @ostinelli . It would be great if you could take a look at this issue.

[ostinelli]

Hi, as per my previous message we reverted back and still are using 3.x. Haven't had the time to check the new code yet...

[f3ndot]

@nbulaj This is still an active issue.

Conveniently, because we now have an Application#confidential? method we can solve this without either vastly refactoring what AccessToken#application_id means (informational vs a cryptographically guaranteed fact) or setting it as NULL to convey its not to be trusted (and thus losing fidelity).

There is a tripping hazard where developers would not know that when Application#confidential? is false, that AccessToken#application_id can only be used for statistical purposes but that issue exists already today. We could offer, outside the scope of this issue, an AccessToken#application! which would raise when the access token was created under public application circumstances.

I'll put up a PR for this issue today

[brianp]

I've come across this issue just this week, and was looking to see if it was already reported.

It looks like it's under control now with a clear direction to fix. Once the fix has been merged and potentially back ported to 4.x.x (?) should a CVE be opened for the issue so automated auditors could recommend the upgrade?

[nbulaj]

Take a look at #1119 (comment)

[f3ndot]

Whether or not we can backport to 4.x (although I'd love that), this should trigger another CVE. I'll request one now

[f3ndot]

CVE requested via the Distributed Weakness Filing Project

[f3ndot]

CVE-2018-1000211 assigned.