Subject: [babel] Shepherd's review of draft-ietf-babel-hmac-02

From: Donald Eastlake <[email-masked]>
Date: Tue, 25 Dec 2018 23:32:03 -0500

Section 1.2, Bottom of page 3.
"robust random number generator" -> "robust random number [RFC4086]
generator". Could also add the RFC 4086 reference at other occurrences
of "random" but reference should be included at least once.

Section 1.2, Top of page 4, third bullet item.
This is a little confusing to read. I assume that the point about the
packet being accepted by node A is that it's a valid packet (has a
good HMAC TLV). But I think the average reading will be wondering
"What is this node A and why isn't it mentioned again later?" Not sure
if there is a reasonable wording change that improves this...

Section 5.2:
The diagram is odd. I would recommend sliding the Index field and
preceding vertical bar character to the left so it starts aligned
with the Type. Also, the PC is fixed length, which is not shown in the
figure - I think you really have to at least mention the length of the
PC in this section. Perhaps best to change the figure to something
like the following:
    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
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |     Type      |    Length     |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                             PC                                |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |            Index...
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
or
   +-----------+
   |    Type   |         1 byte
   +-----------+
   |   Length  |         1 byte
   +-----------+-----------+-----------+-----------+
   |                      PC                       |  4 bytes
   +-----------+-----------+-----------+-----------+
   |            Index...                              variable
   +-----------+-----------+-----------+-----...

While it is reasonable, for implementation convenience, that there is
a maximum size for Index, perhaps for cryptographic strength, there
should be a minimum length, maybe 12 bytes?

Thinking some more about PC, seems like you either (1) need to say
that circular arithmetic is used [rfc1982] (bad idea in my opinion),
or (2) say what happens when PC hits all 1's (presumably adopt a new
Index, probably a better idea). It might be useful to also state that
PC is an "unsigned integer" or the like.

Section 7.
Should say IANA is "requested" rather than "instructed". (Or, after
the number are allocated, should be re-worded to indicated that the
IANA actions have been completed.)

Appendix B.2.
Heading should end with "-hmac-01", not "-hman-00".

Thanks,
Donald
===============================
 Donald E. Eastlake 3rd   +1-508-333-2270 (cell)
 1424 Pro Shop Court, Davenport, FL 33896 USA
 [email-masked]

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 26 Dec 2018 20:03:12 +0100

Thanks for your review, Donald.

> of "random" but reference should be included at least once.

Done, thanks.

> if there is a reasonable wording change that improves this...

I too am unhappy with this property.  We've spent a lot of time thinking
about how to reformulate it in a more comprehensible manner, but we've
failed.  If anyone has any idea, I'm listening.

> with the Type.

I've tried, but I didn't like the result.  I'll leave it as it is, if
that's okay with you.

> Also, the PC is fixed length, which is not shown in the figure - I think
> you really have to at least mention the length of the PC in this
> section.

Done.

> should be a minimum length, maybe 12 bytes?

I disagree.  The only property that the index needs to satisfy is that it
is never reused for a different PC sequence for a given (key, sender)
pair.  In particular, if a node never loses state (e.g. it has reliable
persistent storage), then it never needs to change indices -- and in that
case, a 0-octet index is suitable.

> PC is an "unsigned integer" or the like.

In Section 4.2, we say:

      if the PC overflows, a new index is generated;

In Section 5.2, we say:

      A new index MUST be generated whenever the PC overflows.

Please let me know if you think that's not explicit enough.

> Should say IANA is "requested" rather than "instructed".

Done.

> Heading should end with "-hmac-01", not "-hman-00".

Done.

Thanks again,

-- Juliusz

---

From: Dave Taht <[email-masked]>
Date: Wed, 26 Dec 2018 12:36:48 -0800

> failed.  If anyone has any idea, I'm listening.

I'll think upon it too.

> case, a 0-octet index is suitable.

Well, say that in the draft. It's come up as a gut objection in
everybody thus far.

>       if the PC overflows, a new index is generated;

When. You used an example of several hundred years, but I think that
was dependent on the current default 4 sec hello interval?

I'm still working out how to go about attacking things. One way is to
force the daemon to crash (malformed packet, a ping of death, some
other vulnerability in another daemon that can send a kill -9 to this
process), on restart, which resets the pc, and forces choosing a new
index. With a zero length index, well...

> _______________________________________________
> babel mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/babel

-- 

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-205-9740

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 26 Dec 2018 23:17:14 +0100

> Well, say that in the draft. It's come up as a gut objection in

From Section 6:

   Second, it assumes that indices and nonces are generated uniquely
   over the lifetime of a key used for HMAC computation (more precisely,
   indices must be unique for a given (key, source) pair, and nonces
   must be unique for a given (key, source, destination) triple).  This
   property can be satisfied either by using a cryptographically secure
   random number generator to generate indices and nonces that contain
   enough entropy (64-bit values are believed to be large enough for all
   practical applications), or by using a reliably monotonic hardware
   clock.  If unicity cannot be guaranteed (e.g., because a hardware
   clock has been reset), then rekeying is necessary.

> index. With a zero length index, well...

You detect that you've suffered a crash, and you refuse to run until you
detect a key rotation.  Or you detect that you've suffered a crash, and
switch to 64-bit indices until the next key rotation.

I'm not saying that that's the right way to implement this protocol (it's
too complex and fragile for my taste), but if someone is intent on saving
every extra bit on the wire, the protocol allows that.

-- Juliusz

---

From: Donald Eastlake <[email-masked]>
Date: Thu, 27 Dec 2018 00:23:32 -0500

Hi Juliusz,

Deleting stuff where we don't disagree:

On Wed, Dec 26, 2018 at 2:03 PM Juliusz Chroboczek <[email-masked]> wrote:
> ...

I'm pretty sure the draft with the current figure is going to run into
problems at AD review and if it survives that, I can pretty much guarantee
the draft will run in problems with at least one AD when it hits the IESG.
The current figure tries to be in the style used for protocol headers but
that isn't the style normally used in the IETF for TLVs. The alternative
figures I included in my comments are more in the traditional figure style
for a Routing area TLV. See, for example, the numerous TLV and sub-TLV
diagrams in https://tools.ietf.org/html/rfc7176 which are close to my
second suggestion (except that I squeezed the lines a bit, dropping the
alternative "+" signs so that four bytes would fit more easily on a line).
Or https://tools.ietf.org/html/rfc8401 which has diagrams closer to my
first example.

If you really want to use a protocol header style figure for this TLV, I
think you will end up needing to change it to something like the following:
    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
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |     Type      |    Length     |         PC - upper half       |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |         PC - lower half       |            Index...
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-

> ...

The assumption that a node will never lose state and never count out the PC
space seems quite brittle to me.

> ...

Thanks,
Donald

---

From: Gabriel Kerneis <[email-masked]>
Date: Thu, 27 Dec 2018 10:14:57 +0100

> Or https://tools.ietf.org/html/rfc8401 which has diagrams closer to my

I implemented recently an (unreleased) parser for RFC 6126 bis, and I would
definitely have appreciated diagrams looking like RFC 7176. I find them
much easier to read, especially with the "(X bytes)" annotations.

Gabriel