Subject: [OAUTH-WG] AD review of draft-ietf-oauth-threatmodel-05

From: Stephen Farrell <[email-masked]>
Date: Mon, 28 May 2012 19:34:38 +0100

Hi all,

I've gotten the publication request for oauth-threatmodel
so here's my AD review of -05.

Its quite a read (and a good one) but I've a bunch of
questions. Some of these will need fixing I suspect
but a lot are ok to fix later after IETF LC, depending
on whether the authors want to re-spin it before then
or not. But I'd like to at least see reactions to the
questions before I start IETF LC.

Although there are many many nits and typos, those
don't actually make the document unreadable so
though I'd prefer to see 'em fixed now, I'm ok with
that happening later if its a problem to get it
all done now.

If you want to argue for going ahead with IETF LC
now please do so, but I suspect that this might need
a revised ID to fix at least a couple of the points
raised below. If nobody does argue to go ahead now,
I'll mark it as revised ID needed, but first let's
see what the answers are to the questions.

Cheers,
S.

(1) s/RFC1750/RFC4086/ is needed as noted in the write-up.

(2) You don't say anything about the probability of
occurrence of the various threats. I realise that you
can't be precise but it seems wrong to say nothing.  Would
it be worth at least saying that that's not done here and
that readers of this document need to do their own risk
analysis including that aspect?

(3) Many deployments will use TLS accelerators.  That
means that TLS isn't fully e2e, and that opens up some
(mainly) insider attacks or attacks that can be launched
from within a compromised DMZ, but from outside the server
applications. Does that need a mention somewhere? (I've
seen systems like that deployed and a lot could go wrong
from the inside, so I think this is a real threat.)

(4) Could you use just one of "client identity" or "client
identifier" consistently? I'd much prefer the latter,
which has also been the outcome of various discussions on
this topic elsewhere. For example you say "revocation of
such an identity" at the end of p13, and that's a
potential rathole, better to say "revocation of the rights
associated with a client identifier" or similar I think.
And similar changes throughout.

(5) 4.4.2.2: Here you recommend native applications should
use an embedded browser, but earlier you said that was a
bad idea. I think you need to be consistent or else
provide more about when its ok to embed and when its not.
Did I misread it or does that need a change?

(6) 4.4.3.1: This calls for "obfuscation" of passwords in
logs. I think you ought be stronger there and call for
strong encryption of passwords wherever they are stored,
be that logs or DBs or whatever. Would'nt that be reasonable?

(7) 4.6.4: 1st countermeasure: I don't think you mean
address here (in the sense of an IP address, which is what
that means) but rather the domain name or FQDN or URL.  In
any case, you need to say what is meant clearly.  (Also in
5.1.5.6 and elsewhere when you use the term "address".)

(8) 4.6.6: You say to use Cache-Control, but don't say
how.  Is more needed really? Maybe there's a good document
you could reference for that? (I'm not suggesting you add
a lecture on caching btw:-)

(9) 5.1.1: needs a reference to RFC 5246 (and better to
just say TLS and not say SSL, at least for me :-)

(10) 5.1.1: needs a reference to whatever you mean by
"VPN" since there are many types of VPN. I assume you mean
an IPsec VPN? But even if not, saying more would be good.

(11) 5.1.2: needs a reference to RFC 5280 and/or RFC 6125
and/or RFC 2818. Bascially, you need to say how this is
done (by reference).

(12) 5.1.4.1: Isn't there some good reference you can give
here? (Having said that, I'd have to go look myself, but
I'd maybe start with owasp or sans.)

(13) 5.1.4.2.2: if p(collision) should be <=2^(-160) then
what's the point of saying it ought be <= 2^(-128)? Also
if sha-1 were perfect, then the 160 bits output would
really have a collision probability of about 2^(-80) with
many many tokens, but not 2^(-160). I think you need
to fix something there.  Perhaps you're really saying that
all high-entropy secrets should be >=128 bits long and
constructed with a good (P)RNG? If so, saying so more
clearly would be better. Not everyone will get the
collision probability way of saying that even when its
properly stated. (This text is also repeated, be better if
you just said this once I think.)

(14) 5.1.5.2: what is a "reasonable duration" - I don't
think its ok to say that but nothing else. Can't you give
some "reasonable" examples based on current usage?

(15) 5.1.5.5: needs a reference to SAML assertions with
the current text.

(16) 5.2.2.3: this describes a refresh token rotation
scheme that I don't recall being discussed on the list, so
this is just to check that that rotation scheme, as
described, doesn't ring any alarms bells for the WG. If
not, that's fine. And if it was discussed on the list and
I've forgotten, then sorry about that:-)

(17) 5.2.2.5: Using IMEI's like this has privacy
implications that I think you ought call out. A single
sentence and a reference to something that says "be
careful about privacy here" would be fine I'd say. (And
you need a reference for "IMEI" and to expand the term.)

