[Swan-dev] a note on newoe-27-replace-sa-authnull-authnull's core dump

[Andrew Cagney]

Here's a change to consider (I'm not pushing it):

Along the lines of "2.21.3.  Error Handling after IKE SA is
Authenticated", when the state machine totally fails and there is a
protected (but not necessarily authenticated) channel, send back a
protected INVALID_SYNTAX and discard the state.

I suspect this is also the better thing to do when
encrypted_payload_status.bad but I didn't tweak that.
(and it should really be recording the response and then zombifying
the state so retransmits get handled)

Andrew






On Wed, 15 May 2019 at 16:55, Andrew Cagney <andrew.cagney at gmail.com> wrote:
>
> https://testing.libreswan.org/v3.27-1219-g7142d2c37-master/newoe-27-replace-sa-authnull-authnull/OUTPUT/east.pluto.log.gz
>
> FYI, several things go wrong.  Most notably, pluto's inability to
> handle an IKE_AUTH where the IKE SA succeeds but the CHILD SA fails.
>
> - IKE_SA_INIT is exchanged
>
> - first IKE_AUTH packet arrives:
> -- IKE SA successfully authenticates; it starts constructing the
> response; state is transitioned to established _but_ Message IDs are
> not updated (since things are still in flux it is too early)
> -- CHILD SA barfs and is deleted; the response is thrown away and the
> Message IDs aren't updated
>
> - duplicate IKE_AUTH packet arrives:
> -- the duplicate isn't recognized because the Message IDs don't line up
> -- state machine doesn't match as this is nonsensical
> -- then the crash
>
> I've stopped the crash with:
>
> "private-or-clear#192.1.3.0/24"[2] ...192.1.3.209 #3: dropping message
> with no matching microcode
-------------- next part --------------
diff --git a/programs/pluto/ikev2.c b/programs/pluto/ikev2.c
index c6f6560cd..ce9a1969c 100644
--- a/programs/pluto/ikev2.c
+++ b/programs/pluto/ikev2.c
@@ -2200,33 +2200,54 @@ void ikev2_process_state_packet(struct ike_sa *ike, struct state *st,
 				 */
 				send_v2N_response_from_md(md, v2N_INVALID_IKE_SPI,
 							  NULL/*no data*/);
-			} else {
+			} else if (ike != NULL && ike->sa.hidden_variables.st_skeyid_calculated) {
 				/*
-				 * Presumably things are pretty messed
-				 * up.  While there might be a state
-				 * there probably isn't an established
-				 * IKE SA (so don't even consider
-				 * trying to send an encrypted
-				 * response), for instance:
+				 * Hopefully:
+				 *
+				 * 2.21.3.  Error Handling after IKE
+				 * SA is Authenticated: [...] If a
+				 * peer parsing a request notices that
+				 * it is badly formatted (after it has
+				 * passed the message authentication
+				 * code checks and window checks) and
+				 * it returns an INVALID_SYNTAX
+				 * notification, then this error
+				 * notification is considered fatal in
+				 * both peers, meaning that the IKE SA
+				 * is deleted without needing an
+				 * explicit Delete payload.
+				 *
+				 * However, the more common case is
+				 * that this end is really messed up
+				 * messed up.
 				 *
-				 * - instead of an IKE_AUTH request,
-				 * the initiator sends something
-				 * totally unexpected (such as an
-				 * informational) and things end up
-				 * here
-
-				 * - when an IKE_AUTH request's IKE SA
-				 * succeeeds but CHILD SA fails (and
-				 * pluto screws up the IKE SA by
-				 * updating its state but not its
-				 * Message ID and not responding), the
-				 * re-transmitted IKE_AUTH ends up
-				 * here
+				 * For instance, when an IKE_AUTH
+				 * request's IKE SA succeeeds but
+				 * CHILD SA fails (and pluto screws up
+				 * the IKE SA by updating its state
+				 * but not its Message ID and not
+				 * responding), the re-transmitted
+				 * IKE_AUTH ends up here.
+				 */
+				libreswan_log("responding to message no matching microcode with encrypted INVALID_SYNTAX notification and discarding state");
+				send_v2N_response_from_state(ike, md, v2N_INVALID_SYNTAX,
+							     NULL/*no data*/);
+				/* i.e, discard_state(&ike->sa); */
+				change_state(&ike->sa, STATE_IKESA_DEL);
+				delete_state(&ike->sa);
+			} else {
+				/*
+				 * For instance, instead of an
+				 * IKE_AUTH request, the initiator
+				 * sends something totally unexpected
+				 * (such as an informational) and
+				 * things end up here.
 				 *
-				 * Should it send a non-encrypted
-				 * v2N_INVALID_SYNTAX?
+				 * The message hasn't been integrity
+				 * checked (if it had above would have
+				 * handled it) so just toss it.
 				 */
-				rate_log("dropping message with no matching microcode");
+				rate_log("discarding message with no matching microcode");
 			}
 		}
 		return;