Subject: [Gen-art] Gen-ART Review of draft-ietf-oauth-spop-11

From: Alexey Melnikov <[email-masked]>
Date: Tue, 09 Jun 2015 13:05:20 +0100

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

This review is in response to a request for early Gen-ART review.

Document: draft-ietf-oauth-spop-11
Reviewer: Alexey Melnikov
Review Date: 2015-06-09
IETF LC End Date: 2015-06-01
IESG Telechat date: 2015-06-11

Summary: Almost Ready

Major Concerns:

What is the justification for having the "plain" verifier? If one is 
intercepted by a malicious application, your extension becomes pointless.

Minor Concerns:

Is code_challenge_method missing in 4.5?

Sections 7.1 and 7.3 are talking about the same thing? Should they be 
merged into one? If they are not talking about the same thing, should 
they be named differently?

Nits:

4.2.  Client creates the code challenge

    The client then creates a code challenge, "code_challenge", derived
    from the "code_verifier" by using one of the following
    transformations on the "code_verifier":

    plain  "code_challenge" = "code_verifier"
    S256  "code_challenge" = BASE64URL-

       ENCODE(SHA256(ASCII("code_verifier")))
    It is RECOMMENDED to use the S256 transformation when possible.
    ABNF for "code_challenge" is as follows.

    code-challenge = 43*128unreserved
    unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
    ALPHA = %x41-5A / %x61-7A
    DIGIT = %x30-39

SHA-256 requires a normative reference.

---

From: Barry Leiba <[email-masked]>
Date: Tue, 9 Jun 2015 13:13:14 +0100

> intercepted by a malicious application, your extension becomes pointless.

Yes, this is what my DISCUSS is about.  I'm glad to have support for that view.

> Is code_challenge_method missing in 4.5?

No.  The challenge method is set when the challenge is sent and
recorded (4.2), and not when the verifier is sent later.  If the
method was sent in 4.5, it would open a hole in the protocol (at least
as long as "plain" exists).

Barry

---

From: Alexey Melnikov <[email-masked]>
Date: Tue, 09 Jun 2015 13:20:41 +0100

> Yes, this is what my DISCUSS is about.  I'm glad to have support for that view.
Very much so. I think S256 should be the default or the only choice.
> as long as "plain" exists).
But the document doesn't say that the code_challenge_method is embedded 
in the token somehow. Or did I miss it?

---

From: Barry Leiba <[email-masked]>
Date: Tue, 9 Jun 2015 13:40:56 +0100

> But the document doesn't say that the code_challenge_method is embedded in
> the token somehow. Or did I miss it?

The challenge and the challenge method are given in 4.2, and both need
to be remembered by the server.  When the verifier is presented in
4.5, the server uses the remembered method to transform the verifier
into the challenge, and then compares it with the remembered
challenge.  We don't need nor want the method to be presented again at
that point, and because the client isn't doing the transform at that
step, it doesn't need to know the method.

Barry

---

From: John Bradley <[email-masked]>
Date: Tue, 9 Jun 2015 06:04:12 -0700

Thanks for the review.

PKCE started out only supporting plain, and that is what most current deployments are using.
That has the lowest implementation for client developers.

In the majority of situations (the ones currently exploited) on iOS the attacker only has access to the response coming back from the
AS and not the request with the code_challenge.  This is to do with the way custom scheme redirects work in the OS.

Knowing that for not much more work we can protect the code_challenge as well.  We made that Mandatory to implement on the server side,
and SHOULD for clients. 

A client opting to do plain is still getting significant protection and the choice is based on there risk tolerance.

The feedback was that there are some developers that would rather ignore the risk than so S256.

If we didn’t have to deal with the pesky developers then I would agree with you that we should not allow plain.

I fixed that issue in 7.1 and refactored the security considerations text based on feedback from Ben on Sunday.

Let me know if Draft 11 fixes the issue for you.

I will take care of the normative reference for SHA256 in the next update.  Thanks for noticing.

I am at The Cloud Identity Summit (CIS) all week so my responses may be spotty until next week.

Regards
John B.

> On Jun 9, 2015, at 5:05 AM, Alexey Melnikov <[email-masked]> wrote:
> 
> What is the justification for having the "plain" verifier? If one is intercepted by a malicious application, your extension becomes pointless.
> 
> 
> Minor Concerns:
> 
> Sections 7.1 and 7.3 are talking about the same thing? Should they be merged into one? If they are not talking about the same thing, should they be named differently?
> 
> 
> Nits:
> 
>

---

From: Barry Leiba <[email-masked]>
Date: Tue, 9 Jun 2015 14:11:28 +0100

> In the majority of situations (the ones currently exploited) on iOS the
> attacker only has access to the response coming back from the
> AS and not the request with the code_challenge.  This is to do with
> the way custom scheme redirects work in the OS.

Ack; thanks for that explanation.

> The feedback was that there are some developers that would rather
> ignore the risk than do S256.

I find that curious, as it's very little code involved.

I'm still not certain that we should publish a standard protocol that
allows this weakness, rather than requiring that it be robust.

