[OAUTH-WG] Barry Leiba's Discuss on draft-ietf-oauth-spop-12: (with DISCUSS and COMMENT)

["Barry Leiba" <[e-mail]>]

Barry Leiba has entered the following ballot position for
draft-ietf-oauth-spop-12: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-oauth-spop/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

After getting John's explanation of the background and details of the
attack (and the different communications paths involved, I still would
prefer that we build this protocol as a more robust one (with only S256
and not "plain"), but I understand the reasons to have it this way, and I
think it will be acceptable with some changes to the text to make the
situation clearer.

To that end, I suggest this:

In the introduction, I think the explanation of Figure 1 (the fourth
paragraph) should be changed to make it clear what communication paths
are used in each step, and where the vulnerable piece is.  Something like
this:

OLD
   In step (1) the native app
   running on the end device, such as a smart phone, issues an
   authorization request via the browser/operating system, which then
   gets forwarded to the OAuth 2.0 authorization server in step (2).
   The authorization server returns the authorization code in step (3).
   The malicious app is able to observe the authorization code in step
   (4) since it is registered to the custom URI scheme used by the
   legitimate app.  This allows the attacker to reguest and obtain an
   access token in step (5) and step (6), respectively.
NEW
   In step (1) the native app
   running on the end device, such as a smart phone, issues an
   authorization request via the browser/operating system.  The
   request includes a URI by which the response will be returned,
   and that uses a custom URI scheme.  Step (1) happens through a
   secure API that cannot be intercepted.  The request then gets
   forwarded to the OAuth 2.0 authorization server in step (2).
   Because OAuth requires the use of TLS, this communication is
   protected by TLS, and also cannot be intercepted. The
   authorization server returns the authorization code over the
   same TLS connection in step (3). In step (4), the Authorization
   Code is returned to the requester via the URI that was provided
   in step (1).
   
   A malicious app that has been designed to attack this native
   app has previously registered itself as a handler for the custom
   URI scheme, and is now able to observe the Authorization Code in
   step (4).  This allows the attacker to request and obtain an
   access token in steps (5) and (6).
END

That (or something like it) makes it clear that only one step is
vulnerable, and explicitly tells us that the other communication paths
are already protected.

In the list of pre-conditions, I suggest changing (1) to make "another
application" be "a legitimate application that uses OAuth".

*** IMPORTANT *** I am still puzzled by this, in pre-condition (4), which
seems to contradict what John said and what I proposed above:

   4) The attacker (via the installed app) is able to observe responses
      from the authorization endpoint.  As a more sophisticated attack
      scenario the attacker is also able to observe requests (in
      addition to responses) to the authorization endpoint.

Can you please explain the "more sophisticated attack", and how the
attacker can observe the request?  Because "plain" will NOT work in such
a situation.

I suggest changing the last paragraph in the Introduction like this:

OLD
   To mitigate this attack, this extension utilizes a dynamically
   created cryptographically random key called 'code verifier'.  A
   unique code verifier is created for every authorization request and
   its transformed value, called 'code challenge', is sent to the
   authorization server to obtain the authorization code.
NEW
   To mitigate this attack, this extension utilizes a dynamically
   created cryptographically random key called 'code verifier'.  A
   unique code verifier is created for every authorization request and
   its transformed value, called 'code challenge', is sent to the
   authorization server to obtain the authorization code.  This
   transmission is sent through a secure API, and cannot be
   intercepted.
END

I know it repeats what was said in the explanation above, but it's an
important enough point to merit repetition.

I suggest changing Section 4.4.1 like this (because of a misuse of "MAY"
that I missed before):

OLD
   If the client is capable of using "S256", it MUST use "S256", as
   "S256" is Mandatory To Implement (MTI) on the server.  Clients MAY
   use "plain" only if they cannot support "S256" for some technical
   reason and knows that the server supports "plain".
NEW
   If the client is capable of using "S256", it MUST use "S256", as
   "S256" is Mandatory To Implement (MTI) on the server.  Clients are
   permitted to use "plain" only if they cannot support "S256" for
   some technical reason and know that the server supports "plain".
END

Finally, I suggest changing this in Section 7.2:

