Subject: [babel] Roman Danyliw's Discuss on draft-ietf-babel-hmac-08: (with DISCUSS and COMMENT)

From: Roman Danyliw via Datatracker <[email-masked]>
Date: Wed, 07 Aug 2019 12:35:06 -0700

Roman Danyliw has entered the following ballot position for
draft-ietf-babel-hmac-08: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-babel-hmac/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

A few minor clarifications needed for implementation/precision in the security
claims:

(1) Section 1.2.  Per “any packet accepted as authentic is the exactly copy of
a packet originally sent”, this text can be read two ways – packet as Babel
packet or as an IP packet.  I think  mean the former.  Recommend making this
clearer as s/any packet/any Babel packet/

(2) Section 2, Per the paragraph, “By itself, this mechanism is safe against
replay …”, please reiterate that for the attack by C to work: A and B must have
both lost state; that C is replaying packets with PC previously sent by B
(e.g., n+2).

(3) Section 4.1.  Per “The node takes the concatenation of the pseudo-header
and the packet including the packet header but excluding the packet trailer
(from octet 0 inclusive up to (Body Length + 4) exclusive)”, as input for the
HMAC.  “packet” is used to sometimes mean IP packet and sometimes a Babel
packet carried in an IP packet.  As such, the above sentence could be
interpretation as:

Option #1: HMAC(pseudo-header + the IP header + Babel packet header + Babel
packet body)

Option #2: HMAC(pseudo-header + Babel packet header + Babel packet body)

I believe it is option #2.  Please be very clear in this text.

Other items:

(4) Section 2.  This section suggests that “one or more HMACs can be appended
to the packet”.  Under what conditions would it be more than one?  What happens
if only some of the HMACs are valid?  Is use of the same key assumed?

(5) Section 4.1.  The hash algorithm appears to be negotiated/set out of band
(rather than negotiated). The text should explicitly state that somewhere.

(6) Section 6.  Per “In particular, reception of a packet with no correct HMAC
creates no local state  whatsoever  (Section 4.3)”, unless this HMAC
verification is happening on the NIC, this doesn’t seem sufficiently precise. 
The “no local state” claim is likely true only as it relates to the tables data
structures describes in Section 3.  However, the IP and DTLS stack certainly
have to account for the packet.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

(7) Section 1 enumerates a series of attacks.  These are different than the
ones listed in Section 6 of draft-ietf-babel-rfc6126bis and Section 1 of
draft-ietf-babel-dtls.  As HMAC and DTLS are mitigations for attacks in
draft-ietf-babel-rfc6126bis, they really should be harmonized.

(8) Section 1.  Per the “spoof of a malformed packet”, how would an HMAC
address this?  Even assuming that a node discards the message without
processing if the HMAC is bad, this would still be a problem from a malicious
peer.

(9) Editorial nits:
-- Section 1.  “seqno” is commonly used in Babel text but it needs to be cited
of explained here on for used -- s/seqno/sequence number/

-- Section 1.2.  Editorial. s/serious effort/effort/

-- Section 3.1. Typo.  s/authentified/authenticated/

-- Section 5.3.  Typo.  s/minumum/minimum/

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 08 Aug 2019 15:46:56 +0200

Dear Roman,

Thank you for your review.

> clearer as s/any packet/any Babel packet/

Done.

> (2) Section 2, Per the paragraph, “By itself, this mechanism is safe
> against replay …”, please reiterate that for the attack by C to work:
> A and B must have both lost state; that C is replaying packets with PC
> previously sent by B (e.g., n+2).

Only A needs to have lost its state.  Clarified.

> packet carried in an IP packet.

Clarified.

> (4) Section 2.  This section suggests that “one or more HMACs can be
> appended to the packet”.  Under what conditions would it be more than
> one?  What happens if only some of the HMACs are valid?  Is use of the
> same key assumed?

This section is a conceptual overview, and is optimised for easy
readability.  The exact details are given in 4.3.  I've added the
paranthetical remark "(one per key)", but refuse to overload this section
any further.

> (rather than negotiated). The text should explicitly state that somewhere.

Section 3.1.

> (6) Section 6.  Per “In particular, reception of a packet with no
> correct HMAC creates no local state whatsoever (Section 4.3)”, unless
> this HMAC verification is happening on the NIC, this doesn’t seem
> sufficiently precise.  The “no local state” claim is likely true only as
> it relates to the tables data structures describes in Section 3.
> However, the IP and DTLS stack certainly have to account for the packet.

There is no DTLS in this protocol.

I'm leaving this sentence as is -- after trying it out, I don't think that
mentioning the small amount of state caused by reception of a UDP packet
(an ND entry) is informative.

> draft-ietf-babel-rfc6126bis, they really should be harmonized.

I'll look at making this uniform between the three drafts.

> processing if the HMAC is bad, this would still be a problem from
> a malicious peer.

Please explain.

> (9) Editorial nits:

Fixed, thanks.

-- Juliusz

---

From: Roman Danyliw <[email-masked]>
Date: Thu, 8 Aug 2019 22:03:44 +0000

Hi Juliusz!

Thanks for the quick response.  Responses inline ...