(18) 5.2.2.6: needs a reference for X-FRAME-OPTION.
There's a websec draft about that I think.

(19) 5.2.3.4: what do you mean when you say "for different
deployments of a client"? That could be one secret per
install or one secret for all customers of a mobile
operator and those are radically different. I think you
need to be clear and hope you mean the former. That's
almost cleared up in the 3rd paragraph of the section but
I wanted to just check. Not sure "deployment" is the best
term there really - what's wrong with "installation"?

(many:-) nits and typos:

2.3.1: maybe explain "handle-based design" or give a
reference? (Or maybe just a forward ref to 3.1?)

2.3.3: It might be worth re-iterating that the term
"client" in oauth is used differently, e.g.  by copying a
bit of text from the base spec. I can see folks being
confused by this otherwise.

3.1: "is digitally signed" - do you mean it has data
integrity and origin authentication?  If MACs are commonly
used (or maybe authenticated encryption), and not
necessarily signatures, then saying that would be better.

3.1.2: typo: s/this mechanisms/this mechanism/

3.1.2: maybe s/Expires_In/expires_in/ to match the case in
the base spec? I think it'd be better not to capitalise
this in case it finds its way into someone's code. You
could also use "Expires In" in the title and then say that
its "expires_in" in the protocol. Might be worth doing
something generic to call out when you're talking about
specific things from the protocol, e.g. use a convention
like ``expires_in'' or "expires_in" consistently and say
that in the intro.

3.4: typo: s/the later/the latter/

3.4: bullet item 1 isn't really a reason for anything but
a downside of doing this, at least as written. Maybe this
needs to be tweaked?

3.6: expand CSRF on 1st use and maybe give a reference
CSRF is expanded in 4.4.1.8 which is a good bit later,
and also in 4.4.2.5 which could presumably be
shortened a bit by removing the repetition.

3.7: typo: s/collage associated request/collate associated
requests/

3.7: Maybe give a pointer to the definition of "native
application" in the base spec (Its section 9 of that.)
2nd last para of p13 would be a good place for that.

section 4: maybe s/Security Threat Model/Threat Model/
in the section title - what other kind of threat
model is there?

section 4: I wondered how we know the threat model
is "comprehensive"? Perhaps "detailed" is better?

4.2.1: typo: s/Users/users/g unless you mean something
special?

4.2.2: typo: s/a understandable/an understandable/

4.2.2: "...based on who the client is." is unclear
and not gramatically nice:-) "...based on the client
identifier." would seem better.

4.3.1: typo? s/on transit/in transit/ Or did you
mean something else? and s/may attempts/may attempt/

4.3.3: maybe s/Revelation/Disclosure/ to be a tad
less biblical:-)

4.3.3: typo? 1st countermeasure reads oddly and
refers to a different place than other references
to TLS - is that correct?

4.3.3: digest auth is nearly the same as sending
credentials over the wire, TLS client auth based
on certificates would be a better example, even
if its not often done.

4.3.4: 4.3.2 points to 5.1.4.1.2 but this doesn't.
Maybe it ought to?

4.3.6: typo s/an authorization servers/an authorization
server/

4.3.6: 4.3.5 refers to 5.1.4.2.2 wrt entropy. Is there
a reason to not do that here too?

4.4: typo? s/tokens endpoint/token endpoint/ ?

4.4.1.1: typo: s/by different ways/in different ways/

4.4.1.1: typo-fix-typo? HTTP has a "Referer" header field,
but you fixed their typo here - might be better to live
with the bad spelling, in which case
s/referrer/referer/g;-)

