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