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

From: "Barry Leiba" <[email-masked]>
Date: Mon, 08 Jun 2015 12:42:49 -0700

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:
----------------------------------------------------------------------

How does "plain" do anything at all to mitigate this attack?  Wouldn't
anyone who could snag the grant also be able to snag the code verifier as
well?  Why is "plain" even here?

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

General:
I would think that this mechanism should never be a substitute for using
TLS, and that this document should be explicit about that, and should say
that the proper mitigation for situations where TLS can be used... is to
use TLS.  Is there a reason we should NOT say that?

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 --
I don't understand the distinction between STRING and ASCII(STRING).  Can
you please explain it?

-- Section 4.3 --
If "plain" does stay, why on Earth is it the default?  Even if just for
form's sake, shouldn't S256 be the default?

-- Section 4.4 --
The word "code" is used for too many things, and "Authorization Grant" 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".

-- Section 4.4.1 --
Please expand "PKCE", which is, at the moment, only expanded in the
document title.

-- 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.

-- 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 [email-masked] 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 [email-masked] 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

-- Section 7.2 --
Please rewrite the first paragraph.  Please do not leave it for the RFC
Editor, as they may inadvertently get it technically wrong when they try.

---

From: "Nat Sakimura" <[email-masked]>
Date: Wed, 10 Jun 2015 16:04:08 +0900

Thanks Barry for the comment.

My (personal) responses inline.
John and Naveen, if you have anything to add, please chime in.

>
> -----Original Message----- 
> From: Barry Leiba
> Sent: Tuesday, June 09, 2015 4:42 AM
> To: The IESG
> Cc: [email-masked] ; [email-masked] ; 
> [email-masked] ; [email-masked] ; 
> [email-masked]
> Subject: [OAUTH-WG] Barry Leiba's Discuss on draft-ietf-oauth-spop-12: 
> (with DISCUSS and COMMENT)
>

You have to understand that this spec deals with a scenario that the 
communication is conducted over two segments:
(1)    Unprotected: Within the machine through mechanisms like custom 
scheme.
(2)    Protected: Over the network, which is protected by TLS.
The “snagging” happens over the segment (1). For example, an attacker can 
snag the grant over this segment.
However, he cannot snag code verifier because it is sent over (2), which is 
TLS protected.

OAuth (RFC6749 and RFC6750) mandates the use of TLS over the network.
PKCE inherits this property. We could mention it again here, but it is sort 
of already done by inheritance.

> suggest removing the quotation marks when you mean to refer to the

That’s the peculiarity of the current XML2TXT converter.
In XML, it is <spanx style="verb"> to signify strings themselves rather than 
the values of the variables.
It renders nicely on HTML format etc., but TXT seem to have this confusing 
property.
Perhaps RFC Editor can deal with.

> I don't understand the distinction between STRING and ASCII(STRING).  Can

It is a notation used by JSON Web Signature, etc.
It is making sure not to conflate the sequence of characters with sequence 
of octets of characters.

> form's sake, shouldn't S256 be the default?

It is partly historical. The draft started off there, then kept adding other 
mechanisms. There are many implementations of PKCE now but the largest 
portion of it is using “plain”. For the sake of interoperability with 
them, it needs to stay as it is written.

> what you mean by the "code".

RFC 6749 defines two types of Authorization Grant: Authorization Code and 
Implicit. In PKCE, we are specifically dealing with Authorization Code so 
replacing them with Authorization Grant loosens it up and is not desirable. 
I agree that we have been a bit lax in the use of the term “code” as an 
abbreviation of “Authorization Code”. Also, looking at the TXT 
representation of the spec, largely due to the formatting peculiarity, it is 
making them even more confusing than compared to other format. Therefore, I 
suggest the following:

- Replace all occurrence of “code” as an abbreviation of “Authorization 
Code” with “Authorization Code”.
- Capitalize the defined term “code challenge” and “code verifier” so 
that there will become “Code Challenge” and “Code Verifier” throughout.
> Similarly, in Section 4.5 please say "code_verifier" rather than "secret".

Good catch. Accept.

> Please expand "PKCE", which is, at the moment, only expanded in the

Suggest the following:
Replace “this extension” of the last paragraph of 1. Introduction by 
“Proof Key for Code Exchange (PKCE) extension”.
Add “3.1 Abbreviation” subsection and collect the abbreviations that are 
used in this specification, namely:
- PKCE – Proof Key for (Authorization) Code Exchange
- Authz -- Authorization
- App – Application
- MITM – Man In The Middle

