Issue with no_hmac_verify flag

[jmuchemb]

When starting from a network that does not use the HMAC that is in development, we consider that a proper way to enable it is to do it in 2 steps, so that all nodes don't need to restart with the new configuration at the same time.

The intermediate state is to have a network that is made of 2 kinds to nodes: those with initial configuration (no hmac settings at all), and other that sign packets but accept unsigned packets. For that, no_hmac_verify works.

Then, once all nodes use the intermediate configuration, we want nodes to restart without the no_hmac_verify settings. We started with a single node and found that it couldn't communicate with neighbours. babeld.log is full of _Received wrong PC or failed the challenge. _ messages.

This whole scenario worked with #35. Here is a patch that adapts this PR for the current hmac branch:

fixup! Add no_hmac_verify flag

Commit a62b7c9b6dbab378d69a523a10a9de33c2624fbb was broken in that
2 nodes with same id/hmac settings could not communicate when
1 of the 2 has no_hmac_verify.

diff --git a/hmac.c b/hmac.c
index 769daf4..cb07416 100644
--- a/hmac.c
+++ b/hmac.c
@@ -259,6 +259,7 @@ check_hmac(const unsigned char *packet, int packetlen, int bodylen,
 {
     int i = bodylen + 4;
     int len;
+    int rc = -1;
 
     debugf("check_hmac %s -> %s\n",
           format_address(src), format_address(dst));
@@ -278,8 +279,9 @@ check_hmac(const unsigned char *packet, int packetlen, int bodylen,
                               packet + i + 2, len, ifp->key);
            if(ok)
                return 1;
+           rc = 0;
        }
        i += len + 2;
     }
-    return 0;
+    return rc;
 }
diff --git a/message.c b/message.c
index c57a0d2..4ea1aea 100644
--- a/message.c
+++ b/message.c
@@ -580,21 +580,24 @@ parse_packet(const unsigned char *from, struct interface *ifp,
         bodylen = packetlen - 4;
     }
 
-    if(ifp->key != NULL && !(ifp->flags & IF_NO_HMAC_VERIFY)) {
-        if(check_hmac(packet, packetlen, bodylen, from, to, ifp) != 1) {
-            fprintf(stderr, "Received wrong hmac.\n");
-            return;
-        }
-
-        neigh = find_neighbour(from, ifp);
-        if(neigh == NULL) {
-            fprintf(stderr, "Couldn't allocate neighbour.\n");
-            return;
-        }
-
-        if(preparse_packet(packet, bodylen, neigh, ifp) == 0) {
-            fprintf(stderr, "Received wrong PC or failed the challenge.\n");
-            return;
+    if(ifp->key != NULL) {
+        switch(check_hmac(packet, packetlen, bodylen, from, to, ifp)) {
+            case -1:
+                if(ifp->flags & IF_NO_HMAC_VERIFY)
+                    break; /* missing key ignored */
+            case 0:
+                fprintf(stderr, "Received wrong hmac.\n");
+                return;
+            case 1:
+                neigh = find_neighbour(from, ifp);
+                if(neigh == NULL) {
+                    fprintf(stderr, "Couldn't allocate neighbour.\n");
+                    return;
+                }
+                if(preparse_packet(packet, bodylen, neigh, ifp) == 0) {
+                    fprintf(stderr, "Received wrong PC or failed the challenge.\n");
+                    return;
+                }
         }
     }
 

But no_hmac_verify does not even seem to be a good name. I have a preference for the name we chose in #35: the goal is not to skip checking but rather ignore when there's no HMAC.

[jmuchemb]

(#52)

[MisterDA]

the goal is not to skip checking but rather ignore when there's no HMAC.

What difference does it make to reject incorrectly-signed packets?

On a side note, my code e314264 is probably wrong. The name babel-mac-verify comes from the Information Model.

[jmuchemb]

the goal is not to skip checking but rather ignore when there's no HMAC.

What difference does it make to reject incorrectly-signed packets?

The issue is that correctly-signed packets are rejected.

I have the feeling to have said everything, but I can try to explain differently. Let's given 3 kinds of nodes:

1. nodes without any hmac-related option given to babeld command-line
2. nodes with hmac configured with 1 key, and some whatever flag (no_hmac_verify, hmac_verify, ignore_no_hmac) at a non-default value
3. nodes with hmac configured with 1 key and the flag at a default value

We expects that:

1. nodes 1 can communicate with nodes 2
2. nodes 2 can communicate with nodes 3
3. nodes 1 can not communicate with nodes 3

#35 verifies the above 3 cases. The hmac branch of jech/babeld (91a9259) fails for case 2.

Otherwise, what would be the purpose of the discussed flag if it's not to provide a way to do a smooth upgrade to using HMAC ?

On a side note, my code e314264 is probably wrong.

Oops, I didn't notice that #52 has more commits than the hmac branch of jech/babeld. We use the latter.

About e314264 I don't see any code initializing hmac_verify to the default value of true.

[MisterDA]

Your presentation with the three kinds of nodes is very clear, thanks.
I applied (b909a58) your commit with some minor changes into my branch (#52).

About e314264 I don't see any code initializing hmac_verify to the default value of true.

Thanks, fixed in 7b79301.

[jech]

I'm sorry if I'm daft, Julien, but I still don't understand.

The intent of the old code is that no HMAC verification is made at all if NO_HMAC_VERIFY is set. In other words, HMACs are ignored on reception, unconditionally. This is a simple semantics, and one that's easy to explain.

It looks like the code is buggy; that's possible, in which case I'll be grateful for a bugfix.

Your patch, however, changes the logic: now unsigned packets are accepted, but incorrectly signed packets (e.g. packets signed with an obsolete key) are rejected. This is a complicated semantics, one that is not easy to explain, and one that I don't feel is useful.

Would you be willing to provide a fix for the bug without changing the intent of the code?

[jech]

The flag is now called accept-bad-signatures, and causes the interface to accept both unsigned packets and packets with an incorrect key. Rejecting the latter provides no additional security, since an attacker can simply send unsigned packets.

Please let me know if there's still something missing.