pkce does not seem to work

[leo-baltus]

Describe the bug

Running hydra v1.0.0 with a postgresql backend, its seems that code_challenge_method, code_challenge and code_verifier are ignored, as it does not make any difference how right or wrong it is used.

Reproducing the bug

curl 'https://auth.hydra.xxx.nl/oauth2/auth?access_type=offline&client_id=radio1-app&code_challenge=ahsbr8liNiieY9JKiSoX0EEVK52xrlGCFvM-zG-Tuhc&code_challenge_method=S256&redirect_uri=http%3A%2F%2F127.0.0.1%3A5555%2Fcallback&response_type=code&scope=openid+profile+email&state=I+wish+to+wash+my+irish+wristwatch'

This should be a valid challenge, the resulting code-to-access-token exchange is always sucessfull no matter how wrong my code_verifier is.

Also setting e.g. code_challenge_method=S257 does not result in some form of error.

Server logs

logs showing how S257 is allowed:

{"level":"info","method":"GET","msg":"started handling request","remote":"145.58.114.228","request":"/oauth2/auth?access_type=offline\u0026client_id=radio1-app\u0026code_challenge=_RpfHqw8pAZIomzVUE7sjRmHSM543WVdC4o-Kc4_3C0\u0026code_challenge_method=S257\u0026redirect_uri=http%3A%2F%2F127.0.0.1%3A5555%2Fcallback\u0026response_type=id_token\u0026scope=openid+profile+email\u0026state=I+wish+to+wash+my+irish+wristwatch","request_id":"244f22dfd4acb49e867323294257871d","time":"2019-08-05T16:04:49Z"}
{"level":"info","measure#hydra/public: https://auth.hydra.xxx.nl/.latency":10941200,"method":"GET","msg":"completed handling request","remote":"145.58.114.228","request":"/oauth2/auth?access_type=offline\u0026client_id=radio1-app\u0026code_challenge=_RpfHqw8pAZIomzVUE7sjRmHSM543WVdC4o-Kc4_3C0\u0026code_challenge_method=S257\u0026redirect_uri=http%3A%2F%2F127.0.0.1%3A5555%2Fcallback\u0026response_type=id_token\u0026scope=openid+profile+email\u0026state=I+wish+to+wash+my+irish+wristwatch","request_id":"244f22dfd4acb49e867323294257871d","status":302,"text_status":"Found","time":"2019-08-05T16:04:49Z","took":10941200}

Server configuration

log:
  level: info
  format: json
serve:
  public:
    port: 4444
    host:
    cors:
      enabled: false
    access_log:
      disable_for_health: false
  admin:
    port: 4445
    host:
    cors:
      enabled: false
    access_log:
      disable_for_health: false
  tls:
dsn: postgres://hydra:password@sqlproxy:5432/hydradb?sslmode=disable
webfinger:
  jwks:
    broadcast_keys:
      - hydra.openid.id-token # This key is always exposed by default
  oidc_discovery:
    client_registration_url: https://my-service.com/clients
    supported_claims:
      - email
      - username
    supported_scope:
      - email
      - whatever
      - read.photos
    userinfo_url: https://example.org/my-custom-userinfo-endpoint
oidc:
  subject_identifiers:
    enabled:
      - pairwise
      - public
    pairwise:
      salt: some-random-salt
  dynamic_client_registration:
    default_scope:
      - openid
      - offline
      - offline_access
urls:
  self:
    issuer: https://auth.hydra.xxx.nl
    public: https://auth.hydra.xxx.nl
  login: https://login.hydra.xxx.nl/login
  consent: https://login.hydra.xxx.nl/consent
  post_logout_redirect: https://usabilitygeek.com/ux-logout-lapse/
strategies:
  scope: DEPRECATED_HIERARCHICAL_SCOPE_STRATEGY
ttl:
  login_consent_request: 1h
  access_token: 1h
  refresh_token: 720h
  id_token: 1h
  auth_code: 10m
