It would be great if you could do a similarly detailed read of the bearer token spec as well!
-- Mike Sent from my Windows Phone -----Original Message----- From: Brian Eaton Sent: Sunday, May 22, 2011 8:39 PM To: oauth@ietf.org Subject: [OAUTH-WG] draft 16 review notes I just read over the whole of the draft for the first time in a while. I looked it over mostly for a) places where spec and reality were going to have trouble intersecting and b) places where security advice would be useful and c) grammer and speling, because I notices things like that Mostly noticed nits. I'm really happy with the way some of the trickier issues have gotten dealt with. Sometimes those differences get papered over, but they mostly seem to have been dealt with in constructive ways. I didn't agree with all of the decisions, but I liked that the spec either made the decisions or gave clear guidance on trade-offs. So bear in mind that even though most of the comments in this e-mail are criticism, that's not because there isn't a lot of good in the spec. I won't be able to make the interim meeting tomorrow, but I wanted to send these out before hand. 1. Introduction nit: text mixes “resource-owner” and “resource owner”. I think “resource owner” is correct. consider adding bullet point: Compromise of any third-party application server results in compromise of the user’s password and all of the data protected by that password. nit: “web end-user” is an odd turn of phrase. Maybe just “end user” 1.1 Roles nit: “- access restricted resources which require authentication to access:” seems awkward. could eliminate the phrase entirely and make the meaning of the paragraph just as obvious. 1.4.1 Authorization Code maybe expand the section on important security benefits. “The use of authorization codes also improves the ability to recover in the event of compromise of an application server or authorization server.” 1.4.2 nit: “intermediary authorization grant”. I think “intermediate” is the right term here, but might be wrong. 1.5 Refresh Token “self-contain the authorization information in a verifiable manner.” That is never secure, because it makes review of issued permissions and revocation of those permissions impossible. Refresh tokens are different than access tokens; a refresh token is always going to be an identifier that you use to look up authorization information. The text should be: “The token denotes an identifier used to retrieve the authorization information.” (As another example of why you can’t have a self-verifying refresh token, consider the role of the client id and client secret in the protocol. If refresh tokens were self-verifying, you would also need bake the client secret into the refresh token, thus making client secret rotation impractical.) nit: “..., no longer valid, …” doesn’t make sense. I think the text should be “or is no longer valid.” The parenthetical phrase throws off this paragraph a bit, maybe rephrase as “The refresh token can be used to obtain a new access token when the current access token becomes invalid or expires, or to obtain addtional access tokens with identical or narrower scope. (Access tokens may have a shorter lifetime and fewer permissions than authorized by the resource owner.) Should also add, maybe in the second paragraph: “Unlike access tokens, refresh tokens are intended for use only with authorization servers. Refresh tokens are never sent to resource servers.” 3. Client Authentication I like the compromise reached here on the language around client secrets for installed app clients. It doesn’t reflect reality, but the language allows enough wiggle room to be practical. The spirit of the text is quite sensible, which is what I like best. 3.1 Client Password Authentication Referring to a password as a shared symmetric secret is misleading. “Symmetric secret” is used almost exclusively with secret key cryptography, where both sides store copies of the secret. That’s not the right way to use client passwords; they should be treated like *passwords*, not HMAC keys or encryption keys. The reference to needing consensus on when to use basic auth and when to use client_id and client_secret parameters seems quite reasonable. The current language seems to reflect a poor compromise that will make interoperable implementations less likely for no good reason. I’d rather that someone picked a winner and the spec was simple and consistent. 3.2 Other Client Authentication Methods Again, I like the compromise here. The language is vague but useful. 4.1.1. Authorization Request Please add “state” to the example. “state” is an important part of a secure client implementation, might as well encourage use by including it in examples. 4.1.2 Authorization Response “minimize the risk of leaks”: this should be “mitigate”, not “minimize”. “SHOULD expire shortly”. This should be a MUST. The security analysis of the protocol doesn’t hold unless authorization codes expire. Suggested language: “The authorization code MUST expire shortly after it is issued to mitigate the risk of leaks. A maximum authorization code lifetime of 10 minutes is RECOMMENDED.” Again, please include “state” in the example. 4.1.2.1 Error Response “and MUST NOT redirect”... this is contrary to some industry practice. Various service providers show an interstitial to warn the user, but will then redirect if the user clicks through the interstitial. How about “and MUST NOT automatically redirect” as alternative language. The language around using HTTP status codes as values for the “error=” parameter is surprising. Who requested that? Has anyone implemented that? Why is this a good idea? Again, example including “state” parameter would be a good thing. 4.2 Implicit Grant nit: The phrase “...maintaining their client credentials confidential...”is a bit awkward. This should probably be “maintaining the confidentiality of their client credentials...” The introductory paragraph says the flow is suitable for clients that can’t keep their client credentials secret. However, that’s not sufficient. In addition, the client must be able to maintain the security of a redirect URI via something like the browser same-origin policy. Suggested language: “The implicit grant type is suitable for clients that cannot maintain the confidentiality of their client credentials, but which can be known to operate on a particular redirect URI.” These applications are typically implemented in a browser using a scripting language such as JavaScript. nit in step (D): “(does not include the fragment).” should be “(which does not include the fragment).” 4.2.1 Authorization Request Please include “state” in example. Validation of the redirect_uri parameter is more important for the implicit grant type. Section 2.1.1 leaves registration of the redirect URI as “SHOULD”. It’s a “MUST” for the implicit flow. Suggested language: The authorization server validates the request to ensure all required parameters are present and valid. The authorization server MUST verify that the redirect URI to which it will redirect the authorization code matches a redirect URI pre-registered by the client. The authorization server SHOULD verify the scheme, authority, and path of the redirect URI. The authorization server MAY verify the query parameters as well. 4.2.2. Access Token Response Please include “state” in example. 4.3 Resource Owner Password Credentials nit: “converting the stored credentials with an access token” should be “... to an access token.” 4.3.2 Access Token Request Really need language in here about the risk of brute force attacks on passwords. 4.4 Client Credentials The spec has this flow returning a refresh token. This is a security problem. We agreed to remove it or at least recommend against it almost a year ago. Thread here: http://www.ietf.org/mail-archive/web/oauth/current/msg03651.html General comments on HTTP headers. Should follow cache header recommendations from HTTP/1.1 spec for backwards compatibility with HTTP/1.0 caching proxies. This means adding “Pragma: no-cache” to all responses. See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.32. Should include charset in all content-type examples, to prevent security problems due to content sniffing. See http://code.google.com/p/browsersec/wiki/Part2#Survey_of_content_sniffing_behaviors. For example: Content-Type: application/json; charset=UTF-8 6. Refreshing an Access Token nit: should mention that when the authorization server returns a new refresh token, they are going to revoke the old one as well. How about: “The authorization server MAY issue a new refresh token, in which case the client MUST discard the old refresh token and replace it with the new refresh token. The authorization server MAY revoke the old refresh token after returning the new refresh token to the client.” Section 8.2 Defining New Endpoint Parameters Telling service providers to use vendor specific prefixes for query parameters sent to their web page is just not going to get implemented. Every service provider has a collection of long-established query parameters that they use on all or almost all of their web pages. That includes their authorization pages. Section 10. Security Considerations I love the introduction here, this is an excellent break down of the different client types, and I’ve never seen anyone else do it so concisely. One nit: I don’t think the statement “It is assumed that such an application can protect dynamically issued credentials, such as refresh tokens, from eavesdropping by other applications residing on the same device” is actually essential to the security analysis, and it rules out a large class of applications (e.g. everything on Windows or MacOS.) Maybe change the section on Native Applications to say “It is assumed that any client authentication credentials included in the application can be extracted, and furthermore that rotation of the client authentication credentials is not practical. Dynamically issued credentials such as refresh tokens, on the other hand, can receive an acceptable level of protection. At a minimum those credentials are protected from hostile servers which the client may contact. On some platform those credentials might be protected from other applications residing on the same device.” There are various security risks mentioned in the OAuth WRAP security considerations that seem worth mentioning here: http://trac.tools.ietf.org/wg/oauth/trac/raw-attachment/wiki/SecurityConsiderations/OAuthWRAP2.0SecurityConsiderations.pdf Section 10.1 Client Authentication nit: “MUST NOT issue client passwords to installed apps” is a dead letter, it is not going to change standard industry practice in the slightest. The language from section 3 is more constructive. I’d suggest the following language for section 10.1 instead “The authorization server MUST NOT assume that native or user-agent based applications can maintain the confidentiality of client secrets.” That does conform with industry practice, so it’s more likely to be implemented. “The authorization server is encouraged to consider using stronger client authentication means than a client password.” Could make that more specific by adding “, such as assertions as per <OAuth SAML spec>” Section 10.2 Client Impersonation I like the content of this section, but it seems to mix several different topics. - general risks of native applications - security considerations for “immediate mode”, where authorization requests are processed without end-user interaction. - validation rules for redirect URIs - open redirectors - access token design Most of those merit separate discussion. 10.3 Access Token Credentials “When using the implicit grant type, the access token credentials are transmitted in the URI fragment, which can expose the credentials to unauthorized parties.” That’s basically FUD. This should be made much more specific. It falls into the general category of security considerations for redirectors and clients... 10.5 Request Confidentiality. Why not specify TLS here, as is done elsewhere in the spec? 10.6 Endpoint Authenticity nit: “server-side authentication” should be “server authentication” (copying language from the TLS spec...) nit: should refer to RFC 2818 for server authentication rules 10.9 Authorization Codes “SHOULD be short lived and MUST be single use” is backwards. These MUST be short-lived. If they are long-lived, an attacker who compromises an end-user account even temporarily can mint a very large number of these tokens and replay them later as needed. MUST be single use is a dead letter, because it requires atomic operations at scale. Even things like password changes are not atomic in user databases of moderate size. Authorization codes might be generated quite frequently (e.g. when “immediate mode” flows are used), so a MUST for an atomic operation is unrealistic. nit: “Authorization codes operate as plaintext bearer credentials, used to verify that the end-user who granted authorization at the authorization server, is the same end-user returning to the client to complete the process.” That assumes that the user visited the client to trigger the authorization. That’s not always realistic, see the “unsolicited authentication response” from the SAML 2 protocol for examples of other protocols where this is common. 10.10 Session Fixation I’m not sure about the attack being described here. Some of the mitigations make it sound like the attack described is session hijacking, not session fixation. If the mitigations mentioned the state parameter, then I would think that the attack described was session fixation at the client, but that doesn’t come up. Can someone explain the attack in greater detail? 10.12 Resource Owner Password Credentials Really like this section, nicely put. =) After reading through the security considerations section a couple of times, I think it could benefit from a different organization. Specifically - keep the introduction, it’s awesome. - write new sections for each of the following 1) Authorization Tokens 2) Web Application Clients 3) User-Agent Clients 4) Installed Application Clients 5) Authorization Servers 6) Resource Servers - merge the current recommendations (which are very sensible) into the appropriate sections. Just for kicks I started to write up security considerations for authorization tokens. I'll send that to the list separately. Cheers, Brian _______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth _______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