4.4.1.1: Is there no better way to reference these
OASIS docs? More importantly, is there no better (more
stable) reference than thomasgross.net for the
PDF you reference? Tidying this up would be good.

4.4.1.1 and elsewhere: a few times you say "such as
TLS or SSL," I think it'd be better to just say TLS
there and do it consistently everwhere. In other
places (e.g. 4.4.1.5) you say "HTTPS" - again that'd
be better done consistently.

4.4.1.1: typo: s/redeem a authorization code/redeem
an authorizatio code/

4.4.1.4: "counterfeit a valid client" reads oddly,
do you mean "pretend to be a valid client"? If so,
I think that'd be clearer.

4.4.1.4: "and to prevent unauthorized access to
them" - I think you might add "via the oauth
protocol" there since the AS isn't responsible for
all possible ways of preventing unauthorized access.

4.4.1.4: typo: s/not neccesserily indicates/doesn't
necessarily indicate/

4.4.1.4: typo: s/user should be explained the purpose/
something better/ :-)

4.4.1.4: expand/define CAPTCHA on 1st use. Give a
reference for this too. Especially since you also
have 5.1.4.2.5, which is maybe the best place for
the reference.

4.4.1.4: isn't a PIN code another user authentiation?
Seems like a bad example of automatic authentication,
since it isn't automatic if the user has to enter a
PIN.

4.4.1.6: Is Facebook a trademark? Maybe better to not
use that if so?

4.4.1.7: typo: s/achieve that goal/achieves that goal/

4.4.1.7: typo: s/victims resources/victim's resources/

4.4.1.7: typo: s/The attackers gains/The attacker gains/

4.4.1.7: typo: s/then the target web site/rather than
the target web site/

4.4.1.7: "session fixation" could do with a reference
or definition, and that might be better earlier in
this section and not just in the countermeasures
part.

4.4.1.7: typo: s/kind of attacks/kind of attack/

4.4.1.8: typo: s/not follow untrusted/to not follow
untrusted/

4.4.1.9: maybe add a reference for "iframe"? And
you say "iFrames" later, better to be consistent.

4.4.1.9: 1st countermeasure - do you mean end-user
authorization here or end-user authentication? And
would it be better to say "system browser" or
something rather than "external browser"? (I forget
what phrase you used for that earlier but there
was something bettter:-)

4.4.1.9: "javascript framebusting" really needs
a reference

4.4.1.10: typo: s/the victims resources/the victim's
resources/

4.4.1.10: typo: s/or split the/or splits the/

4.4.1.10: "corresponding form post requests" isn't
clear to me - does that need rephrasing or is it
just me?

4.4.1.10: this is the first mention of cookies, which
I found surprising, but that's all;-)

4.4.1.10: the 2nd "ways to achieve this" bullet is
a bit unclear - which "It" and whose browser? I
think this could be clearer.

4.4.1.10: typo: s/as native app/as a native application/

4.4.1.10: what does "assume" the threat mean?

4.4.1.10: typo: s/an user interaction/a user interaction/

4.4.1.10: typo: s/CAPTCAs, or/CAPTCHAs/ or else get
rid of the "or" from the last bullet

4.4.1.10: typo: s/send out of bound/sent out of band/

4.4.1.10: typo: s/instance message/instant message/

4.4.1.11: typo? s/directing user(s) browser/directing
the user's browser/ ?

4.4.1.11: I don't get the explanation here. Are you
assuming (but not saying) that generating non-trivial
entropy is a slow process? (e.g. /dev/random blocking?)
Its also not clear what "pool" you mean? This probably
needs a bit of tweaking.

4.4.1.12: semicomplete.com may not be a sufficiently
stable reference - can't you find a better one?

4.4.1.12: typo: s/Defenses such rate limiting/Defenses
such as rate limiting/

4.4.1.12: typo: s/an anonymizing systems/an anonymizing
system/

4.4.1.12: nicest typo yet! s/annoying system/anonymizing
system/ :-)

4.4.1.12: typo? maybe s/iframe/iFrame/ again?

4.4.1.12: 1st reference to "the CSRF token" here? That
might warrant a reference of some sort? (Maybe to
a later section?)

4.4.1.12: Facebook again.

