Re: [secdir] secdir review of draft-ietf-oauth-device-flow

[William Denniss <[e-mail]>]

On Sun, Aug 26, 2018 at 7:30 PM Christopher Wood <
[e-mail]> wrote:

> Hi William,
>
> Please see inline below.
>
> On Wed, Aug 1, 2018 at 5:03 PM William Denniss <[e-mail]>
> wrote:
>
>>
>> On Tue, Jun 12, 2018 at 5:55 PM, Christopher Wood <
>> [e-mail]> wrote:
>>
>>> Hello,
>>>
>>> I have reviewed this document as part of the security directorate's
>>> ongoing effort to review all IETF documents being processed by the
>>> IESG.  These comments were written primarily for the benefit of the
>>> security area directors.  Document editors and WG chairs should treat
>>> these comments just like any other last call comments.
>>>
>>>   The summary of my review is: Ready with nits.
>>>
>>> Overall, the document is in fine shape. I have a few general comments
>>> (not quite nits,
>>> though not quite issues either), listed below:
>>>
>>> - Section 3.5, fifth paragraph: Requiring clients to poll at a
>>> “reasonable” polling interval
>>> without a suggestion of what is reasonable seems strange. Could you
>>> suggest a value that’s
>>> within reason, e.g., every second?
>>>
>>
>> We documented a default of 5s in version 12.
>>
>>
>>> - Section 5.1, first paragraph: It might be useful to point to Section
>>> 6.1 wherein User Code
>>> generation is discussed. Right now minimum entropy “requirements” are
>>> listed without further
>>> details regarding viable mechanisms.
>>>
>>
>> The authors are still considering this feedback, along with Benjamin
>> Kaduk's DISCUSS.
>>
>>
>>> - Section 5.2, second paragraph: The text claims that an end user would
>>> end up “on the
>>> authorization page of the wrong service.” Can you provide more details
>>> here? What stops
>>> the malicious MITM from serving an authorization page that’s
>>> indistinguishable from the
>>> legitimate service page?
>>
>>
>>
>>> - Section 5.3, first paragraph: How specifically does the authorization
>>> service prevent
>>> devices from lying when providing “information about the device”? Or,
>>> alternatively, how
>>> does the authorization service learn this information?
>>>
>>
>> These 2 also pending.
>>
>>
>>> - Section 5.4: Would it be useful to suggest that clients SHOULD use a
>>> secure (encrypted
>>> and authenticated) channel when communicating to the user device?
>>>
>>
>> This section is actually referring to real-world spying, i.e. someone in
>> the same room as you who can see the TV. Perhaps we need to make that more
>> clear?
>>
>
> That might help. It seems to be only a matter of clarity, not correctness.
>

Thank you for the suggestion, a clarification has been added and will be
published in -14.

As currently drafted it now reads:

            While the device is pending authorization, it may be possible
for a
            malicious user to physically spy on the device user interface
            (by viewing the screen on which it's displayed, for example)
and hijack the
            session by completing the authorization faster than the user
that
            initiated it.


>
>>
>>>
>>> The remainder of my comments, listed below, are editorial in nature,
>>> aimed towards improving
>>> readability of the document.
>>>
>>> - Section 1, step (E): This is the first time client polling is
>>> mentioned without
>>> discussion of timeouts or server-generated errors. The draft provides
>>> such details later
>>> on, so it would be helpful to allude or point to them here.
>>>
>>
>> I believe this is covered in detail in the document, this section is
>> intending to just be a high-level overview.
>>
>>
>>> - Section 3.3, second paragraph: Please cite TLS upon use (“… in a
>>> secure TLS-protected
>>> session.”).
>>>
>>
>> Done, thanks!
>>
>>
>>> - Section 3.3, second paragraph: The text suggests that the server
>>> informs the user to
>>> “return to their device.” Perhaps this should be prefaced with a MAY, as
>>> the client will
>>> eventually learn that authorization is complete upon polling.
>>>
>>
>> MAY was added, thanks!
>>
>>
>>> - Section 3.3.1, first paragraph: Should it be required that
>>> “verification_uri_complete”
>>> is constructed in part from the “verification_uri” and “user_code”? I’m
>>> not sure this is
>>> necessary, though the example given is constructed this way. If not
>>> required, this might be
>>> worth noting.
>>>
>>
>> The working group considered this, in fact originally we were not going
>> to have verification_uri_complete and it would just be defined as a
>> composition of those two values. In the end, the work group decided to make
>> it separate. The authorization server can combine them to create the
>> complete verification URI, or may use something else.
>>
>> The text currently states the following which I believe covers this:
>>
>> "A verification URI that includes the "user_code" (or
>> other information with the same function as the "user_code"),
>> designed for non-textual transmission."
>>
>> - Section 3.5, first paragraph: s/token endpoint/authentication server?
>>>
>>
>> This section does actually relate to the token endpoint.
>>
>
> Ah, okay. I misunderstood. Thanks!
>
>
>>
>>> - Section 5.1, third paragraph: This text is mostly redundant with the
>>> preceding paragraphs.
>>> I would remove or merge it with the paragraphs above.
>>>
>>
>> I don't agree that it's redundant. Willing to review text if you have a
>> concrete proposal.
>>
>
> My only recommendation is to remove the paragraph. I think it’s already
> covered by the preceding paragraphs. That said, I don’t think this is
> necessary. It’s merely a suggestion.
>
>
Note that this paragraph was significantly reworked in -13 and has a longer
discussion on entropy now.


>
>>
>> Please let me know if you’ve further questions, comments, or concerns. I
>>> hope this helps.
>>
>>
> It does indeed. Thanks for your changes!
>
> Best,
> Chris
>

Thanks again for your review!

William