But I'll let my Sec AD colleagues weigh in, and will go with their
opinions on it.

Barry

---

From: John Bradley <[email-masked]>
Date: Tue, 9 Jun 2015 07:04:05 -0700

Keeping the plain for clients  option was part of getting some of the larger players like Google, and DT to implement it.

For known attacks plain is fine. 

As a datapoint I had one of the Azure people tall me last night that they get push back by developers on doing the base64url 
encoding.   Nothing surprises me about developers. 

There are some extensions to other specs that may use PKCE where S256 would be required,
so having it as MTI on the server is a good thing.

John B.

> On Jun 9, 2015, at 6:11 AM, Barry Leiba <[email-masked]> wrote:
>

---

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

FYI, at the very beginning of the work, we suggested developers to go with 
at least S256, but the response we got was flatly "NOT". As John has 
written, "plain" mitigates the attack they are facing and there was no 
appetite to go any further... It is a bit unfortunate, but we would have to 
face the reality and make the life better a step at a time. It certainly is 
better to have them doing it in a standardized way than have them in the 
proprietary silos.

Nat

-----Original Message----- 
From: John Bradley
Sent: Tuesday, June 09, 2015 11:04 PM
To: Barry Leiba
Cc: Alexey Melnikov ; IETF Gen-ART ; [email-masked] ; 
The IESG
Subject: Re: Gen-ART Review of draft-ietf-oauth-spop-11

Keeping the plain for clients  option was part of getting some of the larger 
players like Google, and DT to implement it.

As a datapoint I had one of the Azure people tall me last night that they 
get push back by developers on doing the base64url
encoding.   Nothing surprises me about developers.

There are some extensions to other specs that may use PKCE where S256 would 
be required,
so having it as MTI on the server is a good thing.

John B.

---

From: Jari Arkko <[email-masked]>
Date: Thu, 11 Jun 2015 13:00:02 +0300

Thanks for your review, Alexey. I have followed the discussion about
plain with interest, but will leave holding the discuss to Barry.

John, it was not clear to me if you had done everything you needed to with
respect to Alexey’s other comments (reproduced below). Have you?

> Sections 7.1 and 7.3 are talking about the same thing? Should they be merged into one? If they are not talking about the same thing, should they be named differently?

Jari

---

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

I believe that the other comments were all-ready addressed in draft 12 as a result of Ben Laurie’s comments from last week.

If Alexey can double check that I got everything that would be great.  I re-worked those sections to capture Ben’s comments.

John B.

> On Jun 11, 2015, at 3:00 AM, Jari Arkko <[email-masked]> wrote:
>

---

From: Alexey Melnikov <[email-masked]>
Date: Thu, 11 Jun 2015 13:57:08 +0100

Hi John,

> If Alexey can double check that I got everything that would be great.  I re-worked those sections to capture Ben’s comments.
My comment on 7.1/7.3 is addressed.

Can you elaborate a bit on why  code_challenge_method is not needed in 4.5?

---

From: Jari Arkko <[email-masked]>
Date: Thu, 11 Jun 2015 15:58:10 +0300

Thanks, John & Alexey!

Jari

---

From: John Bradley <[email-masked]>
Date: Thu, 11 Jun 2015 06:40:18 -0700

The idea is that the communication in the first request request to the authorization endpoint is via a secure path that the attacker can’t modify.

That is where the code_challenge and code_challenge_method are bound to the code.  we expect that will be done by creating some extra columns a internal DB that is used to store whatever grants the AS is attaching to the code , client_id, and redirect_uri used.   The AS needs all of those things for standard OAuth, so we are just adding two new things fro them to track.

It is the response from the AS to the client that gets hijacked.

If a bad client has the code, you don’t really want it to be able to downgrade the code_challenge_method.   

Sending code_challenge_method to the token endpoint would require that it be matched against the one stored in the db at the AS, so best case it is extra stuff to send with no value, worst case would be that a AS ignores the one from the authorization request and acts on the one to the token endpoint allowing a downgrade attack.

The downgrade attack to plain is still impractical if they haven’t captured the authorization request, but that is no reason to do more work for less security.

I hope that helps explain the logic.

Regards
John B.

> On Jun 11, 2015, at 5:57 AM, Alexey Melnikov <[email-masked]> wrote:
> 
> Hi John,
>

---

From: Alexey Melnikov <[email-masked]>
Date: Thu, 11 Jun 2015 14:45:59 +0100

Hi John,

> I hope that helps explain the logic.
This is fine, but I wish you've said that in the document! This was not 
clear.

---

From: John Bradley <[email-masked]>
Date: Thu, 11 Jun 2015 06:49:14 -0700

Thanks,

Thats why we have these reviews to help us poor authors get the stuff in our heads down in the specs, if it is not clear.

Once we have all the feedback from the reviews, I will see how I can work this back into the non normative text.

John B.

> On Jun 11, 2015, at 6:45 AM, Alexey Melnikov <[email-masked]> wrote:
> 
> Hi John,
> 
> This is fine, but I wish you've said that in the document! This was not clear.
>> Regards
>> John B.
>>