OK, here’s a stab at a new security considerations section on X.509 parsing and validation:
--- 5.3 X.509 Certificate Parsing and Validation Complexity Parsing and validation of X.509 certificates and certificate chains is complex and implementation mistakes have previously exposed security vulnerabilities. Complexities of validation include (but are not limited to) [1][2][3]: - checking of Basic Constraints, basic and extended Key Usage constraints, validity periods, and critical extensions; - handling of null-terminator bytes and non-canonical string representations in subject names; - handling of wildcard patterns in subject names; - recursive verification of certificate chains and checking certificate revocation. For these reasons, implementors SHOULD use an established and well-tested TLS library for validation of X.509 certificate chains and SHOULD NOT attempt to write their own X.509 certificate validation procedures. [1] https://www.cryptologie.net/article/374/common-x509-certificate-validationcreation-pitfalls/ <https://www.cryptologie.net/article/374/common-x509-certificate-validationcreation-pitfalls/> [2] http://www.cs.utexas.edu/~shmat/shmat_ccs12.pdf <http://www.cs.utexas.edu/~shmat/shmat_ccs12.pdf> [3] https://tools.ietf.org/html/rfc5280 <https://tools.ietf.org/html/rfc5280> --- NB - this blog post [1] is the best concise summary of attacks I could find. Most of these attacks have been published as Black Hat talks and I can’t seem to find definitive references or good survey papers (beyond [2], which is a little older). Let me know what you think, Neil > On 12 Apr 2018, at 20:42, Brian Campbell <bcampb...@pingidentity.com> wrote: > > Thanks Neil. > > Other than the potential metadata changes, which I'd like more WG input on > and may raise in a new thread, I think I've got enough to make updates > addressing your comments. But please do send text for that Security > Considerations bit, if you come up with something. > > On Thu, Apr 12, 2018 at 3:03 AM, Neil Madden <neil.mad...@forgerock.com > <mailto:neil.mad...@forgerock.com>> wrote: > Hi Brian, > > Thanks for the detailed responses. Comments in line below (marked with ***). > > Neil > > On Wednesday, Apr 11, 2018 at 9:47 pm, Brian Campbell > <bcampb...@pingidentity.com <mailto:bcampb...@pingidentity.com>> wrote: > Thanks for the review and feedback, Neil. I apologize for my being slow to > respond. As I said to Justin recently > <https://mailarchive.ietf.org/arch/msg/oauth/cNmk8fSuxp37L-z8Rvr6_EnyCug>, > I've been away from things for a while. Also there's a lot here to get > through so took me some time. > > It looks like John touched on some of your comments but not all. I'll try and > reply to them as best I can inline below. > > > On Thu, Mar 29, 2018 at 9:18 AM, Neil Madden <neil.mad...@forgerock.com > <mailto:neil.mad...@forgerock.com>> wrote: > Hi, > > I have reviewed this draft and have a number of comments, below. ForgeRock > have not yet implemented this draft, but there is interest in implementing it > at some point. (Disclaimer: We have no firm commitments on this at the > moment, I do not speak for ForgeRock, etc). > > 1. https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-3.1 > <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-3.1> defines a > new confirmation method “x5t#S256”. However, there is already a confirmation > method “jwk” that can contain a JSON Web Key, which itself can contain a > “x5t#S526” claim with exactly the same syntax and semantics. The draft > proposes: > > { “cnf”: { “x5t#S256”: “…” } } > > but you can already do: > > { “cnf”: { “jwk”: { … , “x5t#S256”: “…” } } } > > If the intent is just to save some space and avoid the mandatory fields of > the existing JWK types, maybe this would be better addressed by defining a > new JWK type which only has a thumbprint? e.g., { “kty”: “x5t”, “x5t#S256”: > “…” }. > > The intent of the x5t#S256 confirmation method was to be space efficient and > straightforward while utilizing the framework and registry that RFC 7800 > gives. Even a new JWK type like that would still use more space. And I'd > argue that the new confirmation method is considerably more straightforward > than registering a new JWK type (and the implications that would have on JWK > implementations in general) in order to use the existing "jwk" confirmation > method. > > *** > OK, that is reasonable. Given that the draft says SHOULD rather than MUST for > using this confirmation key method, I think it is currently allowed to use > either representation. > > > > 2. I find the naming “mutual TLS” and “mTLS” a bit of a misnomer: it’s really > only the client authentication that we are interested here, and the fact that > the server also authenticates with a certificate is not hugely relevant to > this particular spec (although it is to the overall security of OAuth). Also, > TLS defines non-certificate based authentication mechanisms (e.g. TLS-SRP > extension for password authenticated key exchange, PSK for pre-shared key > authentication) and even non-X.509 certificate types > (https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#tls-extensiontype-values-3 > > <https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#tls-extensiontype-values-3>). > I’d prefer that the draft explicitly referred to “X.509 Client Certificate > Authentication” rather than mutual TLS, and changed identifiers like > ‘tls_client_auth’ > (https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1.1 > <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1.1>) to > something more explicit like ‘tls_x509_pki_client_auth’. > > This is especially confusing in section 3 on sender constrained access > tokens, as there are two different servers involved: the AS and the protected > resource server, but there is no “mutual” authentication between them, only > between each of them and the client. > > Choosing names and terminology is difficult and the "right" wording is often > subjective. I believe that the current wording sufficiently conveys what is > going on in the draft to most readers. Most readers thus far seem to agree. > There is some text now that does say that the mutual auth in the draft is in > fact X.509 client cert authn but, in the next revision, I'll look for other > opportunities where it could be stated more clearly. > > *** Thanks. > > > > 3. The draft links to the TLS 1.2 RFC, while the original OAuth 2.0 RFC only > specifies TLS 1.0. Is the intention that TLS 1.2+ is required? The wording in > Section 5.1 doesn’t seem clear if this could also be used with TLS 1.0 or > 1.1, or whether it is only referring to future TLS versions. > > The reference to BCP 195 (which unfortunately the original OAuth 2.0 RFC > doesn't have because it didn't exist then) is meant to account for changing > versions and recommendations around TLS. Currently that BCP says TLS 1.2 is a > must and suggests against 1.1 & 1.0 but doesn't outright prohibit them. > > *** OK, that seems good to me. > > > > 4. It might be useful to have a discussion for implementors of whether TLS > session resumption (and PSK in TLS 1.3) and/or renegotiation impact the use > of client certificates, if at all? > > That might well be useful but I don't myself know what it would say. I've > (maybe naively) figured those are deployment details that will just work out. > Perhaps you could propose some text around such a discussion that the WG > could consider? > > *** > To be honest, when I raised this it was because I didn’t really know what the > implications were. I’ve done some reading around and I think it should all > just work - both session resumption and PSK-based resumption preserve the > original client and server authentication context so we can assume that any > presented client cert is still valid for the resumed session. So I think we > can leave out any discussion of this and assume it works as expected. > > > > 5. Section 3 defines sender-constrained access tokens in terms of the > confirmation key claims (e.g., RFC 7800 for JWT). However, the OAuth 2.0 Pop > Architecture draft defines sender constraint and key confirmation as > different things > (https://tools.ietf.org/html/draft-ietf-oauth-pop-architecture-08#section-6.2 > <https://tools.ietf.org/html/draft-ietf-oauth-pop-architecture-08#section-6.2>). > The draft should decide which of those it is implementing and if sender > constraint is intended, then reusing the confirmation key claims seems > misleading. (I think this mTLS draft is doing key confirmation so should drop > the language about sender constrained tokens). > > I will say again that choosing names and terminology is difficult... > > Although I must admit that I started using "sender constrained" somewhat > indiscriminately at first and it's just sort of stuck. At the time I was > trying to incorporate bits of draft-sakimura-oauth-jpop in a way that might > help bring on and keep the authors of that draft onboard with this mtls > draft. In retrospect it looks like I did that part wrong anyway. But that was > the thinking at the time and the history, for whatever it's worth. More > recently, Nat was requesting that "sender constrained" be included in the > title. So there's that too. > > In defense of my mistake, however, if there's a line between "sender > constrained" and "key confirmation" tokens, it's a bit of a fuzzy line. And > I'd argue that what this draft is doing pretty close to the line. > > But ultimately I think you are right that what this mtls draft is doing with > access tokens is more accurately described with the key confirmation term. > > So, yes, the draft should probably drop (or at least minimize) the language > about sender constrained. I'll do that in the next draft version, barring big > objections from the WG. > > The tricky thing with making that change is that there a client and server > metadata parameters with the name > "mutual_tls_sender_constrained_access_tokens" that should probably also > change. But that would be a breaking change (of sorts anyway), which > shouldn't be taken lightly at this stage. I feel that some explicit okays > from the WG are needed before doing so (rough consensus stye) . Any WG > members want to weigh in here and help get a "sense of the group" concerning > changing those metadata names? > > *** Thanks. I agree this might cause compatibility issues. It is not a big > issue for us, but might cause some confusion. > > > > 6. The OAuth 2.0 PoP Architecture draft says > (https://tools.ietf.org/html/draft-ietf-oauth-pop-architecture-08#section-5 > <https://tools.ietf.org/html/draft-ietf-oauth-pop-architecture-08#section-5>): > > Strong, fresh session keys: > > Session keys MUST be strong and fresh. Each session deserves > an > independent session key, i.e., one that is generated > specifically > for the intended use. In context of OAuth this means that > keying > material is created in such a way that can only be used by the > combination of a client instance, protected resource, and > authorization scope. > > > However, the mTLS draft section 3 > (https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-3 > <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-3>) says: > > The client makes protected resource requests as described in > [RFC6750], however, those requests MUST be made over a mutually > authenticated TLS connection using the same certificate that was used > for mutual TLS at the token endpoint. > > These two statements are contradictory: the OAuth 2.0 PoP architecture > effectively requires a fresh key-pair to be used for every access token > request, whereas this draft proposes reusing the same long-lived client > certificate for every single access token and every resource server. > > In the self-signed case (and even in the CA case, with a bit of work - e.g., > https://www.vaultproject.io/docs/secrets/pki/index.html > <https://www.vaultproject.io/docs/secrets/pki/index.html>) it is perfectly > possible for the client to generate a fresh key-pair for each access token > and include the certificate on the token request (e.g., as per > https://tools.ietf.org/html/draft-ietf-oauth-pop-key-distribution-03#section-5.1 > > <https://tools.ietf.org/html/draft-ietf-oauth-pop-key-distribution-03#section-5.1> > - in which case an appropriate “alg” value should probably be described). > This should probably at least be an option. > > This draft doesn't necessarily seek to align with the (long expired) PoP > architecture draft. Rather it is aiming to provide a pragmatic solution for > PoP style access tokens and OAuth client auth using mTLS client certificates. > > That said, with the current draft, it's certainly possible for a client to > update its cert more frequently, even so far as using a new one for each > access token. The details of how that would work will vary some based on the > token endpoint authentication method. But it's not precluded. > > *** You are right, the text doesn’t preclude that. I am happy with that > solution. I suspect most people will deploy and be happy with reusing a > long-lived cert for every access token, so this may not matter in practice. > > > 7. The use of a single client certificate with every resource server (RS) > should be called out in a Privacy Considerations section, as it allows > correlation of activity. > > Practically speaking the access tokens being presented likely already have > correlatable info in them about the client as well as the user. I don't know > that there's much of a (new) concern in reality. If you feel this concern is > unique and import enough though, perhaps there's some text you'd like to > propose for a Privacy Considerations section that the WG could consider? I > mean, I guess it doesn't hurt to mention it but I would like to avoid > overstating the issue. > > *** On reflection, I am going to withdraw this comment. As you say there are > other ways to correlate clients. The privacy issue would mainly arise in the > context of dynamic client registration e.g., a pattern I’ve seen described > where every instance of a mobile app does dynamic client registration to > avoid including credentials directly in the app bundle. This would make > clients one-to-one with users. But (a) those apps are fairly unlikely to be > using TLS certs, and (b) that is more of a privacy consideration for dynamic > client registration rather than this draft. > > > > 8. This is maybe a more general point, but RFC 6750 defines the > Authorization: Bearer scheme (https://tools.ietf.org/html/rfc6750#section-2 > <https://tools.ietf.org/html/rfc6750#section-2>) for a client to communicate > it’s access token to the RS in a standard way. As sender-constrained access > tokens are not strictly bearer tokens any more, should this draft also > register a new scheme for that? Should there be a generic PoP scheme? > > The thinking and general consensus (in this draft as well as the OAuth token > binding work) has been that continuing to use the RFC 6750 scheme while > putting the "not strictly bearer" stuff in (or referenced by) the token > itself will be easier on deployment and implementation. And better for > adoption as a result. I believe some early implementation work has borne that > out too. > > *** OK. > > > 9. The Security Considerations should really make some mention of the long > history of attacks against X.509 certificate chain validation, e.g. failure > to check the “CA” bit in the basic constraints, errors in parsing DNs, etc. > It should be strongly suggested to use an existing TLS library to perform > these checks rather than implementing your own checks. This relates to > Justin’s comments around DN parsing and normalisation. > > Suggesting to use an existing TLS library is certainly sound advice and I > sort of felt is implied in the draft. But saying so more strongly/explicitly > might be worthwhile. And pointing to historical reasons to do so would > probably be good too. Could you propose a new Security Considerations > section or maybe augmentation of §5.2 with that content? > > *** I’ll try and come up with some text. > > > > 10. The PKI client authentication method > (https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1 > <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07#section-2.1>) makes no > mention at all of certificate revocation and how to handle checking for that > (CRLs, OCSP - with stapling?). Neither does the Security Considerations. If > this is a detail to be agreed between then AS and the CA (or just left up to > the AS TLS stack) then that should perhaps be made explicit. Again, there are > privacy considerations with some of these mechanisms, as OCSP requests are > typically sent in the clear (plain HTTP) and so allow an observer to see > which clients are connecting to which AS. > > I didn't think that a TLS client could do OCSP stapling? > > *** I think you are right about this. I always assumed it was symmetric (and > I think it technically could work), but the spec only talks about stapling in > the server-side of the handshake. > > That aside, revocation checking (how and even if) is something that's at the > discretion of the AS. I can add something in §2.1 to say that more > explicitly. > > *** Thanks. > > > > 11. The same comment applies to how the protected resource checks for > revocation of the certificate presented during sender constrained access > token usage. Should the RS make its own revocation checks based on the > information in the certificate presented, or should it trust the certificate > while the access token is still valid? If the latter case, is the AS > responsible for revoking any access tokens whose certificate have been > revoked (if so, should it be doing an OCSP call on every token introspection > request, and should the RS be passing on the certificate/serial number on > that request)? If the Client request uses OCSP Stapling > (https://en.wikipedia.org/wiki/OCSP_stapling > <https://en.wikipedia.org/wiki/OCSP_stapling>) how can the RS verify the > signature on that if it does not have a separate trust relationship with the > CA already? > > The draft effectively uses cert mtls at the RS as a proof-of-possession > method only and not as authentication. So revocation checking isn't really > applicable. In specific deployment situations, I suppose an RS could check > revocation. But that'd be a deployment decision for the RS that's beyond the > scope of this draft. > > *** OK, that is an interesting observation. If either the client or AS > suspect the key has been compromised they can revoke the access token(s) > instead. > > > > 12. The use of only SHA-256 fingerprints means that the security strength of > the sender-constrained access tokens is limited by the collision resistance > of SHA-256 - roughly “128-bit security" - without a new specification for a > new thumbprint algorithm. An implication of this is that is is fairly > pointless for the protected resource TLS stack to ever negotiate cipher > suites/keys with a higher level of security. In more crystal ball territory, > if a practical quantum computer becomes a possibility within the lifetime of > this spec, then the expected collision resistance of SHA-256 would drop > quadratically, allowing an attacker to find a colliding certificate in ~2^64 > effort. If we are going to pick just one thumbprint hash algorithm, I would > prefer we pick SHA-512. > > The idea behind haveing just one thumbprint hash algorithm was to keep things > simple. And SHA-256 seems good enough for the reasonably foreseeable future > (and space aware). Also a new little spec to register a different hash > algorithm, should the need arise, didn't seem particularity onerous. > > That was the thinking anyway. Maybe it is too short sighted though? > > I do think SHA-256 should stay regardless. > > But the draft could also define SHA-512 (and maybe others). What do you and > WG folks think about that? > > *** Yes please. > > It would probably then be useful for the metadata in §3.3 and §3.4 to change > from just boolean values to something to convey what hash alg/cnf method the > client expects and the list of what the server supports. That's maybe > something that should be done anyway. That'd be a breaking change to the > metadata. But there's already another potential breaking change identified > earlier in this message. So maybe it's worth doing... > > How do folks feel about making this kind of change? > > > > > > Cheers, > > Neil > > > > On 19 Mar 2018, at 22:34, Rifaat Shekh-Yusef <rifaat.i...@gmail.com > > <mailto:rifaat.i...@gmail.com>> wrote: > > > > All, > > > > As discussed during the meeting today, we are starting a WGLC on the MTLS > > document: > > https://tools.ietf.org/html/draft-ietf-oauth-mtls-07 > > <https://tools.ietf.org/html/draft-ietf-oauth-mtls-07> > > > > Please, review the document and provide feedback on any issues you see with > > the document. > > > > The WGLC will end in two weeks, on April 2, 2018. > > > > Regards, > > Rifaat and Hannes > > > > _______________________________________________ > > OAuth mailing list > > OAuth@ietf.org <mailto:OAuth@ietf.org> > > https://www.ietf.org/mailman/listinfo/oauth > > <https://www.ietf.org/mailman/listinfo/oauth> > > > _______________________________________________ > OAuth mailing list > OAuth@ietf.org <mailto:OAuth@ietf.org> > https://www.ietf.org/mailman/listinfo/oauth > <https://www.ietf.org/mailman/listinfo/oauth> > > > > CONFIDENTIALITY NOTICE: This email may contain confidential and privileged > material for the sole use of the intended recipient(s). Any review, use, > distribution or disclosure by others is strictly prohibited. If you have > received this communication in error, please notify the sender immediately by > e-mail and delete the message and any file attachments from your computer. > Thank you. > > > CONFIDENTIALITY NOTICE: This email may contain confidential and privileged > material for the sole use of the intended recipient(s). Any review, use, > distribution or disclosure by others is strictly prohibited. If you have > received this communication in error, please notify the sender immediately by > e-mail and delete the message and any file attachments from your computer. > Thank you.
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