4.4.1.12: Signing the code seems like a bit of a hack.  Do
you really want to recommend this here? I suspect it'd end
up different if you actually tried it out aiming for an
interoperable solution. I'd suggest deleting this, or
maybe make it less specific, e.g. saying if origin
authentication for authorization codes could be validated
by clients, then that'd be a countermeasure, but that
there's no current spec for that. (And doing that might
just move the DoS to somewhere else depending how you
did it.)

4.4.2: typo: s/and It cannot/and it cannot/

4.4.2.1: Is the countermeasure here TLS? If so, better to
say so.

4.4.2.2: requests aren't cached are they but rather
responses?

4.4.2.3: typo: s/An malicious/A malicious/

4.4.2.3: The reference back to 4.4.1.4 isn't very
clear since a lot of the countermeasures there
mention authentication. It'd be better to explicitly
list things here or else if you stick with the
backwards reference to be more explicit about whic
exactly apply.

4.4.2.5: Is this entirely identical to 4.4.1.8? That
doesn't seem right. If it is, then say so explicitly.
If not, then say what's different.

4.4.3: 1st mention of username/password anti-pattern,
so you could add a reference

4.4.3: Be good to metion here that passwords are often
used for >1 service, so this anti-pattern risks whatever
else is accessible with that credential or an
algorithmically derivable equivalent (e.g.
[email-masked]/pwd might easily allow someone to guess
that the same pwd is used for [email-masked]) and
then another countermeasure is to educate users to
not use the same pwd for multiple services (hard as
that is to really do;-)

4.4.3.4: again digest auth isn't a good example
here, but client certs are.

4.4.3.6: What does "Abandon on grant type..." mean?
If you mean "don't do this" then say that more
clearly.

4.6.2: typo: s/transport security measure/transport
security measures/ or maybe just say TLS

4.6.2: typo: s/nounces/nonces/

4.6.3: 1st countermeasure: maybe s/difficult/infeasible/
I don't see why "difficult" guessing is hard enough?

4.6.4: typo: s/a valid access tokens/a valid access token/

4.6.4: Do you mean the IP address in the 2nd
countermeasure?  I'm not sure, esp. given the above.

4.6.4: typo: s/miss the capabilities/lack the capability/

4.6.6: reference to 2616 is broken

4.6.6: Should you reference httpbis drafts? Just asking, I
can see arguments for referencing just 2616 or just
httpbis or both, given that this'll be read for hopefully
a while after httpbis is done.

4.6.7: referrer vs. referer again

4.6.7: What is an appropriat logging configuration? Can
you give a reference maybe or is it really that well
known?

4.6.7: Reloading the target page again? Are you really
recommending that?

5.1: typo: s/consideratios/considerations/

5.1.3: I think you should note the email notifications
can be a phishing vector so don't make your emails
such that lookalike phish messages can easily be
derived from them.

5.1.3: Don't you need to say something about getting
email notifications delivered and not treated as spam?
Maybe you could recommend dkim here or other ways to
get better delivery. (Not sure myself, probably best
to ask someone who operates like this so see if there's
stuff to be said.)

5.1.4.1.3: typo: s/not store credential/not store
credentials/

5.1.4.1.4: salted hashes don't prevent offline
dictionary attacks, they just increase the workload

5.1.5.1.4: would it be worthwhile recommending that
different client credentials be used in development
or integration mode vs. when operational? I've seen
a bunch of times when programmers were given "live"
API keys when that could've been avoided.

5.1.4.1.5: I don't get this. If it was only that
easy:-) I think you need to say which private keys
are used here and for what for this section to be
useful.

5.1.4.2.1: I think you could note that complex password
policies can also increase the liklihood that users
re-use passwords or write them down or store them
somewhere not so good, if e.g. the entropy required
over all the user's passwords (dozens usually) is
too great for long-term memory.

5.1.5.1: typo: s/Basis of/The basis of/

5.1.5.1: typo: s/sensible service/sensitive service/
(2nd best typo:-)

5.1.5.3: typo: s/preciser/more precise/

5.1.5.3: typo: s/refreshments/refreshes/

5.1.5.4: 2nd bullet is not a threat but a goal

5.1.5.4: typo: s/redeem a/redeem an/

5.1.5.5: what is a "rough" server? Do you mean rogue?

5.5.5.6: I think you need to clarify what "address"
means here too.

