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

[John Bradley <[e-mail]>]

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 <[e-mail]> wrote:
> 
> Hi, Nat, and thanks for the reply.
> 
>>> 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?
>> 
>> 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.
> 
> 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...
> 
>>> 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?
>> 
>> 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.
> 
> 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.
> 
>>> 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.
>> 
>> 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 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.
> 
>>> -- Section 2 --
>>> I don't understand the distinction between STRING and ASCII(STRING).  Can
>>> you please explain it?
>> 
>> 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.
> 
> 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".
> 
>>> -- 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?
>> 
>> 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.
> 
> 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.
> 
>>> -- 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".
>> 
>> 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”.
> 
> 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.
> 
>>> -- Section 4.4.1 --
>>> Please expand "PKCE", which is, at the moment, only expanded in the
>>> document title.
>> 
>> 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
> 
> 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".)
> 
>>> -- 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.
>> 
>> 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?
> 
> 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.
> 
>>> -- 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
>> 
>> 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.
> 
> Thanks!
> 
>>> -- 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