Ignore PKCE params for non-PKCE grants #1415

toupeira · 2020-05-21T16:22:57Z

Summary

This aligns the behavior with sections 3.1 and 3.2 from RFC 6749:

The authorization server MUST ignore unrecognized request parameters.

https://tools.ietf.org/html/rfc6749#section-3.1
https://tools.ietf.org/html/rfc6749#section-3.2

Other Information

Closes #1411

toupeira · 2020-05-21T16:23:47Z

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -93,7 +84,7 @@ def validate_redirect_uri
      # if either side (server or client) request PKCE, check the verifier
      # against the DB - if PKCE is supported
      def validate_code_verifier
-        return true unless grant.uses_pkce? || code_verifier
+        return true unless grant.uses_pkce?


This check had to be removed as well, and caused the changed spec in spec/requests/flows/authorization_code_spec.rb to fail.

I think we can thinking about validate_code_verifier as following:

(1) pkce non supported (pkce_supported? == false) -> return true

pkce is supported (pkce_supported? == true)

code_challenge.present? code_challenge.blank?

code_verifier.present? (2) run validate (3) run validate

code_verifier.blank? (5) (already validated at validate_params)return false (4) return true

So, as the table, currently changes (Line 87 and 88) will not run validated on (2), when code_verifier is present, and code_challenge is not sent from client at authorization request, this can causing Downgrade Attack on PKCE

About (5)th condition, it is currently validated at validate_params, but I still check it in the code 👇, because I think we should consider this is independent from validate_params, any change to validate_params should not break validate_code_verifier (because validate_params's task is to informs that client missing necessary parameter)

So I think this validation could be written as below:

def validate_code_verifier return true unless pkce_supported? # for (1) if code_verifier.blank? return true if grant.code_challenge.blank? # for (4) return false # for (5) end # run validate for (2) (3) if grant.code_challenge_method == "S256" ..... end end

@linhdangduy thanks, this makes sense to me. 👍

I updated the method now as per your suggestion, though I simplified the 4/5 case into return grant.code_challenge.blank? if code_verifier.blank? and added a spec for the case where PKCE is enabled but both code_challenge and code_verifier are blank. I think the remaining cases should already be covered by the specs in spec/lib/oauth/authorization_code_request_spec.rb.

toupeira · 2020-05-21T16:25:15Z

spec/requests/flows/authorization_code_spec.rb

-          "error" => "invalid_grant",
-          "error_description" => translated_error_message(:invalid_grant),
-        )
+        url_should_have_param("code", Doorkeeper::AccessGrant.first.token)


I'm a bit confused by the PKCE specs in this file because it seems we don't mock Doorkeeper::AccessGrant).pkce_supported?, or set it up through other ways.

Should we add this?

I think yes

@nbulaj I saw now that we indirectly set this up by adding the relevant columns: https://github.com/toupeira/doorkeeper/blob/4a7a106dc03411d9ada0a627f6b46e9e8022c393/spec/dummy/db/schema.rb#L25-L28

Running these specs with WITHOUT_PKCE=1 makes them fail, so I think we're good here and I retract my concern 😁

linhdangduy · 2020-05-22T00:08:53Z

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -55,14 +54,6 @@ def pkce_supported?
        Doorkeeper.config.access_grant_model.pkce_supported?
      end

-      def validate_pkce_support
-        @invalid_request_reason = :not_support_pkce if grant &&


Because not_support_pkce is used only here, so should remove from other files, too. (en.yml, invalid_request_response.rb)

@linhdangduy thanks, missed those and removed them now 👍

linhdangduy · 2020-05-22T00:47:07Z

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -93,7 +84,7 @@ def validate_redirect_uri
      # if either side (server or client) request PKCE, check the verifier
      # against the DB - if PKCE is supported
      def validate_code_verifier
-        return true unless grant.uses_pkce? || code_verifier
+        return true unless grant.uses_pkce?


I think we can thinking about validate_code_verifier as following:

(1) pkce non supported (pkce_supported? == false) -> return true

pkce is supported (pkce_supported? == true)

code_challenge.present? code_challenge.blank?

code_verifier.present? (2) run validate (3) run validate

code_verifier.blank? (5) (already validated at validate_params)return false (4) return true

So, as the table, currently changes (Line 87 and 88) will not run validated on (2), when code_verifier is present, and code_challenge is not sent from client at authorization request, this can causing Downgrade Attack on PKCE

