Early Review of draft-ietf-oauth-v2-
review-ietf-oauth-v2-secdir-early-johansson-2011-10-07-00

Request Review of draft-ietf-oauth-v2
Requested rev. no specific revision (document currently at 31)
Type Early Review
Team Security Area Directorate (secdir)
Deadline 2012-03-30
Requested 2011-10-07
Authors Dick Hardt
Draft last updated 2011-10-07
Completed reviews Secdir Early review of -?? by Leif Johansson
Secdir Last Call review of -?? by Leif Johansson
Tsvdir Last Call review of -?? by Haibin Song
Assignment Reviewer Leif Johansson
State Completed
Review review-ietf-oauth-v2-secdir-early-johansson-2011-10-07
Review completed: 2011-10-07

Review
review-ietf-oauth-v2-secdir-early-johansson-2011-10-07

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Security review of OAUTH 2.0 core: draft-ietf-oauth-v2-21

Do not be alarmed.  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.

This review is rather lengthy. This should not be interpreted as
anything beyond a desire to do a thorough review.

It may well be that I have stumbled on things already covered on the
list. If so I apologize and ask that you silently ignore such bits.
Also I have included things that are not directly security related
but that I found problematic for other reasons.

The notes are presented in the order I wrote them down.

** General observations:

POST and/or GET

Examples are sometimes POST and sometimes GET. In many cases it is not
clear to me from the surrounding text if both POST and GET are allowed
or if only one is mandated. Illustrating with both a GET _and_ POST
example in the cases where both are supported would help or make the
method explicit in the text before the example.

The P-word

The term 'password' is sprinkled throughout the document, sometimes
as in "client password" or "resource owner password credentials" and
I suspect that sometimes it is password as in 'an example of a
credential type' and in other cases it is password as in 'plain old
password'. This needs to be cleared up throughout (I've included some
examples below).

Normative Language

I've often found myself wanting more normative language often to replace
existing but less precise text. I've called out some important cases
below.

Unknown parameters

The sentence "The client SHOULD ignore unrecognized response parameters"
occurs in several places. First of all I would argue that this has to
be a MUST if you want to be able to guarantee extensibility. Secondly
there are some error responses that are triggered by unsupported request
parameters. This seems like an inconsistency.

** Specifics

1.1 Roles

Forward references to the sections where the roles are defined would
improve readability.

As an illustration of an alternative model for the authorization server
maybe an informative reference to UMA would be illustrative here.

1.2 Protocol Flow

(A) talks about 'an intermediary such as an authorization server.' it
isn't clear it me if this refers to a separate authorization server
or if it is the same authorization server that is referenced in (B).

1.3.3 Resource Owner Password Credentials

When I first read this I thought - why not talk about Resource Owner
Credentials - in fact there is a parenthesis there "(e.g a username
and password)" that seems to indicate that other credentials could
be supported.

This needs to be cleared up. Either its username and password and
nothing else or there is a way to support things like Negotiate or
X.509-based credentials in which case it should really be Resource
Owner Credentials with no reference to passwords other than as as
an example of one possible credential type.

1.4 Access Token

first paragraph: "The string is usually opaque". This should be
reformulated as normative language. In fact I would have liked
guidance on generating those tokens (which has been brought up
on the list) possibly as part of an implementation-guide.

1.5 Refresh Token

Why is the refresh token not simply treated as an access token
for the access token resource in the authorization server? This
would seem to simplify the model a bit by removing a type of
token whose semantic (at least to this reviewer) is a duplication
of a normal access token for a particular type of resource.

Also in the first paragraph: "(access tokens may have a shorter
lifetime and fewer permissions". Why not try to write normative
language here - there are security implications of allowing
a refresh token to get more permissions (say) than the original
access token.

2.1 Client types

I find the terminology public/confidential somewhat counter-intuitive.
An app you have on your personal device is 'public' while a shared
cloud service is 'confidential'...hmm...

This section also lacks normative language which isn't good. There
should be language defining the behavior of public and confidential
client aswell as the three profiles.

For instance under native application we have "It is assumed that
any client authentication credentials included in the application
can be extracted". This should really be normative language. Some
of what I am looking for can be found in section 2.3 but it isn't
clear to me that it covers the definition of the profiles.

2.3.1 Client Password

There is also some confusion here about what the client must implement
and what the server must implement wrt the use of client_id. I read the
text as trying to say that Servers SHOULD support both and clients SHOULD
only do Basic.

This section also seems to have been the product of a partial
s/password/credential/g operation. Either OAUTH is only meant to use
Basic and passwords or we want to cover all/most HTTP authentication
methods and credentials and then section 2.3.2 (which feels like an
afterthought) needs to get folded into 2.3.1 and the normative language
(and examples!) need some work to cover at least X.509s and perhaps
even Negotiate.

Personally I would try to fold 2.3.1 and 2.3.2 into one section and say
that servers MUST support Basic and client_id+client_password. Servers
MAY support any HTTP authentication method with a mapping to client_id.
If a client supports username+passwords it SHOULD do Basic and if it
supports other HTTP authentication methods it MUST NOT use
client_password for anything and MUST follow normal HTTP authentication
method negotiation.

Finally, everywhere there is negotiation there must imho be some
mention/discussion/protection of downgrade attacks.

3.1 Authorization Endpoints

6th paragraph: "The authorization server SHOULD ignore unrecognized
request parameters". This formulation returns in several places in the
document and I don't understand why it isn't a MUST - after all doesn't
extensibility depend on this?

3.1.1 Response Type

The response_type parameter is REQURED but its absence SHOULD result in
an error. Why not MUST?