oauth2:
  expose_internal_errors: true
  hashers:
    bcrypt:
      cost: 10
secrets:
  system:
    - this-is-the-primary-secret
    - this-is-an-old-secret
    - this-is-another-old-secret
  cookie:
    - this-is-the-primary-secret
    - this-is-an-old-secret
    - this-is-another-old-secret
tracing:
  provider:

Expected behavior

I expected an error on getting the accesstoken using a false code_verifier
Also I expect errors when using unsupported code_challenge_method or too short code_challenge's

Environment

Docker/kubernetes

• v1.0.0

[aeneasr]

Is token_endpoint_auth_method set to none for radio1-app?

[leo-baltus]

No, it was left at the default client_secret_basic. After setting it to none:

failed to get token: oauth2: cannot fetch token: 401 Unauthorized
Response: {"error":"invalid_client","error_description":"Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)","error_hint":"The OAuth 2.0 Client supports client authentication method \"none\", but method \"client_secret_post\" was requested. You must configure the OAuth 2.0 client's \"token_endpoint_auth_method\" value to accept \"client_secret_post\".","status_code":401}

I have tried the same setup with Okta and Cognito, they work like this out-of-the-box. I.e. code-grant and code_challenge.

[aeneasr]

PKCE only makes sense in cases where no client secret can be used. You are using a client secret (hence the error message regarding client_secret_post). In that case, PKCE is useless and it’s even dangerous to be using client secrets in cases where you want PKCE. However, we should throw a warning when PKCE is being used with a secret. Try exchanging the auth code without a client secret snd it will work.

…

On 6. Aug 2019, at 07:56, leo-baltus ***@***.***> wrote: No, it was left at the default client_secret_basic. After setting it to none: failed to get token: oauth2: cannot fetch token: 401 Unauthorized Response: {"error":"invalid_client","error_description":"Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)","error_hint":"The OAuth 2.0 Client supports client authentication method \"none\", but method \"client_secret_post\" was requested. You must configure the OAuth 2.0 client's \"token_endpoint_auth_method\" value to accept \"client_secret_post\".","status_code":401} I have tried the same setup with Okta and Cognito, they work like this out-of-the-box. I.e. code-grant and code_challenge. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[leo-baltus]

For a public client, using a client secret obviously is not a wise thing to do. Im my case I am exploring PKCE in a private client so no issues with that, it might even enhance security.

Maybe PKCE and public clients can be regarded as two separate things? That way all clients can benefit from PKCE.

[aeneasr]

PKCE is a defensive measure against a scenario that can _only_ happen in a public scenario (mobile phone, desktop app). While using it with private clients on server side apps has no downside, it also has no upside (it does not improve security). However, using PKCE in a public scenario with a private client _is_ a security issue. Hence the idea of warning here.

…

On 6. Aug 2019, at 09:14, leo-baltus ***@***.***> wrote: For a public client, using a client secret obviously is not a wise thing to do. Im my case I am exploring PKCE in a private client so no issues with that, it might even enhance security. Maybe PKCE and public clients can be regarded as two separate things? That way all clients can benefit from PKCE. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[leo-baltus]

Consider the case of client behind a loadbalancer which does tls termination leaving the last part of the connection (LB to client) over plain http. I think a client could benefit from the extra measure. Hydra would also be on par with Okta, Cognito etc.

[aeneasr]

If you have a MITM PKCE won’t do you any good. I’m sorry but that’s just not what PKCE does and that’s not how network security works and no hashed string will ever protect you from eavesdropping network connections. Feel free to read the spec yourself if you do not trust us to make the right calls: https://tools.ietf.org/html/rfc7636 The people that wrote the spec (check the title for the public part) are pretty good at what they’re doing. Hydra is not on par with Okta or Cognito. It excels both with better defensive mechanisms and real controls to keep you from making mistakes (like this one). I mean Okta uses security questions for recovering Admin Accounts (iCloud hacks anyone?). Hydra is more flexible, faster, more lightweight and open source.