About (5)th condition, it is currently validated at validate_params, but I still check it in the code 👇, because I think we should consider this is independent from validate_params, any change to validate_params should not break validate_code_verifier (because validate_params's task is to informs that client missing necessary parameter)

So I think this validation could be written as below:

def validate_code_verifier return true unless pkce_supported? # for (1) if code_verifier.blank? return true if grant.code_challenge.blank? # for (4) return false # for (5) end # run validate for (2) (3) if grant.code_challenge_method == "S256" ..... end end

This aligns with sections 3.1 and 3.2 from RFC 6749: > The authorization server MUST ignore unrecognized request parameters. https://tools.ietf.org/html/rfc6749#section-3.1 https://tools.ietf.org/html/rfc6749#section-3.2

toupeira

@linhdangduy @nbulaj thanks for your feedback! 🙏 Your comments should be addressed now.

toupeira · 2020-05-25T15:07:05Z

spec/requests/flows/authorization_code_spec.rb

-          "error" => "invalid_grant",
-          "error_description" => translated_error_message(:invalid_grant),
-        )
+        url_should_have_param("code", Doorkeeper::AccessGrant.first.token)


@nbulaj I saw now that we indirectly set this up by adding the relevant columns: https://github.com/toupeira/doorkeeper/blob/4a7a106dc03411d9ada0a627f6b46e9e8022c393/spec/dummy/db/schema.rb#L25-L28

Running these specs with WITHOUT_PKCE=1 makes them fail, so I think we're good here and I retract my concern 😁

toupeira · 2020-05-25T15:08:00Z

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -55,14 +54,6 @@ def pkce_supported?
        Doorkeeper.config.access_grant_model.pkce_supported?
      end

-      def validate_pkce_support
-        @invalid_request_reason = :not_support_pkce if grant &&


@linhdangduy thanks, missed those and removed them now 👍

toupeira · 2020-05-25T15:10:18Z

lib/doorkeeper/oauth/authorization_code_request.rb

@@ -93,7 +84,7 @@ def validate_redirect_uri
      # if either side (server or client) request PKCE, check the verifier
      # against the DB - if PKCE is supported
      def validate_code_verifier
-        return true unless grant.uses_pkce? || code_verifier
+        return true unless grant.uses_pkce?


@linhdangduy thanks, this makes sense to me. 👍

I updated the method now as per your suggestion, though I simplified the 4/5 case into return grant.code_challenge.blank? if code_verifier.blank? and added a spec for the case where PKCE is enabled but both code_challenge and code_verifier are blank. I think the remaining cases should already be covered by the specs in spec/lib/oauth/authorization_code_request_spec.rb.

nbulaj

🚝
LGTM

nbulaj · 2020-05-26T07:27:06Z

Thanks @toupeira ! 🤝

toupeira commented May 21, 2020

View reviewed changes

toupeira mentioned this pull request May 21, 2020

Ignore code_verifier for non-PKCE grants #1411

Closed

linhdangduy reviewed May 22, 2020

View reviewed changes

Ignore PKCE params for non-PKCE grants

2cac358

This aligns with sections 3.1 and 3.2 from RFC 6749: > The authorization server MUST ignore unrecognized request parameters. https://tools.ietf.org/html/rfc6749#section-3.1 https://tools.ietf.org/html/rfc6749#section-3.2

toupeira commented May 25, 2020

View reviewed changes

nbulaj approved these changes May 26, 2020

View reviewed changes

nbulaj merged commit ebe0fc1 into doorkeeper-gem:master May 26, 2020

toupeira deleted the pkce-params-validation branch May 26, 2020 10:08

nbulaj added this to the 5.5 milestone May 27, 2020

This was referenced Mar 17, 2021

Bump doorkeeper from 5.4.0 to 5.5.0 postxiami/mastodon#44

Closed

Bump doorkeeper from 5.4.0 to 5.5.0 YoheiZuho/mastodon#320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore PKCE params for non-PKCE grants #1415

Ignore PKCE params for non-PKCE grants #1415

toupeira commented May 21, 2020

toupeira May 21, 2020

linhdangduy May 22, 2020 •

edited

Loading

toupeira May 25, 2020

toupeira May 21, 2020

nbulaj May 21, 2020

toupeira May 25, 2020

linhdangduy May 22, 2020

toupeira May 25, 2020

linhdangduy May 22, 2020 •

edited

Loading

toupeira left a comment

toupeira May 25, 2020

toupeira May 25, 2020

toupeira May 25, 2020

nbulaj left a comment

nbulaj commented May 26, 2020

	code_challenge.present?	code_challenge.blank?
code_verifier.present?	(2) run validate	(3) run validate
code_verifier.blank?	(5) (already validated at validate_params)return false	(4) return true

Ignore PKCE params for non-PKCE grants #1415

Ignore PKCE params for non-PKCE grants #1415

Conversation

toupeira commented May 21, 2020

Summary

Other Information

Choose a reason for hiding this comment

linhdangduy May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linhdangduy May 22, 2020 • edited Loading

Choose a reason for hiding this comment

toupeira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbulaj left a comment

Choose a reason for hiding this comment

nbulaj commented May 26, 2020

linhdangduy May 22, 2020 •

edited

Loading

linhdangduy May 22, 2020 •

edited

Loading