[PATCH RFC 2/2] babel: Add HMAC support
Ondrej Zajicek
santiago at crfreenet.org
Tue Aug 14 19:18:37 CEST 2018
Hi
Finally got to finish the review. The code looks OK, comments below. The
getrandom() issue and issue with fixed length of indexes and so on was
already mentioned before. I am OK with reasonable fixed upper bound, but
the code should be able to handle received shorter ones.
I have an issue with the way how the code is structured. Esp. the
validation of received packet is split to too many independently called
parts that it is hard to follow. I think there should be one entry point
called from babel_process_packet(), that would do all necessary checks
desribed in section 4.3 of the draft. That is similar to how it is done
e.g. in RIP - rip_fill_authentication() / rip_check_authentication().
See the comments for babel_process_packet() below.
Also minor terminological note: fields that store pc, index and nonce
should not have prefix hmac_, as they are part of replay-protection, not
HMAC itself. You can have different MAC than HMAC and still use the same
replay-protection. So perhaps call call them auth_pc, auth_index and so
on.
On Fri, Jul 13, 2018 at 10:16:16PM +0200, Toke Høiland-Jørgensen wrote:
> @@ -1377,6 +1378,79 @@ babel_handle_seqno_request(union babel_msg *m, struct babel_iface *ifa)
> }
> }
>
> +void
> +babel_hmac_send_challenge(struct babel_iface *ifa, struct babel_neighbor *n)
> +{
> + struct babel_proto *p = ifa->proto;
> + union babel_msg msg = {};
> +
> + if (n->hmac_next_challenge > current_time())
> + {
> + TRACE(D_PACKETS, "Not sending HMAC challenge to %I due to rate limiting");
> + return;
> + }
Logging of not sending packet is unnecessary, and the check
should be done by caller.
> +
> + TRACE(D_PACKETS, "Sending HMAC challenge to %I on iface %s",
> + n->addr, ifa->ifname);
Just 'on %s' instead of 'on iface %s'.
> + getrandom(n->hmac_nonce, BABEL_HMAC_NONCE_LEN, 0);
> + n->hmac_nonce_expiry = current_time() + BABEL_HMAC_CHALLENGE_TIMEOUT;
> + n->hmac_next_challenge = current_time() + BABEL_HMAC_CHALLENGE_INTERVAL;
> +
> + msg.type = BABEL_TLV_CHALLENGE_REQ;
> + msg.challenge.nonce_len = BABEL_HMAC_NONCE_LEN;
> + msg.challenge.nonce = n->hmac_nonce;
> +
> + babel_send_unicast(&msg, ifa, n->addr);
> +}
> +
> +void
> +babel_hmac_reset_index(struct babel_iface *ifa)
> +{
> + getrandom(ifa->hmac_index, BABEL_HMAC_INDEX_LEN, 0);
> + ifa->hmac_pc = 1;
> +}
> +
> +int
> +babel_handle_hmac(union babel_msg *m, struct babel_iface *ifa)
> +{
It is funny that a function called babel_handle_hmac() does all
authentication checks except true HMAC check (which is done in
babel_hmac_check()).
> + struct babel_proto *p = ifa->proto;
> + struct babel_msg_hmac *msg = &m->hmac;
> +
> + TRACE(D_PACKETS, "Handling HMAC check on %s from %I", ifa->ifname, msg->sender);
> +
> + if (msg->index_len > BABEL_HMAC_INDEX_LEN || !msg->pc_seen)
> + return 0;
> +
> + struct babel_neighbor *n = babel_get_neighbor(ifa, msg->sender);
I think you are not supposed to do babel_get_neighbor()
> +
> + /* On successful challenge, update PC and index to current values */
> + if (msg->challenge_seen &&
> + n->hmac_nonce_expiry &&
> + n->hmac_nonce_expiry >= current_time() &&
> + !memcmp(msg->challenge, n->hmac_nonce, BABEL_HMAC_NONCE_LEN))
> + {
> + n->hmac_index_len = msg->index_len;
> + memcpy(n->hmac_index, msg->index, msg->index_len);
> + n->hmac_pc = msg->pc;
> + }
> +
> + /* If index differs, send challenge */
> + if (n->hmac_index_len != msg->index_len ||
> + memcmp(n->hmac_index, msg->index, msg->index_len))
> + {
> + babel_hmac_send_challenge(ifa, n);
> + return 0;
> + }
> +
> + /* Index matches; only accept if PC is greater than last */
> + if (n->hmac_pc >= msg->pc)
> + return 0;
> +
> + n->hmac_pc = msg->pc;
> + return 1;
> +}
> +
>
> /*
> * Babel interfaces
> @@ -1589,6 +1663,9 @@ babel_add_iface(struct babel_proto *p, struct iface *new, struct babel_iface_con
> init_list(&ifa->neigh_list);
> ifa->hello_seqno = 1;
>
> + if (ic->auth_type == BABEL_AUTH_HMAC)
> + babel_hmac_reset_index(ifa);
> +
> ifa->timer = tm_new_init(ifa->pool, babel_iface_timer, ifa, 0, 0);
>
> init_list(&ifa->msg_queue);
> @@ -1685,6 +1762,9 @@ babel_reconfigure_iface(struct babel_proto *p, struct babel_iface *ifa, struct b
> ifa->next_hop_ip4 = ipa_nonzero(new->next_hop_ip4) ? new->next_hop_ip4 : addr4;
> ifa->next_hop_ip6 = ipa_nonzero(new->next_hop_ip6) ? new->next_hop_ip6 : ifa->addr;
>
> + if (new->auth_type == BABEL_AUTH_HMAC && old->auth_type != BABEL_AUTH_HMAC)
> + babel_hmac_reset_index(ifa);
> +
> if (ipa_zero(ifa->next_hop_ip4) && p->ip4_channel)
> log(L_WARN "%s: Missing IPv4 next hop address for %s", p->p.name, ifa->ifname);
>
> diff --git a/proto/babel/babel.h b/proto/babel/babel.h
> index 14765c60..9ca4d339 100644
> --- a/proto/babel/babel.h
> +++ b/proto/babel/babel.h
> @@ -19,6 +19,7 @@
> #include "nest/route.h"
> #include "nest/protocol.h"
> #include "nest/locks.h"
> +#include "nest/password.h"
> #include "lib/resource.h"
> #include "lib/lists.h"
> #include "lib/socket.h"
> @@ -60,6 +61,13 @@
> #define BABEL_OVERHEAD (IP6_HEADER_LENGTH+UDP_HEADER_LENGTH)
> #define BABEL_MIN_MTU (512 + BABEL_OVERHEAD)
>
> +#define BABEL_AUTH_NONE 0
> +#define BABEL_AUTH_HMAC 1
> +#define BABEL_HMAC_NONCE_LEN 10 /* 80 bit random nonce */
> +#define BABEL_HMAC_INDEX_LEN 10 /* 80 bit indexes allowed */
> +#define BABEL_HMAC_CHALLENGE_TIMEOUT (30 S_)
> +#define BABEL_HMAC_CHALLENGE_INTERVAL (300 MS_)
> +
>
> enum babel_tlv_type {
> BABEL_TLV_PAD1 = 0,
> @@ -73,13 +81,10 @@ enum babel_tlv_type {
> BABEL_TLV_UPDATE = 8,
> BABEL_TLV_ROUTE_REQUEST = 9,
> BABEL_TLV_SEQNO_REQUEST = 10,
> - /* extensions - not implemented
> - BABEL_TLV_TS_PC = 11,
> - BABEL_TLV_HMAC = 12,
> - BABEL_TLV_SS_UPDATE = 13,
> - BABEL_TLV_SS_REQUEST = 14,
> - BABEL_TLV_SS_SEQNO_REQUEST = 15,
> - */
> + BABEL_TLV_CRYPTO_SEQNO = 121,
> + BABEL_TLV_HMAC = 122,
> + BABEL_TLV_CHALLENGE_REQ = 123,
> + BABEL_TLV_CHALLENGE_RESP = 124,
Perhaps keeping the names from the draft (PC, Reply)?
> BABEL_TLV_MAX
> };
>
> @@ -137,6 +142,11 @@ struct babel_iface_config {
>
> ip_addr next_hop_ip4;
> ip_addr next_hop_ip6;
> +
> + u8 auth_type; /* Authentication type (BABEL_AUTH_*) */
> + uint hmac_num_keys; /* Number of configured HMAC keys */
> + uint hmac_total_len; /* Total digest length for all configured keys */
> + list *passwords; /* Passwords for authentication */
> };
>
> struct babel_proto {
> @@ -184,6 +194,9 @@ struct babel_iface {
>
> u16 hello_seqno; /* To be increased on each hello */
>
> + u32 hmac_pc;
> + u8 hmac_index[BABEL_HMAC_INDEX_LEN];
> +
> btime next_hello;
> btime next_regular;
> btime next_triggered;
> @@ -207,6 +220,14 @@ struct babel_neighbor {
> u16 hello_map;
> u16 next_hello_seqno;
> uint last_hello_int;
> +
> + u32 hmac_pc;
> + u8 hmac_index_len;
> + u8 hmac_index[BABEL_HMAC_INDEX_LEN];
> + u8 hmac_nonce[BABEL_HMAC_NONCE_LEN];
> + btime hmac_nonce_expiry;
> + btime hmac_next_challenge;
> +
> /* expiry timers */
> btime hello_expiry;
> btime ihu_expiry;
> @@ -339,6 +360,23 @@ struct babel_msg_seqno_request {
> ip_addr sender;
> };
>
> +struct babel_msg_challenge {
> + u8 type;
> + u8 nonce_len;
> + u8 *nonce;
> +};
> +
> +struct babel_msg_hmac {
> + u8 type;
> + ip_addr sender;
> + u32 pc;
> + u8 pc_seen;
> + u8 index_len;
> + u8 *index;
> + u8 challenge_seen;
> + u8 challenge[BABEL_HMAC_NONCE_LEN];
> +};
> +
> union babel_msg {
> u8 type;
> struct babel_msg_ack_req ack_req;
> @@ -348,6 +386,8 @@ union babel_msg {
> struct babel_msg_update update;
> struct babel_msg_route_request route_request;
> struct babel_msg_seqno_request seqno_request;
> + struct babel_msg_challenge challenge;
> + struct babel_msg_hmac hmac;
> };
>
> struct babel_msg_node {
> @@ -367,6 +407,10 @@ void babel_handle_router_id(union babel_msg *msg, struct babel_iface *ifa);
> void babel_handle_update(union babel_msg *msg, struct babel_iface *ifa);
> void babel_handle_route_request(union babel_msg *msg, struct babel_iface *ifa);
> void babel_handle_seqno_request(union babel_msg *msg, struct babel_iface *ifa);
> +void babel_handle_hmac_challenge_req(union babel_msg *m, struct babel_iface *ifa);
This function is missing?
> +
> +int babel_handle_hmac(union babel_msg *m, struct babel_iface *ifa);
> +void babel_hmac_reset_index(struct babel_iface *ifa);
>
> void babel_show_interfaces(struct proto *P, char *iff);
> void babel_show_neighbors(struct proto *P, char *iff);
> diff --git a/proto/babel/config.Y b/proto/babel/config.Y
> index 3af79fd6..36608bf4 100644
> --- a/proto/babel/config.Y
> +++ b/proto/babel/config.Y
> @@ -25,7 +25,7 @@ CF_DECLS
> CF_KEYWORDS(BABEL, INTERFACE, METRIC, RXCOST, HELLO, UPDATE, INTERVAL, PORT,
> TYPE, WIRED, WIRELESS, RX, TX, BUFFER, PRIORITY, LENGTH, CHECK, LINK,
> NEXT, HOP, IPV4, IPV6, BABEL_METRIC, SHOW, INTERFACES, NEIGHBORS,
> - ENTRIES, RANDOMIZE, ROUTER, ID)
> + ENTRIES, RANDOMIZE, ROUTER, ID, AUTHENTICATION, NONE, HMAC)
>
> CF_GRAMMAR
>
> @@ -91,6 +91,35 @@ babel_iface_finish:
> BABEL_IFACE->ihu_interval = MIN_(BABEL_IFACE->hello_interval*BABEL_IHU_INTERVAL_FACTOR, BABEL_MAX_INTERVAL);
>
> BABEL_CFG->hold_time = MAX_(BABEL_CFG->hold_time, BABEL_IFACE->update_interval*BABEL_HOLD_TIME_FACTOR);
> +
> + BABEL_IFACE->passwords = get_passwords();
You also need to call reset_passwords() in babel_iface_start, see e.g.
rip_iface_start.
> +
> + if (!BABEL_IFACE->auth_type != !BABEL_IFACE->passwords)
> + log(L_WARN "Authentication and password options should be used together");
> +
> + if (BABEL_IFACE->passwords)
> + {
> + struct password_item *pass;
> + uint len = 0, i = 0;
> + WALK_LIST(pass, *BABEL_IFACE->passwords)
> + {
> + /* Set default crypto algorithm (HMAC-SHA256) */
> + if (!pass->alg && (BABEL_IFACE->auth_type == BABEL_AUTH_HMAC))
> + pass->alg = ALG_HMAC_SHA256;
The auth_type condition is unnecessary.
> +
> + if (!(pass->alg & ALG_HMAC))
> + cf_error("Only HMAC algorithms are supported");
> +
> + if (BABEL_IFACE->auth_type != BABEL_AUTH_HMAC)
> + cf_error("Password setting requires HMAC authentication");
This check is effectively the same check as the warning ~20 lines above.
> + len += mac_type_length(pass->alg);
> + i++;
> + }
> + BABEL_IFACE->hmac_num_keys = i;
> + BABEL_IFACE->hmac_total_len = len;
> + }
> +
> };
>
>
> @@ -109,6 +138,9 @@ babel_iface_item:
> | CHECK LINK bool { BABEL_IFACE->check_link = $3; }
> | NEXT HOP IPV4 ipa { BABEL_IFACE->next_hop_ip4 = $4; if (!ipa_is_ip4($4)) cf_error("Must be an IPv4 address"); }
> | NEXT HOP IPV6 ipa { BABEL_IFACE->next_hop_ip6 = $4; if (!ipa_is_ip6($4)) cf_error("Must be an IPv6 address"); }
> + | AUTHENTICATION NONE { BABEL_IFACE->auth_type = BABEL_AUTH_NONE; }
> + | AUTHENTICATION HMAC { BABEL_IFACE->auth_type = BABEL_AUTH_HMAC; }
> + | password_list { }
> ;
>
> babel_iface_opts:
> diff --git a/proto/babel/packets.c b/proto/babel/packets.c
> index 9c767874..a587be5c 100644
> --- a/proto/babel/packets.c
> +++ b/proto/babel/packets.c
> @@ -11,6 +11,7 @@
> */
>
> #include "babel.h"
> +#include "lib/mac.h"
>
>
> struct babel_pkt_header {
> @@ -105,6 +106,26 @@ struct babel_tlv_seqno_request {
> u8 addr[0];
> } PACKED;
>
> +struct babel_tlv_crypto_seqno {
> + u8 type;
> + u8 length;
> + u32 pc;
> + u8 index[0];
> +} PACKED;
> +
> +struct babel_tlv_hmac {
> + u8 type;
> + u8 length;
> + u8 hmac[0];
> +} PACKED;
> +
> +struct babel_tlv_challenge {
> + u8 type;
> + u8 length;
> + u8 nonce[0];
> +} PACKED;
> +
> +
> struct babel_subtlv_source_prefix {
> u8 type;
> u8 length;
> @@ -135,6 +156,16 @@ struct babel_parse_state {
> u8 def_ip4_prefix_seen; /* def_ip4_prefix is valid */
> u8 current_tlv_endpos; /* End of self-terminating TLVs (offset from start) */
> u8 sadr_enabled;
> + u8 is_unicast;
> + /* HMAC data */
> + u32 hmac_pc;
> + u8 hmac_pc_seen;
> + u8 hmac_index_len;
> + u8 *hmac_index;
> + u8 hmac_challenge_resp_seen;
> + u8 hmac_challenge_resp[BABEL_HMAC_NONCE_LEN];
> + u8 hmac_challenge_len;
> + u8 *hmac_challenge;
> };
>
> enum parse_result {
> @@ -152,6 +183,13 @@ struct babel_write_state {
> u8 def_ip6_pxlen;
> };
>
> +struct babel_hmac_pseudohdr {
> + u8 src_addr[16];
> + u16 src_port;
> + u8 dst_addr[16];
> + u16 dst_port;
> +} PACKED;
> +
>
> #define DROP(DSC,VAL) do { err_dsc = DSC; err_val = VAL; goto drop; } while(0)
> #define DROP1(DSC) do { err_dsc = DSC; goto drop; } while(0)
> @@ -270,6 +308,9 @@ static int babel_read_update(struct babel_tlv *hdr, union babel_msg *msg, struct
> static int babel_read_route_request(struct babel_tlv *hdr, union babel_msg *msg, struct babel_parse_state *state);
> static int babel_read_seqno_request(struct babel_tlv *hdr, union babel_msg *msg, struct babel_parse_state *state);
> static int babel_read_source_prefix(struct babel_tlv *hdr, union babel_msg *msg, struct babel_parse_state *state);
> +static int babel_read_crypto_seqno(struct babel_tlv *hdr, union babel_msg *msg, struct babel_parse_state *state);
> +static int babel_read_challenge_req(struct babel_tlv *hdr, union babel_msg *msg, struct babel_parse_state *state);
> +static int babel_read_challenge_resp(struct babel_tlv *hdr, union babel_msg *msg, struct babel_parse_state *state);
>
> static uint babel_write_ack(struct babel_tlv *hdr, union babel_msg *msg, struct babel_write_state *state, uint max_len);
> static uint babel_write_hello(struct babel_tlv *hdr, union babel_msg *msg, struct babel_write_state *state, uint max_len);
> @@ -277,6 +318,7 @@ static uint babel_write_ihu(struct babel_tlv *hdr, union babel_msg *msg, struct
> static uint babel_write_update(struct babel_tlv *hdr, union babel_msg *msg, struct babel_write_state *state, uint max_len);
> static uint babel_write_route_request(struct babel_tlv *hdr, union babel_msg *msg, struct babel_write_state *state, uint max_len);
> static uint babel_write_seqno_request(struct babel_tlv *hdr, union babel_msg *msg, struct babel_write_state *state, uint max_len);
> +static uint babel_write_challenge(struct babel_tlv *hdr, union babel_msg *m, struct babel_write_state *state UNUSED, uint max_len);
> static int babel_write_source_prefix(struct babel_tlv *hdr, net_addr *net, uint max_len);
>
> struct babel_tlv_data {
> @@ -341,6 +383,24 @@ static const struct babel_tlv_data tlv_data[BABEL_TLV_MAX] = {
> babel_write_seqno_request,
> babel_handle_seqno_request
> },
> + [BABEL_TLV_CRYPTO_SEQNO] = {
> + sizeof(struct babel_tlv_crypto_seqno),
> + babel_read_crypto_seqno,
> + NULL,
> + NULL,
> + },
> + [BABEL_TLV_CHALLENGE_REQ] = {
> + sizeof(struct babel_tlv_challenge),
> + babel_read_challenge_req,
> + babel_write_challenge,
> + NULL,
> + },
> + [BABEL_TLV_CHALLENGE_RESP] = {
> + sizeof(struct babel_tlv_challenge),
> + babel_read_challenge_resp,
> + babel_write_challenge,
> + NULL,
> + },
> };
>
> static int
> @@ -1107,6 +1167,73 @@ babel_write_source_prefix(struct babel_tlv *hdr, net_addr *n, uint max_len)
> return len;
> }
>
> +static int
> +babel_read_crypto_seqno(struct babel_tlv *hdr, union babel_msg *m UNUSED,
> + struct babel_parse_state *state)
> +{
> + struct babel_tlv_crypto_seqno *tlv = (void *) hdr;
> +
> + if (!state->hmac_pc_seen) {
> + state->hmac_pc_seen = 1;
> + state->hmac_pc = get_u32(&tlv->pc);
> + state->hmac_index_len = tlv->length - sizeof(state->hmac_pc);
That sizeof does not make sense (as it is completely different
structure). And we already have TLV_OPT_LENGTH().
> + state->hmac_index = tlv->index;
We should use memcpy() here?
> + }
> +
> + return PARSE_IGNORE;
> +}
> +
> +static int
> +babel_read_challenge_req(struct babel_tlv *hdr, union babel_msg *m UNUSED,
> + struct babel_parse_state *state)
> +{
> + struct babel_tlv_challenge *tlv = (void *) hdr;
> +
> + if (!state->is_unicast) {
> + DBG("Ignoring non-unicast challenge request from %I", state->saddr);
> + return PARSE_IGNORE;
> + }
> +
> + if (!state->hmac_challenge_len) {
> + state->hmac_challenge_len = tlv->length;
> + state->hmac_challenge = tlv->nonce;
> + }
I would prefer if the exceptional cases would be in conditionals and
regular progression is not.
> + return PARSE_IGNORE;
> +}
> +
> +static int
> +babel_read_challenge_resp(struct babel_tlv *hdr, union babel_msg *m UNUSED,
> + struct babel_parse_state *state)
> +{
> + struct babel_tlv_challenge *tlv = (void *) hdr;
> +
> + if (tlv->length == BABEL_HMAC_NONCE_LEN && !state->hmac_challenge_resp_seen)
> + {
> + state->hmac_challenge_resp_seen = 1;
> + memcpy(state->hmac_challenge_resp, tlv->nonce, BABEL_HMAC_NONCE_LEN);
> + }
ditto
> + return PARSE_IGNORE;
> +}
> +
> +static uint
> +babel_write_challenge(struct babel_tlv *hdr, union babel_msg *m,
> + struct babel_write_state *state UNUSED, uint max_len)
> +{
> + struct babel_tlv_challenge *tlv = (void *) hdr;
> + struct babel_msg_challenge *msg = &m->challenge;
> +
> + uint len = sizeof(struct babel_tlv_challenge) + msg->nonce_len;
> +
> + if (len > max_len)
> + return 0;
> +
> + TLV_HDR(tlv, msg->type, len);
> + memcpy(tlv->nonce, msg->nonce, msg->nonce_len);
> +
> + return len;
> +}
>
> static inline int
> babel_read_subtlvs(struct babel_tlv *hdr,
> @@ -1189,6 +1316,210 @@ babel_write_tlv(struct babel_tlv *hdr,
> }
>
>
> +static int
> +babel_hmac_hash(struct password_item *pass,
> + struct babel_pkt_header *pkt,
> + ip_addr saddr, u16 sport,
> + ip_addr daddr, u16 dport,
> + byte *buf, uint *buf_len)
> +{
> + struct mac_context ctx;
> + struct babel_hmac_pseudohdr phdr = {};
> + int pkt_len = get_u16(&pkt->length) + sizeof(struct babel_pkt_header);
> +
> + put_ip6(phdr.src_addr, saddr);
> + put_u16(&phdr.src_port, sport);
> + put_ip6(phdr.dst_addr, daddr);
> + put_u16(&phdr.dst_port, dport);
> + DBG("HMAC pseudo-header: %I %d %I %d\n", saddr, sport, daddr, dport);
> +
> + if (mac_type_length(pass->alg) > *buf_len)
> + return 1;
> +
> + mac_init(&ctx, pass->alg, pass->password, pass->length);
> + mac_update(&ctx, (byte *)&phdr, sizeof(phdr));
> + mac_update(&ctx, (byte *)pkt, pkt_len);
> +
> + *buf_len = mac_get_length(&ctx);
> + memcpy(buf, mac_final(&ctx), *buf_len);
> +
> + mac_cleanup(&ctx);
> +
> + return 0;
> +}
> +
> +static int
> +babel_hmac_check(struct babel_iface *ifa,
> + struct babel_pkt_header *pkt,
> + ip_addr saddr, u16 sport,
> + ip_addr daddr, u16 dport,
> + byte *start, uint len)
> +{
> + struct babel_proto *p = ifa->proto;
> + struct password_item *pass;
> + struct babel_tlv *tlv;
> + struct babel_tlv_hmac *hmac;
> + btime now_ = current_real_time();
> + byte *end = start + len;
> +
> + TRACE(D_PACKETS, "Checking packet HMAC signature");
> +
> + if (len < sizeof(*tlv))
> + return 1;
> +
> + WALK_LIST(pass, *ifa->cf->passwords)
> + {
> + byte mac_res[MAX_HASH_SIZE];
> + uint mac_len = MAX_HASH_SIZE;
> + int res = 0;
> +
> + if (pass->accfrom > now_ || pass->accto < now_)
> + continue;
> +
> + if (babel_hmac_hash(pass, pkt,
> + saddr, sport,
> + daddr, dport,
> + mac_res, &mac_len))
> + continue;
Babel does not use something like security association ID to match keys,
like in OSPF or RIP? That is unseemly.
> +
> + LOOP_TLVS(start, end, tlv)
> + {
> + if (tlv->type != BABEL_TLV_HMAC)
> + continue;
> +
> + hmac = (void *)tlv;
> +
> + res = (tlv->length == mac_len && !memcmp(hmac->hmac, mac_res, mac_len));
> + if (res)
> + break;
> +
> + DBG("HMAC mismatch key id %d pos %d len %d/%d\n",
> + pass->id, (byte *)tlv - (byte *)pkt, mac_len, tlv->length);
> + }
> + LOOP_TLVS_END;
> +
> + if (res) {
> + TRACE(D_PACKETS, "HMAC signature OK");
> + return 0;
No need to log OK.
> + }
> + }
> +
> + TRACE(D_PACKETS, "No matching HMAC key found");
> + return 1;
Authentication error log messages (and perhaps received packet messages in
general) should be rate-limited. See LOG_PKT() and LOG_PKT_AUTH() in RIP.
> +frame_err:
> + DBG("HMAC trailer TLV framing error\n");
> + return 1;
> +}
> +
> +static int
> +babel_hmac_check_state(struct babel_iface *ifa, struct babel_parse_state *state)
> +{
> + struct babel_proto *p = ifa->proto;
> + union babel_msg msg = {};
> + int ret;
> +
> + if (state->hmac_challenge_len)
> + {
> + union babel_msg resp = {};
> + TRACE(D_PACKETS, "Sending HMAC challenge response to %I", state->saddr);
> + resp.type = BABEL_TLV_CHALLENGE_RESP;
> + resp.challenge.nonce_len = state->hmac_challenge_len;
> + resp.challenge.nonce = state->hmac_challenge;
> + babel_send_unicast(&resp, ifa, state->saddr);
> + }
> +
> + msg.type = BABEL_TLV_HMAC;
> + msg.hmac.sender = state->saddr;
> + msg.hmac.pc = state->hmac_pc;
> + msg.hmac.pc_seen = state->hmac_pc_seen;
> + msg.hmac.index_len = state->hmac_index_len;
> + msg.hmac.index = state->hmac_index;
> + msg.hmac.challenge_seen = state->hmac_challenge_resp_seen;
> + memcpy(msg.hmac.challenge,
> + state->hmac_challenge_resp,
> + BABEL_HMAC_NONCE_LEN);
> +
> + ret = babel_handle_hmac(&msg, ifa);
We are synthetizing virtual TLV here just to call babel_handle_hmac() and
split the code to two functions?
> +
> + return ret;
> +}
> +
> +static int
> +babel_hmac_add_pc(struct babel_iface *ifa, struct babel_tlv *tlv, int max_len)
> +{
> + struct babel_proto *p = ifa->proto;
> +
> + struct babel_tlv_crypto_seqno *msg = (void *)tlv;
> + int len = sizeof(*msg) + BABEL_HMAC_INDEX_LEN;
> +
> + if (len > max_len) {
> + LOG_PKT("Warning: Insufficient space to add HMAC seqno TLV on iface %s: %d < %d",
> + ifa->ifname, max_len, len);
> + return 0;
> + }
Should log() with L_WARN instead of "Warning:" in text.
> +
> + msg->type = BABEL_TLV_CRYPTO_SEQNO;
> + msg->length = len - sizeof(struct babel_tlv);
> + put_u32(&msg->pc, ifa->hmac_pc++);
> + memcpy(msg->index, ifa->hmac_index, BABEL_HMAC_INDEX_LEN);
> +
> + /* Reset index on overflow to 0 */
> + if (!ifa->hmac_pc)
> + babel_hmac_reset_index(ifa);
> +
> + return len;
> +}
> +
> +static int
> +babel_hmac_sign(struct babel_iface *ifa, ip_addr dest)
> +{
> + struct babel_proto *p = ifa->proto;
> + sock *sk = ifa->sk;
> +
> + struct babel_pkt_header *hdr = (void *) sk->tbuf;
> + int len = get_u16(&hdr->length) + sizeof(struct babel_pkt_header);
> +
> + byte *pos = (byte *)hdr + len;
> + byte *end = (byte *)hdr + ifa->tx_length;
> + struct babel_tlv *tlv = (void *)pos;
> + struct password_item *pass;
> + btime now_ = current_real_time();
> + int tot_len = 0, i = 0;
> +
> + WALK_LIST(pass, *ifa->cf->passwords)
> + {
> + struct babel_tlv_hmac *msg = (void *)tlv;
> + uint buf_len = (uint) (end - (byte *)msg - sizeof(*msg));
> +
> + if (pass->genfrom > now_ || pass->gento < now_)
> + continue;
> +
> + if (babel_hmac_hash(pass, hdr,
> + sk->saddr, sk->fport,
> + dest, sk->dport,
> + msg->hmac, &buf_len))
> + {
> + LOG_PKT("Warning: Insufficient space for HMAC signatures on iface %s dest %I",
> + ifa->ifname, dest);
Should log() with L_WARN instead of "Warning:" in text.
> + break;
> + }
> +
> + msg->type = BABEL_TLV_HMAC;
> + msg->length = buf_len;
> +
> + tlv = NEXT_TLV(tlv);
> + tot_len += buf_len + sizeof(*msg);
> + i++;
> + }
> +
> + TRACE(D_PACKETS, "Added %d HMAC signatures (%d bytes) on ifa %s for dest %I",
> + i, tot_len, ifa->ifname, dest);
Probably no need to log this.
> +
> + return tot_len;
> +}
> +
> +
> /*
> * Packet RX/TX functions
> */
> @@ -1200,6 +1531,9 @@ babel_send_to(struct babel_iface *ifa, ip_addr dest)
> struct babel_pkt_header *hdr = (void *) sk->tbuf;
> int len = get_u16(&hdr->length) + sizeof(struct babel_pkt_header);
>
> + if (ifa->cf->auth_type == BABEL_AUTH_HMAC)
> + len += babel_hmac_sign(ifa, dest);
> +
> DBG("Babel: Sending %d bytes to %I\n", len, dest);
> return sk_send_to(sk, len, dest, 0);
> }
> @@ -1223,12 +1557,19 @@ babel_write_queue(struct babel_iface *ifa, list *queue)
> {
> struct babel_proto *p = ifa->proto;
> struct babel_write_state state = { .next_hop_ip6 = ifa->addr };
> + int tx_length = ifa->tx_length;
>
> if (EMPTY_LIST(*queue))
> return 0;
>
> + if (ifa->cf->auth_type == BABEL_AUTH_HMAC)
> + tx_length -= (sizeof(struct babel_tlv_crypto_seqno) +
> + BABEL_HMAC_INDEX_LEN +
> + ifa->cf->hmac_total_len +
> + sizeof(struct babel_tlv_hmac) * ifa->cf->hmac_num_keys);
I think this should be accounted in ifa->tx_length. Compare
babel_iface_update_buffers() vs. rip_iface_update_buffers().
> +
> byte *pos = ifa->sk->tbuf;
> - byte *end = pos + ifa->tx_length;
> + byte *end = pos + tx_length;
>
> struct babel_pkt_header *pkt = (void *) pos;
> pkt->magic = BABEL_MAGIC;
> @@ -1252,6 +1593,9 @@ babel_write_queue(struct babel_iface *ifa, list *queue)
> sl_free(p->msg_slab, msg);
> }
>
> + if (ifa->cf->auth_type == BABEL_AUTH_HMAC)
> + pos += babel_hmac_add_pc(ifa, (struct babel_tlv *) pos, end-pos);
> +
> uint plen = pos - (byte *) pkt;
> put_u16(&pkt->length, plen - sizeof(struct babel_pkt_header));
>
> @@ -1329,10 +1673,13 @@ babel_enqueue(union babel_msg *msg, struct babel_iface *ifa)
>
> /**
> * babel_process_packet - process incoming data packet
> + * @ifa: Interface packet was received on.
> * @pkt: Pointer to the packet data
> * @len: Length of received packet
> * @saddr: Address of packet sender
> - * @ifa: Interface packet was received on.
> + * @sport: Packet source port
> + * @daddr: Destination address of packet
> + * @dport: Packet destination port
> *
> * This function is the main processing hook of incoming Babel packets. It
> * checks that the packet header is well-formed, then processes the TLVs
> @@ -1344,14 +1691,17 @@ babel_enqueue(union babel_msg *msg, struct babel_iface *ifa)
> * order.
> */
> static void
> -babel_process_packet(struct babel_pkt_header *pkt, int len,
> - ip_addr saddr, struct babel_iface *ifa)
> +babel_process_packet(struct babel_iface *ifa,
> + struct babel_pkt_header *pkt, int len,
> + ip_addr saddr, u16 sport,
> + ip_addr daddr, u16 dport)
> {
> struct babel_proto *p = ifa->proto;
> struct babel_tlv *tlv;
> struct babel_msg_node *msg;
> list msgs;
> int res;
> + int hmac_ok = 1;
>
> int plen = sizeof(struct babel_pkt_header) + get_u16(&pkt->length);
> byte *end = (byte *)pkt + plen;
> @@ -1362,6 +1712,7 @@ babel_process_packet(struct babel_pkt_header *pkt, int len,
> .saddr = saddr,
> .next_hop_ip6 = saddr,
> .sadr_enabled = babel_sadr_enabled(p),
> + .is_unicast = !(ipa_classify(daddr) & IADDR_MULTICAST),
> };
>
> if ((pkt->magic != BABEL_MAGIC) || (pkt->version != BABEL_VERSION))
> @@ -1381,6 +1732,11 @@ babel_process_packet(struct babel_pkt_header *pkt, int len,
> TRACE(D_PACKETS, "Packet received from %I via %s",
> saddr, ifa->iface->name);
>
> + /* First HMAC check - verify signature */
> + if (ifa->cf->auth_type == BABEL_AUTH_HMAC &&
> + babel_hmac_check(ifa, pkt, saddr, sport, daddr, dport, end, len-plen))
> + return;
> +
> init_list(&msgs);
>
> /* First pass through the packet TLV by TLV, parsing each into internal data
> @@ -1410,10 +1766,14 @@ babel_process_packet(struct babel_pkt_header *pkt, int len,
>
> frame_err:
>
> + /* Second HMAC check - verify sequence counter state */
> + if (ifa->cf->auth_type == BABEL_AUTH_HMAC)
> + hmac_ok = babel_hmac_check_state(ifa, &state);
> +
IMHO the draft describes this order:
- parse trailer
- preparse for PC and index
- verify authentication (HMAC and PC/index)
- regular parse and processing
You are doing
- parse trailer
- verify HMAC
- regular parse
- verify PC/index
- regular processing
I would prefer if we just keep whole packet verification as a separate
phase like described in the draft and as a separate function called from
babel_process_packet(), even if that means two iterations through TLVs.
> /* Parsing done, handle all parsed TLVs */
> WALK_LIST_FIRST(msg, msgs)
> {
> - if (tlv_data[msg->msg.type].handle_tlv)
> + if (hmac_ok && tlv_data[msg->msg.type].handle_tlv)
> tlv_data[msg->msg.type].handle_tlv(&msg->msg, ifa);
> rem_node(NODE msg);
> sl_free(p->msg_slab, msg);
> @@ -1473,7 +1833,10 @@ babel_rx_hook(sock *sk, uint len)
> if (sk->flags & SKF_TRUNCATED)
> DROP("truncated", len);
>
> - babel_process_packet((struct babel_pkt_header *) sk->rbuf, len, sk->faddr, ifa);
> + babel_process_packet(ifa,
> + (struct babel_pkt_header *) sk->rbuf, len,
> + sk->faddr, sk->fport,
> + sk->laddr, sk->dport);
> return 1;
>
> drop:
--
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."
More information about the Bird-users
mailing list