Re: [babel] Some open HMAC issues

[Toke Høiland-Jørgensen <[e-mail]>]

Seeing as I have now implemented the draft, I have some more opinions on
this now:

> 2. Should the HMAC cover the packet header?
>
> The Babel packet header has the following format:
>
>      0                   1                   2                   3
>      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |     Magic     |    Version    |        Body length            |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
> Right now, Version is fixed at 2, but if we ever define Babel version 3,
> we might become susceptible to downgrade attacks.  I don't recall why we
> skipped the header in HMAC computation, but I think it's a bug.
>
> (As far as I can tell, 7298bis hashes the whole packet, including the
> packet trailer.)

Yes, I see now reason not to hash the whole thing.

> 5. Use of the packet trailer
>
> The HMAC TLV is carried in the packet trailer, which makes it clear which
> part of the packet is protected by HMAC and avoids the need to clear parts
> of the packet body before hashing.  (Think about extensibility -- there
> might be other TLVs in the future that must not be protected, and if
> different extensions require clearing different parts of the packet, we're
> going to end up with some pretty enjoyable code obfuscation.)
>
> Does anyone have technical arguments against the use of the packet
> trailer?  For the record, Denis didn't use a packet trailer, and David has
> expressed some mild reservations about its use (he feels that we're
> trading specification simplicity for implementation simplicity and
> extensibility, and while I agree with this, I happen to think it's the
> right tradeoff in this particular case).
>
> If we keep the trailer, well need to add something to 6126bis.  I suggest
> the following RFCese:
>
>   - the packet trailer is a sequence of TLVs, just like the packet body,
>     and the TLV number space is the same;
>   - a TLV that appears in the packet trailer MUST be ignored on reception
>     unless its definition explicitly states that it is allowed in the
>     packet trailer;
>   - the mandatory bit MUST NOT be acted upon when it appears in the packet
>     trailer; as a consequence, an implementation that doesn't grok any
>     TLVs that can appear in the trailer MAY simply ignore the packet
>     trailer without ever parsing it.

I agree that sticking the signatures in the packet trailer is the right
thing to do. The zero out / hash self / rewrite hash dance is way too
annoying. Also, when the signature is in the packet trailer the hash
check can move straight to that and ignore the rest of the packet; and
the regular parser can ignore the packet trailer and just parse the
regular TLVs.

> 8. Index and Nonce size
>
> Indices and Nonces are of variable size (up to the maximum TLV size): our
> implementation selects 80-bit values, but it will handle values of up to
> 251 resp. 255 octets (as much as fits in a TLV).
>
> While having variable-length values slightly complicates the implementation,
> I think it's a good idea:
>
>   - an implementation might want to encode a cookie in the Nonce in order
>     to have stronger DoS protection, thus needing a larger Nonce;
>   - an implementation with a stable clock might want to use a tiny Index
>     (see point 5 above).
>
> I suggest we should limit indices and nonces to 192 octets, indices and
> nonces larger than that MUST NOT be sent.  This gives enough margin to
> carry a nonce or index in a sub-TLV (which we're not currently planning to
> do, but what the heck).

Letting the nonce be of arbitrary size is fine, but doing the same thing
for the index is incredibly annoying. Since the index needs to be stored
with the same lifetime as the neighbour, making it arbitrary size means
there's another blob of dynamically assigned memory to keep track of.
Which may have to be resized later if the peer changes its index size.

If we want to allow really small indexes, that is fine, but then limit
the max size to (say) 10 bytes; that way, a buffer of the maximum size
can be statically assigned in the neighbour struct (251 or even 192
bytes is way too much for static allocation).

Or maybe just forgo the complexity entirely and make the index a fixed
size; you just know that someone is going to pick a way too small value
and make themselves susceptible to replay attacks at some point...

-Toke