[apps-discuss] Apps Area review of draft-ietf-oauth-v2-threatmodel-01

[Tim Bray <[e-mail]>]

[pardon resend, must have cut/pasted suggested distribution wrong]

I have been selected as the Applications Area Directorate reviewer for
this draft (for background on appsdir, please see
http://trac.tools.ietf.org/area/app/trac/wiki/ApplicationsAreaDirectorate).

Please resolve these comments along with any other Last Call comments
you may receive. Please wait for direction from your document shepherd
or AD before posting a new version of the draft.

Document: draft-ietf-oauth-v2-threatmodel-01
Title:  OAuth 2.0 Threat Model and Security Considerations
Reviewer: Tim Bray
Review Date:  Jan 23, 2012

Summary: This needs some more editorial work, but is basically sound.
It's not clear, though, whether it wants to be an Informational RFC or
not; the use of RFC2119 language needs special attention.  I think a
few of the "minor issues" are worthy of a little bit more work in
another draft.

Major Issues:

The use of 2119 MUST/SHOULD/etc doesn't seem fully thought through.  I
normally wouldn't expect a "threat model" to include normative text.
The basic idea would be to say "Here is an enumeration of the threats,
and here are the tools available to OAUTH2 implementors to meet them."
 I was impressed by the enumeration, which seemed very complete and
well thought through. But the usage of 2119, which makes statements
normative, seems inconsistent.  I can think of 2 ways to address this:

1. Remove all the 2119 words, so this document isn't normative, and
publish it as an Informational RFC
2. Go through and clean up the 2119 language so it's used
consistently; then this becomes a normative document.

This is going to affect the references to this document from other
I-Ds in the OAuth suite, which are currently in last call.

Here are all the section-numbered notes enumerating my issues around
2119, as I encountered them:

Section 2.3, I'm a little confused about the use of RFC2119 MAY in a
threat analysis.  When you say "The following data elements MAY be
stored or accessible...", Is this saying that "The OAuth2 RFC says
that the following data elements MAY be..." or is it saying something
else. I don't think there's anything seriously wrong here, but a bit
more explanation might be in order.  I note a comparative absence of
2119-ese in section 5 describing countermeasures, where one would
expect to find it.
Also in 4.3.1, first bullet point, there's "Authorization servers MUST..."
Also in: 4.4.1.1, 4.4.1.6, 4.4.1.12, 4.6.*, 5.1.4.1.5, 5.1.5.11
Related: "SHALL"?! in 4.6.3
Adding to the concern, there is use of lower-case "must"; note 2nd &
3rd bullet points in 4.4.3, which use "MUST" and "must" respectively.

Minor Issues:

4.1.2 first attack: It says "An attack may obtain the refresh tokens
issued to a web server client." This needs to be clearer... a "Web
server client" can be a browser or a native app.  Do you mean, "the
refresh tokens issued by the web server to multiple clients?"

4.1.2 last attack.  In the case where a device is cloned, wouldn't
"Utilize device lock to prevent unauthorized device access" still be a
countermeasure?  In many devices, such cloning would carry along the
user's device-lock settings.

4.4.1.4 2nd bullet.  The explanation of why this wouldn't work for
native clients wasn't comprehensible to me.  I'm suspicious of any
such claims because I can emulate most things a browser can do in a
mobile client.  Perhaps this would be obvious to someone who is an
OAuth2 implementor.

4.4.1.9 I think where it says "iFrame" it might mean "WebView", i.e. a
Web Browser control embedded in the native app.  If that's not what it
means, I don't understand what it's saying.  If this is true, then the
second bullet point is probably wrong.

4.6.6 1st bullet.  I'm not convinced that the Cache-Control header
will *ensure* that a client protects information properly.  Should say
something like "minimize the risk that authenticated content is not
protected"

5.* The enumeration, for some but not all of the countermeasures in
this section, of the threats against which this is a countermeasure,
reduces readability and, unless it's generated automatically from the
underlying source, is redundant information, which is unlikely to be
consistent between sections 4 and 5, and adds difficulty to
maintenance of this document without adding much value.  I'd just wipe
all these bullet lists out.  If it's generated automatically it's less
damaging, but still reduces readability.  In the current draft, this
is there for some countermeasures but absent for others.  Another good
reason to just take it out.

5.2.2.5 Device identifiers are very tricky.  It's correct that IMEI is
not adequate, but there are ways to do it without SMS.  For more, see
http://android-developers.blogspot.com/2011/03/identifying-app-installations.html

5.3.4 Surely a little more could be said about device lock.  On a
typical modern phone, "device lock" options include PINs, passwords,
"face recognition" and so on.  These are *not* equal in their level of
security they provide.

Nits:

Formatting is lousy.  There are notations, including ** and _whatever_
that I'm not familiar with in the RFC context.

Section 1.0: s/in-built into/built into/
2.1, last bullet point: "An example could by a..." s/by/be/
2.2, 1st bullet point s/eaves drop/eavesdrop/
2.3, 1st para, s/treat/threat/
2.3.1, last bullet, "per authorization process".  Adjectival phrases
should be hyphenated: "per-authorization process"
2.3.3, last bullet, ditto
3.1, 1st para, "all kinds of tokens" should be "many kinds of tokens"
3.1, 2nd para, should be ; not , after "within the authorization server"
      s/protected/protect/
      s/different system/different systems/
3.4 1st para, s/intermediary/intermediate/
      list item 1. s/short-living/short-lived/
3.5 s/malicious client/malicious clients/
3.7 top of page 12, what is the underscore notation _client_id_ mean?
I'm not familiar with this in the RFC context.
 1st bullet point: s/token/token's/
 2nd bullet point, multiple issues, 1st sentence should be " the
initial authorization and issuance of a token by an end-user
     to a particular client, and subsequent requests by this client to
     obtain tokens without user consent (automatic processing of repeated
     authorization)
 halfway down page 13, s/insures/ensures/
             s/validates the clients/validates the client's/
4. first sentence, s/this sections/this section/
4.1.2 first para, the last sentence is confusing. How about: "Before
enumerating the threats, here are some generally applicable
countermeasures:"
4.2.4 2nd bullet s/could not be/can not be/
4.3.3 1st bullet, capitalized phrase "Confidentiality of Requests" - I
assume that's supposed to be a hyperlink to one of the 5.* sections?
4.4.1.1 last bullet, s/referee/referrer/ - also, should note that the
referrer header may contain an Authorization code in a ?a=b style
argument
4.4.1.2 first bullet, "can be employed" is inconsistent with style of
rest of doc
4.4.1.3 first 2 bullets have un-labeled links.
4.4.1.4 1st bullet s/authentication/authenticate/
4.4.1.4 2nd bullet s/mean/means/
4.4.1.7 2nd bullet s/tokens/token's/
4.4.1.10, 2nd para, s/requisiete/requisite/ s/embbed/embed/
4.4.1.10, 3rd bullet, s/aibility/ability/
4.4.1.10, toward bottom of page 30, s/e.t.c./etc./
4.4.1.12 I think the href to semicomplete.com needs to be turned into
an IETF-style reference
4.4.2 " since HTTP user agents do not send fragments server requests."
What you mean to say is "Since HTTP user agents do not send the
fragment part of URIs to HTTP servers."
4.4.2.2 s/browser/browser's/
5.1.4.1.3 s/consider to not store/refrain from storing/
5.* s/may consider to $(verb)/may consider $(verb)ing/
5.1.6 Needs some sort of sentence structure
5.3.2 Needs some sort of sentence structure; or is this intended just
to be a title, with 5.3.3 etc nested under it?