<div dir="ltr"><div><div><div><br><br>On 3 March 2015 at 16:42, D. Hugh Redelmeier &lt;<a href="mailto:hugh@mimosa.com">hugh@mimosa.com</a>&gt; wrote:<br>&gt; | From: Andrew Cagney &lt;<a href="mailto:andrew.cagney@gmail.com">andrew.cagney@gmail.com</a>&gt;<br>&gt;<br>&gt; (I didn&#39;t write the centralized V2 code driven by the microcode but it<br>&gt; was kind of copied from what I did in V1.)<br>&gt;<br>&gt; | - I earlier posted questions related to ikev2_process_payloads() - it<br>&gt; | is, to me, doing more than it should.<br>&gt; | By moving its search logic into the main search-for-state-transition<br>&gt; | loop things get more transparent, and SMF2_CONTINUE_MATCH (which<br>&gt; | scares me) can also be deleted.<br>&gt; | In addition to checking the clear payload, SMF2_UNPACK_SK indicates<br>&gt; | that the SK (encrypted) payload can be checked - less stuff for my<br>&gt; | rekey states to deal with<br>&gt;<br>&gt; The following is based on my imperfect memory.  With no checking.<br>&gt; Beware.<br>&gt;<br>&gt; I hacked in the SMF2_CONTINUE_MATCH code.  Why?<br>&gt;<br>&gt; - decryption was not done by the central routine: the per-transition<br>&gt;   routines do it.<br>&gt;<br>&gt; - in some cases, decryption exposes information that affects microcode<br>&gt;   selection.  So a second go at matching was required<br>&gt;   (SMF2_CONTINUE_MATCH).<br><br>It sounds like the code in this area has evolved then.  Thanks for the background.<br><br>Currently it is only used to differentiate between two I1 transitions - expected INIT reply and some other INIT reply such as INVALID_KE.  Both of these cases are deal with clear text.<br><br>&gt; Some thoughts:<br>&gt;<br>&gt; - I think that it would be better to have the centralized routine do<br>&gt;   the decryption before calling the appropriate per-transition<br>&gt;   routine.<br><br>I did the change in stages:<br><br>- first I split ikev2_process_payload into several parts:<br>-&gt; code to decode the payload and record the results<br>-&gt; code to check the results match expected<br>-&gt; code to log unexpected results<br>the code looping over SMF2_CONTINUE_MATCH could then be moved into process_v2_packet so that contained separate loops<br><br>- then I merged the two loops<br><br>- finally I added stuff to continue to the SK payload if the applicable bit is set; it just re-used the above (along with a tweaked decrypt payload method)<br><br>&gt; - I think that there are some cases where a bit of transition-specific<br>&gt;   code is needed before the keying material for decryption is<br>&gt;   available.  (So it was easier to hack in what I did rather than<br>&gt;   making larger changes.)<br><br>Yes. In pluto for IKEv2, the original responder only completes the DH calculation when the AUTH request arrives (it&#39;s a smart trick).  In V1 it is done in the background.  This is why I added a flag to mark that things could be decrypted (and allow smother migration).<br><br>&gt; - ikev2_process_payloads gets called twice on an encrypted message:<br>&gt;   the first gobbles just the single E payload; the second gobbles the many<br>&gt;   payloads within the decrypted E payload.<br><br>&gt; - I think that it would be clearer if the E payload were special-cased<br>&gt;   and that ikev2_process_payload were only called once<br>&gt;   and that the payload checking only be done once<br>&gt;   and only then would the per-transition function be called.<br>&gt;   But the proof would be if the code were  simplified by this.<br><br>See above, it looks something like this:<br><br><font face="monospace,monospace">        for (svm = v2_state_microcode_table; svm-&gt;state != STATE_IKEv2_ROOF;<br>             svm++) {<br>                if (svm-&gt;state != from_state)<br>                        continue;<br>                if (svm-&gt;recv_type != ix)<br>                        continue;<br>                /*<br>                 * Does the original initiator flag match?<br>                 */<br>                if ((svm-&gt;flags &amp; SMF2_IKE_I_SET) &amp;&amp; !ike_i)<br>                        continue;<br>                if ((svm-&gt;flags &amp; SMF2_IKE_I_CLEAR) &amp;&amp; ike_i)<br>                        continue;<br>                /*<br>                 * Does the message reply flag match?<br>                 */<br>                if ((svm-&gt;flags &amp; SMF2_MSG_R_SET) &amp;&amp; !msg_r)<br>                        continue;<br>                if ((svm-&gt;flags &amp; SMF2_MSG_R_CLEAR) &amp;&amp; msg_r)<br>                        continue;<br>                /*<br>                 * Since there&#39;s a state that, at least, looks like it<br>                 * will accept the packet, unpack the clear payload<br>                 * and continue matching.<br>                 */<br>                if (clear_payload_summary.status != STF_OK) {<br>                        DBG(DBG_CONTROL, DBG_log(&quot;Unpacking clear payload for svm: %s&quot;, svm-&gt;story));<br>                        clear_payload_summary = ikev2_decode_payloads(md, &amp;md-&gt;message_pbs,<br>                                                                      md-&gt;hdr.isa_np);<br>                        if (clear_payload_summary.status != STF_OK) {<br>                                complete_v2_state_transition(mdp, clear_payload_summary.status);<br>                                return;<br>                        }<br>                }<br>                struct ikev2_payload_errors clear_payload_errors<br>                        = ikev2_verify_payloads(clear_payload_summary, svm, FALSE);<br>                if (clear_payload_errors.status != STF_OK) {<br>                        /* Save this failure for later logging. */<br>                        clear_payload_status = clear_payload_errors;<br>                        continue;<br>                }<br>                /*<br>                 * Continue checking by unpacking the SK payload but<br>                 * only its ok and there&#39;s one to unpack.  Otherwise<br>                 * assume its a correct match.<br>                 */<br>                if (!(svm-&gt;flags &amp; SMF2_UNPACK_SK)) {<br>                        break;<br>                }<br>                if (!(svm-&gt;req_clear_payloads &amp; LELEM(PINDEX(ISAKMP_NEXT_v2SK)))) {<br>                        break;<br>                }<br>                if (enc_payload_summary.status != STF_OK) {<br>                        DBG(DBG_CONTROL,<br>                            DBG_log(&quot;Unpacking encrypted payload for svm: %s&quot;,<br>                                    svm-&gt;story));<br>                        stf_status status = ikev2_verify_and_decrypt_sk_payload(md);<br>                        if (status != STF_OK) {<br>                                complete_v2_state_transition(mdp, status);<br>                                return;<br>                        }<br>                        unsigned np = md-&gt;chain[ISAKMP_NEXT_v2SK]-&gt;payload.generic.isag_np;<br>                        enc_payload_summary = ikev2_decode_payloads(md, &amp;md-&gt;clr_pbs, np);<br>                        if (enc_payload_summary.status != STF_OK) {<br>                                complete_v2_state_transition(mdp, enc_payload_summary.status);<br>                                return;<br>                        }<br>                }<br>                struct ikev2_payload_errors enc_payload_errors<br>                        = ikev2_verify_payloads(enc_payload_summary, svm, TRUE);<br>                if (enc_payload_errors.status != STF_OK) {<br>                        /* Save this failure for later logging. */<br>                        enc_payload_status = enc_payload_errors;<br>                        continue;<br>                }<br>                /* must be the right state machine entry */<br>                break;<br>        }<br><br></font></div>(inside verify_and_decrypt_payload there&#39;s a check that crypto is ready, I should probably make that explicit here)<br></div>SMF2_UNPACK_SK provides a migration path and addresses the original responder trick.<br><br></div><div><div><div><div>&gt; My feeling is that a significant amount of code that is replicated in<br>&gt; several per-transition functions could be moved into a central routine<br>&gt; and gated by microcode.  That was the motivation for the microcode<br>&gt; construct in V1.<br><br>Events, such as timeouts, and even request initiation should get dispatched by the state machine.  Make this state machine behave like a state machine.<br><br>&gt; An example of centralizing code is moving the timeout-setting logic out of<br>&gt; the per-transition functions.  I think that Antony has done (most of?<br>&gt; all of?) this.<br>&gt; _______________________________________________<br>&gt; Swan-dev mailing list<br>&gt; <a href="mailto:Swan-dev@lists.libreswan.org">Swan-dev@lists.libreswan.org</a><br>&gt; <a href="https://lists.libreswan.org/mailman/listinfo/swan-dev">https://lists.libreswan.org/mailman/listinfo/swan-dev</a><br></div></div></div></div></div>