OLD
   "S256" method protects against eavesdroppers observing or
   intercepting the "code_challenge".  If the "plain" method is used,
   there is a chance that it will be observed by the attacker on the
   device.  The use of "S256" protects against it.
NEW
   The "S256" method protects against eavesdroppers observing or
   intercepting the "code_challenge", because the challenge cannot
   be used without the verifier.  With the "plain" method, the
   challenge can be directly used during the verification step,
   so "plain" does not protect against interception of the initial
   request.
   
   Because of this, "plain" SHOULD NOT be used, and exists only for
   compatibility with deployed implementations where the request
   path is already protected.  The "plain" method MUST NOT be used
   in new implementations.
END

With that set of changes, or something like them (feel free to re-write,
and certainly correct anything I got wrong!), I will clear this DISCUSS
and we can move ahead.

If you find anything fundamentally messed up in any of that, please do
discuss it with me, and let's sort it out.  And, again, thanks for the
explanation that made this all clear.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Putting quotation marks around "code_verifier" and "code_challenge" in
the formulas is confusing: it makes it look as if you're putting in those
strings themselves, rather than the values of the variables.  It's
probably unlikely that anyone would make that mistake, but I nevertheless
suggest removing the quotation marks when you mean to refer to the
values.

-- Section 2 --
There is no real distinction between STRING and ASCII(STRING), because
STRING is already defined to be ASCII.  Using "ASCII(xxx)" only adds
clutter, and a suggest removing it.

So, for example, these last two comments would make changes such as this
one:

OLD
BASE64URL-ENCODE(SHA256(ASCII("code_verifier" ))) == "code_challenge"
NEW
BASE64URL-ENCODE(SHA256(code_verifier)) == code_challenge
END

-- Section 4.3 --
I would very strongly prefer that S256 be the default, not "plain"... or
that the method be required, with no default.  I understand that this
makes existing implementations break.

-- Section 4.4 --
The word "code" is used for too many things, and "Authorization Code" is
already the right name for what we're talking about here.  I suggest that
in both the section title and body you use that term, to make it clear
what you mean by the "code".  Similarly, in Section 4.5 please say
"code_verifier" rather than "secret".  (Nat plans to make these
changes.)

-- Section 4.4.1 --
Please expand "PKCE", which is, at the moment, only expanded in the
document title.  (Nat plans to make a change for this.)

-- Section 5 --
The SHOULD in the first paragraph is wrong.  You already have a MAY
covering the general behavior.  You should just take out the "SHOULD",
and just say that severs supporting backwards compatibility revert to the
normal OAuth protocol.  (Nat plans to make a change for this.)

-- Section 6.2 --
I have the same comment here as in the other OAuth document: please shift
the focus away from telling IANA how to handle tracking of the expert
review, and make the mailing list something that the designated expert(s)
keep track of.  Also, please give more instructions to the DEs about what
they should consider when they're evaluating a request (for example,
should they approve all requests, or are there criteria they should
apply?).

For the first, here's a text change that I suggest we move toward for
this sort of thing:

OLD
<most of Section 6.2>

NEW
   Additional code_challenge_method types for use with the authorization
   endpoint are registered using the Specification Required policy
   [RFC5226], which includes review of the request by one or more
   Designated Experts.  The DEs will ensure there is at least a two-week
   review of the request on the [e-mail] mailing list,
   and that any discussion on that list converges before they respond to
   the request.  To allow for the allocation of values prior to
   publication, the Designated Expert(s) may approve registration once
   they are satisfied that an acceptable specification will be
published.

   Discussion on the [e-mail] mailing list should use
   an appropriate subject, such as "Request for PKCE
   code_challenge_method: example").

   The Designated Expert(s) should consider the discussion on the
   mailing list, as well as <<these other things>> when evaluating
   registration requests.  Denials should include an explanation
   and, if applicable, suggestions as to how to make the request
   successful.
END

(Nat plans to make a change for this.)

-- Section 7.2 --
For the first paragraph, after discussion with the author I suggest
this:

NEW
Clients MUST NOT downgrade to "plain" after trying the S256 method.
Because servers are required to support S256, an error when S256 is
presented can only mean that the server does not support PKCE at all.
Otherwise, such an error could be indicative of a MITM attacker trying
a downgrade attack.
END