[PATCH v4 0/8] Add MAC authentication support to the Babel protocol

[Ondrej Zajicek]

On Sun, May 30, 2021 at 11:12:04PM +0200, Toke Høiland-Jørgensen wrote:
> >> Toke Høiland-Jørgensen <toke at toke.dk> writes:
> >> 
> >> > This series adds MAC authentication support to the Babel protocol as specified
> >> > in in RFC8967:
> >> >
> >> > https://www.rfc-editor.org/rfc/rfc8967
> >> >
> >> > I have performed basic interoperability testing between this implementation and
> >> > the current babeld HMAC implementation[1]. The two implementations were able to
> >> > successfully exchange authenticated messages with both HMAC-256 and Blake2s-256
> >> > keys.
> >> >
> >> > Given the above, and the fact that the RFC was finally published at the the
> >> > IETF, I believe this series is ready for merging (subject to review, of course).
> >> > For those wanting to test the code, a version of Bird with this series applied
> >> > is available on Github[2] for easy consumption.
> >> >
> >> > [1] https://github.com/jech/babeld/pull/52
> >> > [2] https://github.com/tohojo/bird/tree/babel-mac-04
> 
> Ping? :)

Hi

Sorry for let you wait. I did most of the review some time ago, but got
distracted by other issues and not finished it. Now it is done, i did
some minor cleanups, but it is ready for merging.

I found a bug with getentropy() handling on BSD (it uses different return
value than getrandom(), so it ended in infinite loop).


Also, this part in babel_auth_check_pc() seems to me as buggy:

+  /* If index differs, send challenge */
+  if ((n->auth_index_len != msg->index_len ||
+      memcmp(n->auth_index, msg->index, msg->index_len)) &&
+      n->auth_next_challenge <= current_time())
+  {
+    LOG_PKT_AUTH("Index mismatch from %I on %s; sending challenge",
+                 msg->sender, ifa->ifname);
+    babel_auth_send_challenge(ifa, n);
+    return 1;
+  }

When the condition fails due to rate limiting (auth_next_challenge),
the execution continues in the babel_auth_check_pc(), which now assumes
that the index matches. IMHO it should be more like:

  if ((n->auth_index_len != msg->index_len) ||
      memcmp(n->auth_index, msg->index, msg->index_len))
  {
    LOG_PKT_AUTH("Index mismatch from %I on %s",
                 msg->sender, ifa->ifname);

    if (n->auth_next_challenge <= current_time())
      babel_auth_send_challenge(ifa, n);

    return 1;
  }

Comments?


I also changed 'key' config option to 'password' (so it is 'password'
with either ASCII string or hex-string). In future, we should probably
switch to 'key' for both variants, as that is the name generally used for
that. But using different keywords just for different notation of the
same concept seems confusing to me.

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."