I guess we do not need to list IESG, IANA, and RFC in it…

> and just say that severs supporting backwards compatibility revert to the

Accept in principle, but at the same time, I feel that explicitly writing 
that the server is expected to fall back to RFC6749 and RFC6750 mode without 
error probably is useful for developers. For this purpose, this “SHOULD” 
while redundant, could be useful. Perhaps, just replacing “SHOULD” with 
“should” suffice?

>    and, if applicable, suggestions as to how to make the request

Thanks. The editors were just trying to be consistent with the way the 
parent spec was dealing with this kind of things, but if the suggested way 
is preferred, we can certainly do it.

> Editor, as they may inadvertently get it technically wrong when they try.
Would something like this work?

OLD
Clients MUST NOT try down grading the algorithm after trying S256 method. If 
the server is PKCE compliant, then S256 method works. If the server does not 
support PKCE, it does not generate error. Only the time that the server 
returns that it does not support S256 is there is a MITM trying the 
algorithm downgrade attack.
NEW
Clients MUST NOT down grade the algorithm after trying the S256 method. 
There is no legitimate case that such a downgrade become necessary since 
S256 method always works if the server supports PKCE and it does not 
generate error if the server does not. Only the time that the server returns 
error that it does not support S256 is that there is a MITM attacker trying 
the algorithm downgrade attack.

>
> _______________________________________________
> OAuth mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/oauth

---

From: Barry Leiba <[email-masked]>
Date: Thu, 11 Jun 2015 11:35:59 +0100

Hi, Nat, and thanks for the reply.

> However, he cannot snag code verifier because it is sent over (2), which is

Well, the code verifier isn't sent at all, as I understand the
protocol.  It's the transformed version, the challenge, that's sent.
Am I right?

Now, you appear to be telling me that the challenge (and method) that
are sent in Section 4.3 and the Authorization Response that is sent in
Section 4.4 are sent in different ways (though you don't say that in
the document), and that the transmission in 4.4 is open to
interception, but the transmission in 4.3 is not.  Is that an accurate
summary?

If it is correct that the transmission that's described in 4.3 can
*never* be intercepted, then I'm a bit less worried about "plain".

But given the simplicity of implementing S256:

1. If there's *any* possibility of interception in some scenarios, I
still strongly oppose having "plain" at all, because if it's there,
it's bound to be used when it shouldn't have been.

2. I don't buy the argument that it's OK to build a weaker protocol
because implementors think that weak is good enough.  I think we need
to use our best protocol design principles and make sure what we
publish as "standard" is what we think reflects the best protocol
design.

On to the less strenuous stuff...

> of already done by inheritance.

That's true.  I'll leave this to your judgment, and no need for
further discussion.  The reason I mentioned it was to make sure that
implementors don't think that this mechanism can be a substitute for
TLS.  But if you think the OAuth specs are already solid enough on the
TLS thing, I'm OK with that.

> Perhaps RFC Editor can deal with.

I don't understand that.  When you say, for example:

   "code_verifier" == "code_challenge"

and

   BASE64URL-ENCODE(SHA256(ASCII("code_verifier" )))
      == "code_challenge"

...you are not referring to the strings, but to the values, so why are
you marking them up at all?  They should just be this way:

   code_verifier == code_challenge

and

   BASE64URL-ENCODE(SHA256(code_verifier))
      == code_challenge

(see below about the "ASCII()" thing), with no markup causing quotation marks.

Or maybe I misunderstand what you're saying.  And, no, I don't think
we can leave it to the RFC Editor, unless we're quite explicit (with a
note in the document) about telling them exactly what we want them to
do.

> It is making sure not to conflate the sequence of characters with sequence

You already define "STRING" as a sequence of ASCII characters (and you
have "OCTETS" for arbitrary non-character octets).  ASCII is already a
character encoding.  So there's no need to say that you have to take
characters that are represented in ASCII, and represent them in ASCII.
It's redundant, and it adds clutter.

Of course, it's also not wrong, so this is not a DISCUSS, and you can
do as you think best.  I just suggest removing the "ASCII()"
throughout, as it's totally unnecessary, and, as I say, it adds
clutter.  I'll note that your definition of "ASCII(STRING)" is quite
convoluted, and the reason it's convoluted is that you have to twist
yourself around a pole to make definition that says anything other
than "ASCII(STRING) is just STRING".

> portion of it is using “plain”. For the sake of interoperability with them,
> it needs to stay as it is written.

In other words, if you change the default then existing
implementations will have to change to match.  I get that, but, again,
we shouldn't be making our protocols suboptimal for that reason.  I'd
rather see us eliminate "plain" altogether, so I won't argue this
further on this point.