> -----Original Message-----
> From: iesg [mailto:[email-masked]] On Behalf Of Juliusz Chroboczek
> Sent: Thursday, August 8, 2019 9:47 AM
> To: Roman Danyliw <[email-masked]>
> Cc: Donald Eastlake <[email-masked]>; [email-masked]; The IESG
> <[email-masked]>; [email-masked]; [email-masked]
> Subject: Re: Roman Danyliw's Discuss on draft-ietf-babel-hmac-08: (with
> DISCUSS and COMMENT)
> 
> Dear Roman,
> 
> > (1) Section 1.2.  Per “any packet accepted as authentic is the exactly
> > copy of a packet originally sent”, this text can be read two ways –
> > packet as Babel packet or as an IP packet.  I think  mean the former.
> > Recommend making this clearer as s/any packet/any Babel packet/
> 
> Done.
> 
> > (3) Section 4.1.  Per “The node takes the concatenation of the
> > pseudo-header and the packet including the packet header but excluding
> > the packet trailer (from octet 0 inclusive up to (Body Length + 4)
> > exclusive)”, as input for the HMAC.  “packet” is used to sometimes
> > mean IP packet and sometimes a Babel packet carried in an IP packet.
> 
> Clarified.

Thanks.

> This section is a conceptual overview, and is optimised for easy readability.
> The exact details are given in 4.3.  I've added the paranthetical remark "(one
> per key)",

Ah, I see it now in Section 4.3  Thanks for the pointer.  This text explains the issues of around keys processing.  I couldn't explicitly find text explaining when multiple HMAC TLVs should be sent.  Where should I be looking?

> but refuse to overload this section any further.

No worries.  I'm not suggesting that.  FWIW, I think this narrative section works really well.

> > (5) Section 4.1.  The hash algorithm appears to be negotiated/set out
> > of band (rather than negotiated). The text should explicitly state that
> somewhere.
> 
> Section 3.1.

Got it.  Thanks.

> > sufficiently precise.  The “no local state” claim is likely true only
> > as it relates to the tables data structures describes in Section 3.

Right.  Sorry about that.  I have both documents open.

> (an ND entry) is informative.

I'm not tracking.  "Whatsoever" is an absolute statement.  By all means caveat the sentence by saying no local Babel state (or something to that effect).

> > (7) Section 1 enumerates a series of attacks.  These are different
> > than the ones listed in Section 6 of draft-ietf-babel-rfc6126bis and
> > Section 1 of draft-ietf-babel-dtls.  As HMAC and DTLS are mitigations
> > for attacks in draft-ietf-babel-rfc6126bis, they really should be
> harmonized.
> 

Thanks for coordinating across documents.  Your call, but an alternative to making the text more uniform might be to have the HMAC and DTLS drafts just reference the core Babel drafts in some way.

> > (8) Section 1.  Per the “spoof of a malformed packet”, how would an
> > HMAC address this?  Even assuming that a node discards the message
> > without processing if the HMAC is bad, this would still be a problem
> > from a malicious peer.
> 
> Please explain.

The initial situation I had in mind was if there was a compromised Babel node.  It will have the key to produce a valid HMAC on the crafted packet to cause the peer Babel node to process the packet.

Another situation which would not involve a malicious Babel node with the key is a node running a buggy Babel implementation.  Imagine this implementation has some kind of (buffer overflow) vulnerability in the routines which parses the packet payload to find the HMAC TLV.  In the worst circumstance, this could hypothetically compromise the Babel node even before the code to verified HMAC and choose to discard the packet or not is made.

> Fixed, thanks.

Thanks.

Regards,
Roman

---

From: Juliusz Chroboczek <[email-masked]>
Date: Sat, 17 Aug 2019 00:53:23 +0200

> Ah, I see it now in Section 4.3 Thanks for the pointer.  This text
> explains the issues of around keys processing.  I couldn't explicitly
> find text explaining when multiple HMAC TLVs should be sent.  Where
> should I be looking?

Section 5 (key rotation), formerly Appendix A:

   In order to perform key rotation, the new key is added to all the
   nodes; once this is done, both the old and the new key are sent in
   all packets, and packets are accepted if they are properly signed by
   either of the keys.  At that point, the old key is removed.

> I'm not tracking.  "Whatsoever" is an absolute statement.  By all means
> caveat the sentence by saying no local Babel state (or something to that
> effect).

Done.

> The initial situation I had in mind was if there was a compromised Babel
> node.  It will have the key to produce a valid HMAC on the crafted
> packet to cause the peer Babel node to process the packet.

Yes, this protocol assumes that only honest nodes know the secret key.  We
make no guarantees, not even of any kind, if you've lost your keys.

> Another situation which would not involve a malicious Babel node with
> the key is a node running a buggy Babel implementation.  Imagine this
> implementation has some kind of (buffer overflow) vulnerability in the
> routines which parses the packet payload to find the HMAC TLV.  In the
> worst circumstance, this could hypothetically compromise the Babel node
> even before the code to verified HMAC and choose to discard the packet
> or not is made.

Yes, it could.

That is the reason why HMAC keys sit in the packet trailer rather than in
the packet body: you may use a simplified parser that is only able to
parse the packet trailer.  This parser does not modify any global state,
it does not modify the packet (there's no need to clear the MAC before
recomputing it), and it just returns one bit of information.  You are
expected to vet this specialised parser very carefully, or even run it
out-of-process.

This was debated by the WG; however, once both Toke (BIRD) and Weronika
and Clara (babeld) had implemented parsing the packet trailer, we agreed
that this approach makes the implementation much simpler and less
error-prone.

-- Juliusz