From: Eran Hammer-Lahav <eran@hueniverse.com>
To: Leif Johansson <leifj@sunet.se>, OAuth WG <oauth@ietf.org>
Date: Tue, 20 Sep 2011 15:12:45 -0700
Thread-Topic: secdir review of draft-ietf-oauth-v2
Thread-Index: AcxxejcLK5M7cRirTtqV4b97NCZawwGWSTgQAAPAPYA=
Message-ID: <90C41DD21FB7C64BB94121FBBC2E72345213D92D5A@P3PW5EX1MB01.EX1.SECURESERVER.NET>
References: <4E6E4FEA.7090100@sunet.se> 
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [OAUTH-WG] secdir review of draft-ietf-oauth-v2
Precedence: list

(+OAuth WG, -everyone else)

Thanks Leif.

See comments inline. Whatever issues are still open will be addressed along=
 with the rest of the IETF LC feedback.

EHL


> -----Original Message-----
> From: Leif Johansson [mailto:leifj@sunet.se]
> Sent: Monday, September 12, 2011 11:31 AM

> ** 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 exa=
mples 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 ca=
ses 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.

What sections? That's the only place these roles are defined.

> As an illustration of an alternative model for the authorization
> server maybe an informative reference to UMA would be illustrative here.

A reference to UMA would be confusing in this document.

> 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).

Changed to:

              (A) The client requests authorization from the resource owner=
. The authorization request
              can be made directly to the resource owner (as shown), or pre=
ferably indirectly via
              the authorization server as an intermediary.

> 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 sup=
ported.
>
> 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.

Changed to:

        (i.e. username and password)

This is explicitly just for username and password. Other types require an e=
xtension.

> 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.

There is not requirement for the string to be opaque, but the general archi=
tecture assumes it is. Otherwise, please suggest language.

> 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.

Simpler architecture but probably more confusing to many developers.

> 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.

This was discussed before and we could not reach consensus.

> 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...

These are the best terms we could come up with. Not intuitive but well defi=
ned.

> 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.

Please suggest text.

> 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.

We could not reach consensus beyond this. This language was carefully craft=
ed to work around the disagreements about what is required and what is not.

> 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.

When we say password we mean 'plain old password'. Any other types of crede=
ntials are not covered by design.

> 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 clie=
nt_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.

Downgrade attacks security section sounds useful. Text?

> 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?

I don't have an objection, but extensibility isn't currently required.

> 3.1.1 Response Type
>
> The response_type parameter is REQURED but its absence SHOULD result
> in an error. Why not MUST?

This is an OAuth 1.0 legacy of not mandating a particular error behavior. M=
UST seems like the right language - any objections?

> 3.1.2 Redirection Endpoint
>
> There should be a clear normative specification for how to  match endpoin=
ts.
> 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.

This has been discussed by the WG and no consensus was reached. We can reco=
nsider if someone submits proposed text.

> 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-prote=
ction?

Yep. That's intentional.

> 3.1.2.4 Invalid Endpoint
>
> "The authorization server SHOULD NOT redirect...". Why isn't this a
> MUST NOT?

I'm ok with MUST NOT - any objections?

> 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.

This is only about the HTML response sent back to the user-agent. This is a=
bout including something like 'Share on Twitter/Facebook' widget on a page =
with an access token in the URI fragment (which has caused tokens to leak t=
o Twitter in some cases).

> 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 ;-)

Ok.

> The second bullet: Using client credentials more often also exposes
> them more which might be worth mentioning aswell.

Proposed text?

> 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.

Password is intentional. This is a special case for passwords.

> 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"

Changed to:

              Assuming the resource owner grants access, the authorization =
server redirects the
              user-agent back to the client using the redirection URI provi=
ded earlier (in the
              request or during client registration). The redirection URI i=
ncludes an authorization
              code and any local state provided by the client earlier.

> 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 nee=
ds to be made explicit in that case.

3.1.2 describe how to handle this parameter.

> 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)

Changed to server MUST not allow reuse based on previous feedback. Not sure=
 treating it as an attack is the right language, but the normative language=
 is the same either way (about revoking tokens).

> Also note that this is potentially a DOS attack should a single authz cod=
e leak.

Explain?

> 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"

Not sure what you mean.

> 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).

Good catch. Dropped 'unsupported parameter' part.

> 4.1.3 Access Token Request
>
> "require client authentication for confidential clients or for any
> client issued client credentials (or with other authentication requiremen=
ts)"
>
> 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).

I felt it was important to call out confidential clients explicitly, but I =
agree that if we require issuing credentials to such clients, this can be m=
ade simpler. What do other think? The same end result, but different emphas=
is.

> 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?

Here yes. String comparison.

> 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?

Instead I added this to 4.2.2:

            The authorization server MUST NOT issue a refresh token.

> 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.

That's RFC2616. Seems odd to reference it here.

> 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 u=
ser passwords.

Worse how? You can't use this with resource owner password.

> 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"=
.

Ok.

> 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?

None suggested.

> 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.

Ok.

> In the third paragraph "the same as" wrt redirection URIs occur and
> this needs to be defined (cf comments on 3.1.2 above).

Changed to identical.

> 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.

Public clients rely on the registration to provide a minimal level of clien=
t identification while confidential clients are later authenticated which c=
an mitigate redirection URI manipulation.

> 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.

Need someone to suggest text.

EHL