Re: [secdir] secdir review of draft-ietf-oauth-v2

[Eran Hammer-Lahav <[e-mail]>]

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:[e-mail]]
> 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 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.

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 preferably 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 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.

Changed to:

        (i.e. username and password)

This is explicitly just for username and password. Other types require an extension.

> 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 architecture 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 defined.

> 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 crafted 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 credentials 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 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.

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. MUST seems like the right language - any objections?

> 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.

This has been discussed by the WG and no consensus was reached. We can reconsider 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-protection?

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 about 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 to 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 provided earlier (in the
              request or during client registration). The redirection URI includes 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 needs 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 code 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 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).

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 made simpler. What do other think? The same end result, but different emphasis.

> 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 user 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 client identification while confidential clients are later authenticated which can 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