5.1.5.9: please clarify if you mean digitally signed
(asymmetric) or also include MACing here

5.1.5.10: typo: s/Self-contained/Self-contained tokens/

5.1.5.10: s/privacy/confidentiality/ ?

5.1.5.10: this (typically, I'd guess) requires sharing
symmetric keys between server nodes, so you should
say that as its a bunch of work.

5.1.5.11: I don't get why you want to repeat this
text (and its error:-) better to refer back to
the earlier section I think.

5.1.5.12: Another place where a SAML reference would
be needed.

5.1.6: 2nd bullet is not a "measure" but a goal. If
you had something in mind, it doesn't come out from
that text.

5.2.2.2: You say the binding should be protected, but
don't say how. I think you ought.

5.2.3: typo: s/sub-sequent/subsequent/ but maybe
better to delete the word

5.2.3: 2nd bullet - "trustworthiness" has gotta
be the wrong word there, what did you mean?

5.2.3: typo: s/statics/statistics/

5.2.3: typo: s/support achieve objectives/achieve these
objectives/ ?

5.2.3: typo: s/by hand of an administrator/something
better/

5.2.3.1: "prevents...overestimating" seems wrong, I think
you mean it reduces the probability of mistakenly treating
a useless authentication as a good one. (The point is
that servers don't "overestimate," only people do that.)

5.2.3.1: "cannot be entirely protected" seems like
understatement - the reality is any such secret will
leak out from anything that's any way successful. It
only protects stuff that *nobody* cares about.

5.2.3.1: typo: s/trust on/trust/ But I'd rephrase it
to not use the terms "trust" nor "identity" and then
it'd be better I think.

5.2.3.2: typo? s/The authorization may/The authrozation
server may/ ? Not sure what "issue a cliend id" means
either. (Same in 5.2.3.3)

5.2.3.4: typo: s/A authorization server/An authorization
server/

5.2.3.5: what are "validated properties"?

5.2.3.5: what is the 1st bullet list on p57? there's
no introductory text?

5.2.3.5: the "it" in "it only works" in the last
paragraph isn't clear - which "it" do you mean?

5.2.3.5: saying discovery "gets involved" seems
wrong - do you mean "is used"? And what is the
"that" in "that's no longer feasible"?

5.2.3.6: typo: s/be unintentionally for/unintentionally
affect/?

5.2.3.7: typo: s/to distribute client_secret/to
distribute a client_secret/

5.2.4.1: Is a "security certificate" a public key
certificate? If so, saying that is better.

5.2.4.1: the references to 5.7.2.x are wrong and
look like you're missing some xref's in the xml
maybe

5.2.4.2: you said "address" again, so the usual
question arises :-)

5.2.4.3: typo: s/in all situation/in situations/
(not sure "all" is right so suggest deleting it)

5.2.4.4: again, be good to say how to protect
the binding

5.2.4.5: as for 5.2.4.4 say how binding is done

5.3.1: typo: s/a associated/an associated/

5.3.1: "entirely protected" is (again:-) understatement
see above for a suggestion

5.3.1: what does "trust on the client's identity" mean?
Its not clear anyway

5.3.3: typo: s/operation systems/operating systems/
(enrire 2nd & 3rd paras could do with re-phrasing)

5.3.4: typo: s/their level/the level/

5.3.5: this is too terse - is it really finished? I'd
say there's a sentence or two missing.

5.4.2: what does it mean to "encourage resource
servers" to do something? I guess you mean to encourage
the people deploying those to pick resource servers
with that capability and to turn that on.

5.4.2: not sure if everyone will know what a
"distinguished name" is, maybe add a reference to
rfc 5280?

5.4.2: token-bound secrets would be used to MAC
the request and not to sign, be better to make that
clear even if you still call that "signing" here
(its not really, if we're being pedantic)

5.4.2: typo: s/This mechanisms/This mechanism/

5.4.2 and 5.4.3: I forget - where are the protocol
mechanisms for this defined? Can you give a reference
or say that its future stuff if there aren't any
good ones?

5.5: typo: s/capable to validate/capable of validating/

8.1: Is the base spec really normative? I'd say you're
fine with that as informative too.

8.2: there are a bunch of references missing as per
the questions above

8.2: is there no better (more stable) reference than
portablecontacts.net?