> Code” with “Authorization Code”.

Perfect; thanks.

> - Capitalize the defined term “code challenge” and “code verifier” so that
> there will become “Code Challenge” and “Code Verifier” throughout.

I like that too.

> Replace “this extension” of the last paragraph of 1. Introduction by “Proof
> Key for Code Exchange (PKCE) extension”.

That works well; thanks.

> I guess we do not need to list IESG, IANA, and RFC in it…

That's correct.  The RFC Editor maintain a list of common
abbreviations, noting those that don't need expansion.  See
http://www.rfc-editor.org/rfc-style-guide/abbrev.expansion.txt
(You can get this from the main RFC Editor page by clicking on "RFC
Style Guide", and then "Abbreviations List".)

> while redundant, could be useful. Perhaps, just replacing “SHOULD” with

Again, not a DISCUSS, so use judgment.  I think even "should" is
totally unnecessary -- you're just saying how the protocol works.  If
you want compatibility, this is what you do.  Not what you "should"
do, but what you do in order to maintain compatibility.  But no
further discussion needed here.

> is preferred, we can certainly do it.

Thanks!

> the algorithm downgrade attack.

Better, but still a little awkward.  Let me take a stab and offer something:

NEWER
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

(And, of course, this isn't needed at all if we get rid of "plain".)

Barry

---

From: John Bradley <[email-masked]>
Date: Thu, 11 Jun 2015 05:45:01 -0700

Hi Barry,

The main attack and the origin of the spec is caused by a native app on the device overloading the intent URI/custom scheme 
that the legitimate OAuth client has registered as it’s redirect_uri.

The request path travels from the legitimate app to the system browser by a secure API call, and can’t be intercepted unless the attacker can replace the system browser.
The browser then sends the request to the Authorization endpoint over TLS per OAuth.  That again would require a TLS man in the middle to compromise and is no more a concern than any other use of TLS.

The AS formulates it’s response and sends it back to the browser over TLS as a 302.  That again is fine and not a special concern.
The attack comes when the browser uses the location header to call back that native app on the device.

Because of the nature of intent URI multiple apps can register the same scheme, so are eligible to receive the response.
On Android if there is more than one receiver registered the user is prompted for what app they want invoked. 
There is always a chance that the user will select the wrong app and the attacker gets the Authorization Code.

On iOS the selection behaviour depends on the version , in some versions it just random, others select the most recently run app.  
iOS 8 is random so an attacker has about a 50% chance of success in intercepting the Authorization Code.

The attack opportunity is only present in the response path, because the way that the OS expose the API call to invoke the system browser is diffrent from the way the system browser invokes the callback to a native app.

Given this we came up with the plain transform that requires no crypto that no developer has an excuse for not using all the time.

The code_challenge isn’t vulnerable to interception because it is sent with a different API in the device.   
The response containing the code has something like a 50% chance of interception in the response, but doesn’t contain the code_challenge.

This attack being against a OS API method and not on the wire is why we missed it in the OAuth spec.

The plain transform is effective against this attack as the communication of the code_challenge from the client to the AS is over a secure API and TLS.

The second attack is the one we don’t know about yet.  It may be a buffer leak in the OS or a weakness in the deployment that allows the system browser to use SSL with a weak 
alg that could be sniffed.   I just don’t know, but as most people realize that I am an exceptionally paranoid person 
(Your gov is really watching me, I saw the DHS powerpoint.  It is quite funny, nice to have friends.)

I insisted in including the S256 transform for all the other attacks, as it is not much more complicated and if you can protect against sniffing of the request then a reasonable person would do it.  The specs I am working on that use PKCE have S256 as a MUST.  

So I talked the WG and deployers into putting S256 into the core PKCE spec rather than having it as an extension as it was originally so that it could be MTI for servers to implement.

If we remove plain completely from the spec now, i may become the Google equivalent of disappeared, as them agreeing to put S256 in has lead to this.

Keeping plain but changing code_challenge_method if not present to the default of S256 will break existing deployments with PKCE standard clients, or all the current clients without deploying new authorization endpoints.  
That would not be good for adoption.

A compromise that might work for people would be making code_challenge_method required, and removing the default language.

That way we can say that the server MUST support S256, and that the client SHOULD use S256.

plain is for backwards compatibility, and for constrained environments that are unable to do SHA256.

If an attacker can modify the request, all they need to do is remove the code_challenge to get around this unless the client has used client registration to lock down a required 
code_challenge_method, so we are not introducing a downgrade attack by having plain that is not available to an attacker by removing PKCE entirely from the request.

Unfortunately this week is CIS for me and I am having a hard time keeping up with email.

Let me know what you think.  

I agree that we want to direct developers to S256 as the preference, but that only works if we get AS to deploy it, and developers to use it.

John B.

> On Jun 11, 2015, at 3:35 AM, Barry Leiba <[email-masked]> wrote:
> 
>>> Editor, as they may inadvertently get it technically wrong when they try

---

From: Barry Leiba <[email-masked]>
Date: Thu, 11 Jun 2015 13:54:56 +0100

John, thanks *very* much for this detailed background -- it's really
helpful, and particularly helpful to get it before the telechat
discussion.

Given what you say, I still prefer the "no plain" path, because the
inability to steal the challenge depends upon current OS
implementations.  That said, I'm much more open to accepting the
current situation with the understanding that S256 is the path
forward, and that plain is there to support existing use cases and
deployment.  It would be really good if some of this detail could be
put into the Introduction so that readers can understand (1) why we're
where we are and (2) why we should stop using plain for new
situations, and stick with S256.

So let's see where the discussion goes on the telechat.

Barry

---

From: Nat Sakimura <[email-masked]>
Date: Thu, 11 Jun 2015 22:16:36 +0900

Indeed, "plain" is for "on-boarding" the current implementations and
developers.
S256 is the way forward.

We added intro pretty late to describe what John stated "nicely".
Apparently it did not quite work as questions came around. We probably
should bolster the intro.

Nat

2015-06-11 21:54 GMT+09:00 Barry Leiba <[email-masked]>:

> > The main attack and the origin of the spec is caused by a native app on
> the device overloading the intent URI/custom scheme
> > The request path travels from the legitimate app to the system browser
> by a secure API call, and can’t be intercepted unless the attacker can
> replace the system browser.
> > The browser then sends the request to the Authorization endpoint over
> TLS per OAuth.  That again would require a TLS man in the middle to
> compromise and is no more a concern than any other use of TLS.
> >
> > The AS formulates it’s response and sends it back to the browser over
> TLS as a 302.  That again is fine and not a special concern.
> > Because of the nature of intent URI multiple apps can register the same
> scheme, so are eligible to receive the response.
> > On iOS the selection behaviour depends on the version , in some versions
> it just random, others select the most recently run app.
> > The attack opportunity is only present in the response path, because the
> way that the OS expose the API call to invoke the system browser is
> diffrent from the way the system browser invokes the callback to a native
> app.
> >
> > Given this we came up with the plain transform that requires no crypto
> that no developer has an excuse for not using all the time.
> >
> > The response containing the code has something like a 50% chance of
> interception in the response, but doesn’t contain the code_challenge.
> >
> > The plain transform is effective against this attack as the
> communication of the code_challenge from the client to the AS is over a
> secure API and TLS.
> >
> > The second attack is the one we don’t know about yet.  It may be a
> buffer leak in the OS or a weakness in the deployment that allows the
> system browser to use SSL with a weak
> > alg that could be sniffed.   I just don’t know, but as most people
> realize that I am an exceptionally paranoid person
> > I insisted in including the S256 transform for all the other attacks, as
> it is not much more complicated and if you can protect against sniffing of
> the request then a reasonable person would do it.  The specs I am working
> on that use PKCE have S256 as a MUST.
> >
> > So I talked the WG and deployers into putting S256 into the core PKCE
> spec rather than having it as an extension as it was originally so that it
> could be MTI for servers to implement.
> >
> > If we remove plain completely from the spec now, i may become the Google
> equivalent of disappeared, as them agreeing to put S256 in has lead to this.
> >
> > Keeping plain but changing code_challenge_method if not present to the
> default of S256 will break existing deployments with PKCE standard clients,
> or all the current clients without deploying new authorization endpoints.
> > A compromise that might work for people would be making
> code_challenge_method required, and removing the default language.
> >
> > If an attacker can modify the request, all they need to do is remove the
> code_challenge to get around this unless the client has used client
> registration to lock down a required
> > code_challenge_method, so we are not introducing a downgrade attack by
> having plain that is not available to an attacker by removing PKCE entirely
> from the request.
> >
> > I agree that we want to direct developers to S256 as the preference, but
> that only works if we get AS to deploy it, and developers to use it.
> >
> > John B.
> >
> >
> >
> >
> that

-- 
Nat Sakimura (=nat)
Chairman, OpenID Foundation
http://nat.sakimura.org/
@_nat_en