…

On 6. Aug 2019, at 09:32, leo-baltus ***@***.***> wrote: Consider the case of client behind a loadbalancer which does tls termination leaving the last part of the connection (LB to client) over plain http. I think a client could benefit from the extra measure. Hydra would also be on par with Okta, Cognito etc. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[leo-baltus]

Funny how https://tools.ietf.org/html/rfc7636 exactly specifies my example:

In this attack, the attacker intercepts the authorization code
returned from the authorization endpoint within a communication path
not protected by Transport Layer Security (TLS), such as inter-
application communication within the client's operating system.

The outgoing connection is protected by TLS, so no MITM there. The incoming connection however is vulnerable to MITM but an attacker does not have the verifier to get the access token.

[aeneasr]

No, the scenarios are completely separate. In the spec, the redirect url is not protected by TLS because it's using a custom redirect scheme (the only scenario where no TLS is allowed for Hydra). A malicious app can register on that scheme and receive the code BEFORE the legitimate app.

In your scenario, there is no TLS connection between the LB and the server side app. A MITM could therefor listen in on all the traffic, including the result of the code exchange.

We are going in circles and there is no productive outcome. I'll make a patch to address your initial issue (using PKCE should throw an error when a private client is being used) but that's as much time as I can dedicate to discuss this.

[leo-baltus]

In your scenario, there is no TLS connection between the LB and the server side app. A MITM could therefor listen in on all the traffic, including the result of the code exchange.

We may be talking about different technologies but a LB is different from e.g. VPN, only incoming connections are proxied the client so outgoing calls are initiated over separate connections making their own tls handshake so code exchange is not going through the loadbalancers.

[aeneasr]

We may be talking about different technologies but a LB is different from e.g. VPN, only incoming connections are proxied the client so outgoing calls are initiated over separate connections making their own tls handshake so code exchange is not going through the loadbalancers.

In that case you still need the client secret (not known to the attacker) to exchange the code. Again, this attack only makes sense in the case where the client can not keep a secret confidential - mobile apps, desktop apps, and so on.

[leo-baltus]

Again, these are separate concerns. A public client cannot keep a client secret because it is a public client. A private client can be vulnerable to code hijacking if its incoming requests is not protected by TLS. They both can benefit from PKCE. In fact I do not see why modern private clients would not use PKCE in all cases.

[aeneasr]

Private clients are not vulnerable to code hijacking because the hijacker does not have the client secret, which is required for the exchanging the code for the token. PKCE is a substitute for the client secret. Period.

[leo-baltus]

https://tools.ietf.org/html/draft-ietf-oauth-security-topics-09#section-2.1.1

Note: although PKCE so far was recommended as a mechanism to protect
native apps, this advice applies to all kinds of OAuth clients,
including web applications.

[aeneasr]

Sorry, completely overlooked this :) Thank you for digging this up! Will change behavior accordingly!

[ghost]

Yes, even if it is recommended for maximum security to implement PKCE on all platforms. It requires currently some iframe hacks to manage correctly local redirect management within the same context between parent and child iframe (it is annoying for those who implemented correctly CSPv2, to reduce our CSP security (local references) just to implement PKCE in Web).

[thibaultponcelet]

Hi, In order to follow https://tools.ietf.org/html/draft-ietf-oauth-security-topics-09#section-2.1.1 recommendation we also want to enable PKCE on private clients.

Is there already a timeline on when is this going to be available in Hydra @aeneasr?
We could also help on this if it is not but we have nobody fluent in Go in our team so it could take some time...

Thanks a lot

[aeneasr]

No timeline at the moment as we have iceboxed new features on our end for the next two months. However, we'd love a PR - you can probably work along the lines of: ory/fosite#375

[thibaultponcelet]

Thank you for your quick answer. We will see if we can manage to do it ourselves or not

[thibaultponcelet]

We fixed it in fosite here: ory/fosite#382