3.1.2 Redirection Endpoint

There should be a clear normative specification for how to  match
endpoints. There are traces of this in various parts of the document
when matching is discussed. I recommend making a concise definition
in one place (namely here) and referencing this throughout. Since
this comparison has security implications it should be a priority
for the specification to be air-tight.

3.1.2.2 Registration Requirements

"(the client MAY use the state request parameter to achieve per-request
customization)". Doesn't this overload its use for CSRF-protection?

3.1.2.4 Invalid Endpoint

"The authorization server SHOULD NOT redirect...". Why isn't this a
MUST NOT?

3.1.2.5 Endpoint Content

This section basically seems to say "Serve with server-side code not
with html or client-side code". If this is the case then I suggest
reformulate to say precisely that using normative language.

The use of the word "script" could perhaps also be made less ambiguous
since in this case it could refer to both server-side code aswell as
in-browser code.

3.2.1 Client Authentication

The phrase "clients issued client credentials" could be rephrased to
make less violence on English - perhaps "clients that have been issued
with client credentials", unless that is not the intended meaning in
which case I argue for something easier to understand ;-)

The second bullet: Using client credentials more often also exposes them
more which might be worth mentioning aswell.

4. Obtaining Authorization

Perhaps not critical but the term 'password credentials' occurs in the
first paragraph and this doesn't seem compatible with resource owner
authentication being out of scope. It could just be that it should read
'resource owner credentials' but it could also signal an underlying
assumption about username and password being used for resource owner
authentication. In either case I thing its best to avoid the use of the
word 'password' to avoid any confusion.

4.1 Authorization Code

(C) - "using the redirection URI provided earlier" should perhaps read
"using the redirection URI provided earlier or associated with the
client in client registration"


4.1.1 and 4.1.2 Authorization Request/Authorization Response

The redirect_uri is listed as OPTIONAL with a reference to 3.1.2 but
there is no mention in 4.1.2 how to handle the case when the
redirect_uri is not present. I believe the assumption is that the
redirect_uri is either resent or part of client registration but that
needs to be made explicit in that case.

This may apply to other uses of the redirect_uri parameter eg in 4.2.1.

Furthermore in 4.2.2 "code" I suggest the following re-formulatation of
the last sentence: "The client MUST NOT use an authorization code for
more than one request. If an authorization code is re-used, the
authorization server should treat that as a replay attack and SHOULD
revoke all tokens associated with the client." (i.e loose the "attempt"
bit which carries no real meaning)

Also note that this is potentially a DOS attack should a single authz
code leak.

4.1.2.1 Error Response

First paragraph, last sentence "and MUST NOT automatically redirect". In
the light of how willing users normally are to click on links presented
to them I would strengthen this to "MUST prevent the user from following
the redirect URI"

In the definition of the invalid_request parameter I don't understand
how unsupported parameters can generate an error since endpoints should
ignore unsupported parameters (in order to support extensibility).

4.1.3 Access Token Request

"require client authentication for confidential clients or for any
client issued client credentials (or with other authentication
requirements)"

This text seems unnecessarily convoluted. Isn't enough to say that if
you have issued credentials to a client you MUST require authentication
from that client? This applies equally to the other sections where
client authentication is an issue (eg 4.3.2).

Also cf my comment on 3.1.2 for the discussion of matching redirect_uri
in the last bullet: ".. and that their values are identical". Is this
really meant to mean identical?

4.2 Implicit Grant

The parenthesis "(it does not support the issuance of refresh tokens)"
sounds like it should really be normative language, "refresh tokens
MUST NOT be issues for implicit grant" because afaiu you could easily
extend "fragment-transport" to include a refresh-token, so it isn't
a technically motivated constraint, right?

In (D) I would like to have a normative reference to a document that
specifies browser behavior for URL fragments since this is a critical
security dependency for this grant type.

4.4 Client Credentials

I think the text should note that this model effectively implies
that the client is able to impersonate all users which has the potential
for worse security problems than if the client has access to individual
user passwords.

6 Refreshing an Access Token

scope - The term "lesser than" is intuitive but imprecise. I suggest
"MUST NOT include any scope not originally granted by the resource owner".

7.1 and 8.1 Access Token Types

The section 7.1 lists two definitions of access token types and provides
a couple of examples but doesn't provide any constraints on future
development of access tokens except in 8.1 where it is implied that not
all access token types would be associated with HTTP authentication
mechanisms. Are there really no constraints on access token types
that should be captured?

10.6 Authorization Code Redirection URI Manipulation

Suggest replace the word 'familiar' with 'trusted' in the first sentence
of the second paragraph. The notion of trust opens up several boat loads
of worm but it is the correct term here I think.

In the third paragraph "the same as" wrt redirection URIs occur and
this needs to be defined (cf comments on 3.1.2 above).

Finally "The authorization server MUST require public clients and SHOULD
require confidential clients to register their redirection URI". I am
missing a discussion of why the two types of client differ wrt this
attack vector.

10.10 Credentials Guessing Attack

I found myself wanting implementation advice for how to generate good
tokens at this point. This has been raised on the list too. The same
goes for how to generate good CSRF cookies where the "(eg a hash of the
session cookie..." feels like it is implementation advice yearning to
come out of the closet.


Thats it.

	Cheers Leif
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - 

http://enigmail.mozdev.org/



iEYEARECAAYFAk5uT+YACgkQ8Jx8FtbMZncXQgCfZmTlzuESq0plfpkceQN1ontE
a1QAoIEcg06GYK+6Fn4y40cTL1jQ+KmS
=ox42
-----END PGP SIGNATURE-----