On 06/03/2012 08:57 AM, Torsten Lodderstedt wrote: > Hi Stephen, > > thank you very much for your review. Having scanned through your > comments, I think the document would definitely benefit from > incorporating them into a new revision. We will start working on it now, > but I can't tell you how long it will take (due to daytime job load).
Sure. With 3 authors and most of the comments only needing trivial fixes, I'd hope that's not too long a job. (Or, if it is, maybe ask the chairs to try find more help.) Cheers, S. > > best regards, > Torsten. > > Am 28.05.2012 20:34, schrieb Stephen Farrell: >> >> Hi all, >> >> I've gotten the publication request for oauth-threatmodel >> so here's my AD review of -05. >> >> Its quite a read (and a good one) but I've a bunch of >> questions. Some of these will need fixing I suspect >> but a lot are ok to fix later after IETF LC, depending >> on whether the authors want to re-spin it before then >> or not. But I'd like to at least see reactions to the >> questions before I start IETF LC. >> >> Although there are many many nits and typos, those >> don't actually make the document unreadable so >> though I'd prefer to see 'em fixed now, I'm ok with >> that happening later if its a problem to get it >> all done now. >> >> If you want to argue for going ahead with IETF LC >> now please do so, but I suspect that this might need >> a revised ID to fix at least a couple of the points >> raised below. If nobody does argue to go ahead now, >> I'll mark it as revised ID needed, but first let's >> see what the answers are to the questions. >> >> Cheers, >> S. >> >> >> (1) s/RFC1750/RFC4086/ is needed as noted in the write-up. >> >> (2) You don't say anything about the probability of >> occurrence of the various threats. I realise that you >> can't be precise but it seems wrong to say nothing. Would >> it be worth at least saying that that's not done here and >> that readers of this document need to do their own risk >> analysis including that aspect? >> >> (3) Many deployments will use TLS accelerators. That >> means that TLS isn't fully e2e, and that opens up some >> (mainly) insider attacks or attacks that can be launched >> from within a compromised DMZ, but from outside the server >> applications. Does that need a mention somewhere? (I've >> seen systems like that deployed and a lot could go wrong >> from the inside, so I think this is a real threat.) >> >> (4) Could you use just one of "client identity" or "client >> identifier" consistently? I'd much prefer the latter, >> which has also been the outcome of various discussions on >> this topic elsewhere. For example you say "revocation of >> such an identity" at the end of p13, and that's a >> potential rathole, better to say "revocation of the rights >> associated with a client identifier" or similar I think. >> And similar changes throughout. >> >> (5) 4.4.2.2: Here you recommend native applications should >> use an embedded browser, but earlier you said that was a >> bad idea. I think you need to be consistent or else >> provide more about when its ok to embed and when its not. >> Did I misread it or does that need a change? >> >> (6) 4.4.3.1: This calls for "obfuscation" of passwords in >> logs. I think you ought be stronger there and call for >> strong encryption of passwords wherever they are stored, >> be that logs or DBs or whatever. Would'nt that be reasonable? >> >> (7) 4.6.4: 1st countermeasure: I don't think you mean >> address here (in the sense of an IP address, which is what >> that means) but rather the domain name or FQDN or URL. In >> any case, you need to say what is meant clearly. (Also in >> 5.1.5.6 and elsewhere when you use the term "address".) >> >> (8) 4.6.6: You say to use Cache-Control, but don't say >> how. Is more needed really? Maybe there's a good document >> you could reference for that? (I'm not suggesting you add >> a lecture on caching btw:-) >> >> (9) 5.1.1: needs a reference to RFC 5246 (and better to >> just say TLS and not say SSL, at least for me :-) >> >> (10) 5.1.1: needs a reference to whatever you mean by >> "VPN" since there are many types of VPN. I assume you mean >> an IPsec VPN? But even if not, saying more would be good. >> >> (11) 5.1.2: needs a reference to RFC 5280 and/or RFC 6125 >> and/or RFC 2818. Bascially, you need to say how this is >> done (by reference). >> >> (12) 5.1.4.1: Isn't there some good reference you can give >> here? (Having said that, I'd have to go look myself, but >> I'd maybe start with owasp or sans.) >> >> (13) 5.1.4.2.2: if p(collision) should be<=2^(-160) then >> what's the point of saying it ought be<= 2^(-128)? Also >> if sha-1 were perfect, then the 160 bits output would >> really have a collision probability of about 2^(-80) with >> many many tokens, but not 2^(-160). I think you need >> to fix something there. Perhaps you're really saying that >> all high-entropy secrets should be>=128 bits long and >> constructed with a good (P)RNG? If so, saying so more >> clearly would be better. Not everyone will get the >> collision probability way of saying that even when its >> properly stated. (This text is also repeated, be better if >> you just said this once I think.) >> >> (14) 5.1.5.2: what is a "reasonable duration" - I don't >> think its ok to say that but nothing else. Can't you give >> some "reasonable" examples based on current usage? >> >> (15) 5.1.5.5: needs a reference to SAML assertions with >> the current text. >> >> (16) 5.2.2.3: this describes a refresh token rotation >> scheme that I don't recall being discussed on the list, so >> this is just to check that that rotation scheme, as >> described, doesn't ring any alarms bells for the WG. If >> not, that's fine. And if it was discussed on the list and >> I've forgotten, then sorry about that:-) >> >> (17) 5.2.2.5: Using IMEI's like this has privacy >> implications that I think you ought call out. A single >> sentence and a reference to something that says "be >> careful about privacy here" would be fine I'd say. (And >> you need a reference for "IMEI" and to expand the term.) >> >> (18) 5.2.2.6: needs a reference for X-FRAME-OPTION. >> There's a websec draft about that I think. >> >> (19) 5.2.3.4: what do you mean when you say "for different >> deployments of a client"? That could be one secret per >> install or one secret for all customers of a mobile >> operator and those are radically different. I think you >> need to be clear and hope you mean the former. That's >> almost cleared up in the 3rd paragraph of the section but >> I wanted to just check. Not sure "deployment" is the best >> term there really - what's wrong with "installation"? >> >> (many:-) nits and typos: >> >> 2.3.1: maybe explain "handle-based design" or give a >> reference? (Or maybe just a forward ref to 3.1?) >> >> 2.3.3: It might be worth re-iterating that the term >> "client" in oauth is used differently, e.g. by copying a >> bit of text from the base spec. I can see folks being >> confused by this otherwise. >> >> 3.1: "is digitally signed" - do you mean it has data >> integrity and origin authentication? If MACs are commonly >> used (or maybe authenticated encryption), and not >> necessarily signatures, then saying that would be better. >> >> 3.1.2: typo: s/this mechanisms/this mechanism/ >> >> 3.1.2: maybe s/Expires_In/expires_in/ to match the case in >> the base spec? I think it'd be better not to capitalise >> this in case it finds its way into someone's code. You >> could also use "Expires In" in the title and then say that >> its "expires_in" in the protocol. Might be worth doing >> something generic to call out when you're talking about >> specific things from the protocol, e.g. use a convention >> like ``expires_in'' or "expires_in" consistently and say >> that in the intro. >> >> 3.4: typo: s/the later/the latter/ >> >> 3.4: bullet item 1 isn't really a reason for anything but >> a downside of doing this, at least as written. Maybe this >> needs to be tweaked? >> >> 3.6: expand CSRF on 1st use and maybe give a reference >> CSRF is expanded in 4.4.1.8 which is a good bit later, >> and also in 4.4.2.5 which could presumably be >> shortened a bit by removing the repetition. >> >> 3.7: typo: s/collage associated request/collate associated >> requests/ >> >> 3.7: Maybe give a pointer to the definition of "native >> application" in the base spec (Its section 9 of that.) >> 2nd last para of p13 would be a good place for that. >> >> section 4: maybe s/Security Threat Model/Threat Model/ >> in the section title - what other kind of threat >> model is there? >> >> section 4: I wondered how we know the threat model >> is "comprehensive"? Perhaps "detailed" is better? >> >> 4.2.1: typo: s/Users/users/g unless you mean something >> special? >> >> 4.2.2: typo: s/a understandable/an understandable/ >> >> 4.2.2: "...based on who the client is." is unclear >> and not gramatically nice:-) "...based on the client >> identifier." would seem better. >> >> 4.3.1: typo? s/on transit/in transit/ Or did you >> mean something else? and s/may attempts/may attempt/ >> >> 4.3.3: maybe s/Revelation/Disclosure/ to be a tad >> less biblical:-) >> >> 4.3.3: typo? 1st countermeasure reads oddly and >> refers to a different place than other references >> to TLS - is that correct? >> >> 4.3.3: digest auth is nearly the same as sending >> credentials over the wire, TLS client auth based >> on certificates would be a better example, even >> if its not often done. >> >> 4.3.4: 4.3.2 points to 5.1.4.1.2 but this doesn't. >> Maybe it ought to? >> >> 4.3.6: typo s/an authorization servers/an authorization >> server/ >> >> 4.3.6: 4.3.5 refers to 5.1.4.2.2 wrt entropy. Is there >> a reason to not do that here too? >> >> 4.4: typo? s/tokens endpoint/token endpoint/ ? >> >> 4.4.1.1: typo: s/by different ways/in different ways/ >> >> 4.4.1.1: typo-fix-typo? HTTP has a "Referer" header field, >> but you fixed their typo here - might be better to live >> with the bad spelling, in which case >> s/referrer/referer/g;-) >> >> 4.4.1.1: Is there no better way to reference these >> OASIS docs? More importantly, is there no better (more >> stable) reference than thomasgross.net for the >> PDF you reference? Tidying this up would be good. >> >> 4.4.1.1 and elsewhere: a few times you say "such as >> TLS or SSL," I think it'd be better to just say TLS >> there and do it consistently everwhere. In other >> places (e.g. 4.4.1.5) you say "HTTPS" - again that'd >> be better done consistently. >> >> 4.4.1.1: typo: s/redeem a authorization code/redeem >> an authorizatio code/ >> >> 4.4.1.4: "counterfeit a valid client" reads oddly, >> do you mean "pretend to be a valid client"? If so, >> I think that'd be clearer. >> >> 4.4.1.4: "and to prevent unauthorized access to >> them" - I think you might add "via the oauth >> protocol" there since the AS isn't responsible for >> all possible ways of preventing unauthorized access. >> >> 4.4.1.4: typo: s/not neccesserily indicates/doesn't >> necessarily indicate/ >> >> 4.4.1.4: typo: s/user should be explained the purpose/ >> something better/ :-) >> >> 4.4.1.4: expand/define CAPTCHA on 1st use. Give a >> reference for this too. Especially since you also >> have 5.1.4.2.5, which is maybe the best place for >> the reference. >> >> 4.4.1.4: isn't a PIN code another user authentiation? >> Seems like a bad example of automatic authentication, >> since it isn't automatic if the user has to enter a >> PIN. >> >> 4.4.1.6: Is Facebook a trademark? Maybe better to not >> use that if so? >> >> 4.4.1.7: typo: s/achieve that goal/achieves that goal/ >> >> 4.4.1.7: typo: s/victims resources/victim's resources/ >> >> 4.4.1.7: typo: s/The attackers gains/The attacker gains/ >> >> 4.4.1.7: typo: s/then the target web site/rather than >> the target web site/ >> >> 4.4.1.7: "session fixation" could do with a reference >> or definition, and that might be better earlier in >> this section and not just in the countermeasures >> part. >> >> 4.4.1.7: typo: s/kind of attacks/kind of attack/ >> >> 4.4.1.8: typo: s/not follow untrusted/to not follow >> untrusted/ >> >> 4.4.1.9: maybe add a reference for "iframe"? And >> you say "iFrames" later, better to be consistent. >> >> 4.4.1.9: 1st countermeasure - do you mean end-user >> authorization here or end-user authentication? And >> would it be better to say "system browser" or >> something rather than "external browser"? (I forget >> what phrase you used for that earlier but there >> was something bettter:-) >> >> 4.4.1.9: "javascript framebusting" really needs >> a reference >> >> 4.4.1.10: typo: s/the victims resources/the victim's >> resources/ >> >> 4.4.1.10: typo: s/or split the/or splits the/ >> >> 4.4.1.10: "corresponding form post requests" isn't >> clear to me - does that need rephrasing or is it >> just me? >> >> 4.4.1.10: this is the first mention of cookies, which >> I found surprising, but that's all;-) >> >> 4.4.1.10: the 2nd "ways to achieve this" bullet is >> a bit unclear - which "It" and whose browser? I >> think this could be clearer. >> >> 4.4.1.10: typo: s/as native app/as a native application/ >> >> 4.4.1.10: what does "assume" the threat mean? >> >> 4.4.1.10: typo: s/an user interaction/a user interaction/ >> >> 4.4.1.10: typo: s/CAPTCAs, or/CAPTCHAs/ or else get >> rid of the "or" from the last bullet >> >> 4.4.1.10: typo: s/send out of bound/sent out of band/ >> >> 4.4.1.10: typo: s/instance message/instant message/ >> >> 4.4.1.11: typo? s/directing user(s) browser/directing >> the user's browser/ ? >> >> 4.4.1.11: I don't get the explanation here. Are you >> assuming (but not saying) that generating non-trivial >> entropy is a slow process? (e.g. /dev/random blocking?) >> Its also not clear what "pool" you mean? This probably >> needs a bit of tweaking. >> >> 4.4.1.12: semicomplete.com may not be a sufficiently >> stable reference - can't you find a better one? >> >> 4.4.1.12: typo: s/Defenses such rate limiting/Defenses >> such as rate limiting/ >> >> 4.4.1.12: typo: s/an anonymizing systems/an anonymizing >> system/ >> >> 4.4.1.12: nicest typo yet! s/annoying system/anonymizing >> system/ :-) >> >> 4.4.1.12: typo? maybe s/iframe/iFrame/ again? >> >> 4.4.1.12: 1st reference to "the CSRF token" here? That >> might warrant a reference of some sort? (Maybe to >> a later section?) >> >> 4.4.1.12: Facebook again. >> >> 4.4.1.12: Signing the code seems like a bit of a hack. Do >> you really want to recommend this here? I suspect it'd end >> up different if you actually tried it out aiming for an >> interoperable solution. I'd suggest deleting this, or >> maybe make it less specific, e.g. saying if origin >> authentication for authorization codes could be validated >> by clients, then that'd be a countermeasure, but that >> there's no current spec for that. (And doing that might >> just move the DoS to somewhere else depending how you >> did it.) >> >> 4.4.2: typo: s/and It cannot/and it cannot/ >> >> 4.4.2.1: Is the countermeasure here TLS? If so, better to >> say so. >> >> 4.4.2.2: requests aren't cached are they but rather >> responses? >> >> 4.4.2.3: typo: s/An malicious/A malicious/ >> >> 4.4.2.3: The reference back to 4.4.1.4 isn't very >> clear since a lot of the countermeasures there >> mention authentication. It'd be better to explicitly >> list things here or else if you stick with the >> backwards reference to be more explicit about whic >> exactly apply. >> >> 4.4.2.5: Is this entirely identical to 4.4.1.8? That >> doesn't seem right. If it is, then say so explicitly. >> If not, then say what's different. >> >> 4.4.3: 1st mention of username/password anti-pattern, >> so you could add a reference >> >> 4.4.3: Be good to metion here that passwords are often >> used for>1 service, so this anti-pattern risks whatever >> else is accessible with that credential or an >> algorithmically derivable equivalent (e.g. >> j...@example.com/pwd might easily allow someone to guess >> that the same pwd is used for joe.u...@example.net) and >> then another countermeasure is to educate users to >> not use the same pwd for multiple services (hard as >> that is to really do;-) >> >> 4.4.3.4: again digest auth isn't a good example >> here, but client certs are. >> >> 4.4.3.6: What does "Abandon on grant type..." mean? >> If you mean "don't do this" then say that more >> clearly. >> >> 4.6.2: typo: s/transport security measure/transport >> security measures/ or maybe just say TLS >> >> 4.6.2: typo: s/nounces/nonces/ >> >> 4.6.3: 1st countermeasure: maybe s/difficult/infeasible/ >> I don't see why "difficult" guessing is hard enough? >> >> 4.6.4: typo: s/a valid access tokens/a valid access token/ >> >> 4.6.4: Do you mean the IP address in the 2nd >> countermeasure? I'm not sure, esp. given the above. >> >> 4.6.4: typo: s/miss the capabilities/lack the capability/ >> >> 4.6.6: reference to 2616 is broken >> >> 4.6.6: Should you reference httpbis drafts? Just asking, I >> can see arguments for referencing just 2616 or just >> httpbis or both, given that this'll be read for hopefully >> a while after httpbis is done. >> >> 4.6.7: referrer vs. referer again >> >> 4.6.7: What is an appropriat logging configuration? Can >> you give a reference maybe or is it really that well >> known? >> >> 4.6.7: Reloading the target page again? Are you really >> recommending that? >> >> 5.1: typo: s/consideratios/considerations/ >> >> 5.1.3: I think you should note the email notifications >> can be a phishing vector so don't make your emails >> such that lookalike phish messages can easily be >> derived from them. >> >> 5.1.3: Don't you need to say something about getting >> email notifications delivered and not treated as spam? >> Maybe you could recommend dkim here or other ways to >> get better delivery. (Not sure myself, probably best >> to ask someone who operates like this so see if there's >> stuff to be said.) >> >> 5.1.4.1.3: typo: s/not store credential/not store >> credentials/ >> >> 5.1.4.1.4: salted hashes don't prevent offline >> dictionary attacks, they just increase the workload >> >> 5.1.5.1.4: would it be worthwhile recommending that >> different client credentials be used in development >> or integration mode vs. when operational? I've seen >> a bunch of times when programmers were given "live" >> API keys when that could've been avoided. >> >> 5.1.4.1.5: I don't get this. If it was only that >> easy:-) I think you need to say which private keys >> are used here and for what for this section to be >> useful. >> >> 5.1.4.2.1: I think you could note that complex password >> policies can also increase the liklihood that users >> re-use passwords or write them down or store them >> somewhere not so good, if e.g. the entropy required >> over all the user's passwords (dozens usually) is >> too great for long-term memory. >> >> 5.1.5.1: typo: s/Basis of/The basis of/ >> >> 5.1.5.1: typo: s/sensible service/sensitive service/ >> (2nd best typo:-) >> >> 5.1.5.3: typo: s/preciser/more precise/ >> >> 5.1.5.3: typo: s/refreshments/refreshes/ >> >> 5.1.5.4: 2nd bullet is not a threat but a goal >> >> 5.1.5.4: typo: s/redeem a/redeem an/ >> >> 5.1.5.5: what is a "rough" server? Do you mean rogue? >> >> 5.5.5.6: I think you need to clarify what "address" >> means here too. >> >> 5.1.5.9: please clarify if you mean digitally signed >> (asymmetric) or also include MACing here >> >> 5.1.5.10: typo: s/Self-contained/Self-contained tokens/ >> >> 5.1.5.10: s/privacy/confidentiality/ ? >> >> 5.1.5.10: this (typically, I'd guess) requires sharing >> symmetric keys between server nodes, so you should >> say that as its a bunch of work. >> >> 5.1.5.11: I don't get why you want to repeat this >> text (and its error:-) better to refer back to >> the earlier section I think. >> >> 5.1.5.12: Another place where a SAML reference would >> be needed. >> >> 5.1.6: 2nd bullet is not a "measure" but a goal. If >> you had something in mind, it doesn't come out from >> that text. >> >> 5.2.2.2: You say the binding should be protected, but >> don't say how. I think you ought. >> >> 5.2.3: typo: s/sub-sequent/subsequent/ but maybe >> better to delete the word >> >> 5.2.3: 2nd bullet - "trustworthiness" has gotta >> be the wrong word there, what did you mean? >> >> 5.2.3: typo: s/statics/statistics/ >> >> 5.2.3: typo: s/support achieve objectives/achieve these >> objectives/ ? >> >> 5.2.3: typo: s/by hand of an administrator/something >> better/ >> >> 5.2.3.1: "prevents...overestimating" seems wrong, I think >> you mean it reduces the probability of mistakenly treating >> a useless authentication as a good one. (The point is >> that servers don't "overestimate," only people do that.) >> >> 5.2.3.1: "cannot be entirely protected" seems like >> understatement - the reality is any such secret will >> leak out from anything that's any way successful. It >> only protects stuff that *nobody* cares about. >> >> 5.2.3.1: typo: s/trust on/trust/ But I'd rephrase it >> to not use the terms "trust" nor "identity" and then >> it'd be better I think. >> >> 5.2.3.2: typo? s/The authorization may/The authrozation >> server may/ ? Not sure what "issue a cliend id" means >> either. (Same in 5.2.3.3) >> >> 5.2.3.4: typo: s/A authorization server/An authorization >> server/ >> >> 5.2.3.5: what are "validated properties"? >> >> 5.2.3.5: what is the 1st bullet list on p57? there's >> no introductory text? >> >> 5.2.3.5: the "it" in "it only works" in the last >> paragraph isn't clear - which "it" do you mean? >> >> 5.2.3.5: saying discovery "gets involved" seems >> wrong - do you mean "is used"? And what is the >> "that" in "that's no longer feasible"? >> >> 5.2.3.6: typo: s/be unintentionally for/unintentionally >> affect/? >> >> 5.2.3.7: typo: s/to distribute client_secret/to >> distribute a client_secret/ >> >> 5.2.4.1: Is a "security certificate" a public key >> certificate? If so, saying that is better. >> >> 5.2.4.1: the references to 5.7.2.x are wrong and >> look like you're missing some xref's in the xml >> maybe >> >> 5.2.4.2: you said "address" again, so the usual >> question arises :-) >> >> 5.2.4.3: typo: s/in all situation/in situations/ >> (not sure "all" is right so suggest deleting it) >> >> 5.2.4.4: again, be good to say how to protect >> the binding >> >> 5.2.4.5: as for 5.2.4.4 say how binding is done >> >> 5.3.1: typo: s/a associated/an associated/ >> >> 5.3.1: "entirely protected" is (again:-) understatement >> see above for a suggestion >> >> 5.3.1: what does "trust on the client's identity" mean? >> Its not clear anyway >> >> 5.3.3: typo: s/operation systems/operating systems/ >> (enrire 2nd& 3rd paras could do with re-phrasing) >> >> 5.3.4: typo: s/their level/the level/ >> >> 5.3.5: this is too terse - is it really finished? I'd >> say there's a sentence or two missing. >> >> 5.4.2: what does it mean to "encourage resource >> servers" to do something? I guess you mean to encourage >> the people deploying those to pick resource servers >> with that capability and to turn that on. >> >> 5.4.2: not sure if everyone will know what a >> "distinguished name" is, maybe add a reference to >> rfc 5280? >> >> 5.4.2: token-bound secrets would be used to MAC >> the request and not to sign, be better to make that >> clear even if you still call that "signing" here >> (its not really, if we're being pedantic) >> >> 5.4.2: typo: s/This mechanisms/This mechanism/ >> >> 5.4.2 and 5.4.3: I forget - where are the protocol >> mechanisms for this defined? Can you give a reference >> or say that its future stuff if there aren't any >> good ones? >> >> 5.5: typo: s/capable to validate/capable of validating/ >> >> 8.1: Is the base spec really normative? I'd say you're >> fine with that as informative too. >> >> 8.2: there are a bunch of references missing as per >> the questions above >> >> 8.2: is there no better (more stable) reference than >> portablecontacts.net? >> >> >> >> >> >> >> >> >> >> >> _______________________________________________ >> 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
