Hi Derrell, thank you for your review. Please see inline.
> Hi, > > I am not the assigned SECDIR reviewer for this document, but I took a look > at draft-ietf-ipsecme-ikev2-downgrade-prevention-06 and wanted to pass along > a few security-review comments for Deb and the IESG. Thanks for this! > Short version: the core construction appears sound. Binding the final > IKE_SA_INIT request and response into the IKE_AUTH authentication inputs > should detect transcript tampering when both peers implement the extension > and at least one relevant authentication credential is not compromised. The > -06 revision also adds useful PSK/EAP caveats. > > The distinction I want to draw out is this: the extension proves that both > peers saw the same handshake. It does not by itself guarantee that the > strongest mutually supported handshake was selected, because the draft does > not make responder algorithm-selection behavior, or the retry transcript > boundary, normative. That distinction matters here because Section 4 uses > INVALID_KE_PAYLOAD as part of the motivating attack. > > A forged INVALID_KE_PAYLOAD is a pre-authentication message and is not part > of the bound transcript. An on-path attacker can use such a message to steer > an initiator into retrying with a Key Exchange payload for a weaker group. > Whether that becomes a downgrade then depends on the responder's selection > policy: does it select its preferred mutually supported group from the SA > proposal and reject a mismatching KE payload, or does it accept the group > that matches the retried KE payload? > > I do not have an empirically reproduced exploit. However, source inspection > suggests that implementation behavior is not uniform: > > - strongSwan and libreswan appear to select the responder proposal by local > preference and check the received KE payload only afterward. If the selected > group differs from the received KE payload, they respond with > INVALID_KE_PAYLOAD for the selected group rather than accepting a weaker > group merely because it appeared in a retry. This is the safe behavior. > > - OpenBSD iked appears to feed the received KE group into proposal selection. > In the current source, ikev2_sa_responder() passes msg->msg_dhgroup into > ikev2_sa_negotiate_common(), which passes it to proposals_negotiate(). > proposals_match() then enforces the DH transform matching that group. > With a policy offering both a stronger and weaker group in one proposal, > this appears to allow a retried KE payload for the weaker group to select > the weaker group, rather than producing INVALID_KE_PAYLOAD for the > responder's stronger preference. > > So the assumption the construction relies on -- that a responder will not let > an unauthenticated, attacker-influenced KE payload pull it to a weaker group > it would not otherwise have chosen -- holds in some implementations, but is > not stated by this draft. Since this document updates RFC 7296, I think that > assumption should be made explicit rather than left to implementer inference. I do not think that this draft should mandate how the responder should make its selection. Mostly because "weaker" and "stronger" are not always possible to correctly attribute to real KE methods. > Suggested clarification for Section 6: > > For the authentication calculations defined below, RealMessage1 and > RealMessage2 are the final accepted IKE_SA_INIT request and response, > respectively, as defined by Section 2.15 of [RFC7296]. If one or more > prior IKE_SA_INIT exchanges were rejected, for example due to COOKIE > or INVALID_KE_PAYLOAD, those rejected messages are not included in the > authenticated transcript. I think this is good clarification, but only for section 3. The reason is that this clarification applies to the original authentication data calculation and we do not redefine the meaning of RealMessage1 & RealMessage2. > Suggested responder-selection text: > > A responder implementing this specification MUST NOT let the > unauthenticated Key Exchange payload in a retried IKE_SA_INIT request > cause it to select a less-preferred key exchange method than it would > otherwise select from the same SA proposals according to local policy. > If the received Key Exchange payload does not match the selected key > exchange method, the responder MUST reject the request with > INVALID_KE_PAYLOAD as specified in [RFC7296]. > > This does not require a universal global preference order. It just prevents an > unauthenticated INVALID_KE_PAYLOAD from steering a responder into accepting a > lower-preference KE method it would not otherwise have selected. I think that literally this text requires the responder to always follow its local policy when selecting transforms with an accent on the INVALID_KE_PAYLOAD scenario. If we removed this accent, then this is obvious and even already stated in RFC 7296 (Section 3.3.4): Upon receipt of a payload with a set of Transform IDs, the implementation MUST compare the transmitted Transform IDs against those locally configured via the management controls, to verify that the proposed suite is acceptable based on local policy. The implementation MUST reject SA proposals that are not authorized by these IKE suite controls. Note that cryptographic suites that MUST be implemented need not be configured as acceptable to local policy. Since the responder does not know whether the request was restarted (in case INVALID_KE_PAYLOAD was sent by the attacker), this generic rules always applies and I don't see a need for an additional text in this draft. > Other comments: > > 1. The abstract could better reflect the actual security envelope. The > mechanism works when both peers implement it and at least one relevant > authentication credential remains uncompromised. The PSK/EAP limitations > are now discussed later, but the abstract still reads broadly. > > Suggested replacement: > > This document describes an extension to the Internet Key Exchange > protocol version 2 (IKEv2) in which the peers authenticate the final > IKE_SA_INIT transcript as part of IKE_AUTH. When both peers implement > the extension and at least one relevant authentication credential is > not compromised, this detects certain downgrade attacks on IKEv2. Thanks, looks good. I would slightly rephrase the first sentence: This document describes an extension to the Internet Key Exchange protocol version 2 (IKEv2) in which the peers authenticate the full IKE_SA_INIT transcript. > 2. The draft should explicitly state that notify stripping fails closed. > > Suggested text: > > If an attacker removes this notification from either direction when > both peers implement the extension, the peers will compute different > authentication inputs and the exchange will fail during IKE_AUTH. > Thus, attempts to suppress the extension do not silently disable its > protection when both peers implement it. OK, added as a note. > 3. Deployments that rely on this mechanism should have a local "require this > extension or abort" policy. Even if no wire change is needed, Security > Considerations should mention it operationally. > > Suggested text: > > Deployments that rely on this mechanism for downgrade resistance > should provide a local policy to require successful negotiation of > this extension and to abort the connection otherwise. OK. > 4. The "MUST ignore" rule for non-empty notification data seems loose. Unless > the WG has a concrete plan to extend this notify type with data, "MUST > reject non-empty notification data" would be simpler and more fail-closed. > If the WG wants future extensibility on the same notify type, the draft > should say that explicitly. There are no concrete plans now, but situation may change in future. Having this "open door" for possible future enhancements could simplify negotiation of them. Note, that RFC 9242 contains the same rule. > 5. Section 8 has minor typos: "attcker", "pevention", and "mesaage". Also > "Note, that" should be "Note that". Thanks, already fixed on github. > Overall, I think the document is on the right track. My main request is to > make > the retry transcript and responder-selection assumptions explicit, since at > least one open-source implementation appears not to satisfy the > responder-selection assumption today. This update to RFC 7296 should not rely > on implementer inference for that property. My position here is that RFC 7296 already contains the (obvious) requirement that the responder must follow its local policy when selecting transforms. And implementers are out of our control anyway. Thank you again, the PR is here: https://github.com/smyslov/ikev2-downgrade-prevention/pull/52 Regards, Valery. > Derrell _______________________________________________ IPsec mailing list -- [email protected] To unsubscribe send an email to [email protected]
