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

[Barry Leiba <[e-mail]>]

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

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