Subject: [Rift] RIFT fingerprint coverage

From: Bruno Rijsman <[email-masked]>
Date: Sun, 21 Jul 2019 11:35:01 +0200

Hi Tony and others,

I just noticed that the RIFT fingerprints do not cover the key-id nor the fingerprint-length nor the major-version.

This means that if an adversary changes the key-id, fingerprint-length, or major-version, it will not be detected by the fingerprint.

I cannot think of an immediate attack vector (maybe force a weaker algorithm by changing the key-id), but "it feels wrong".

I wonder whether we should expand the coverage of fingerprints slightly, as shown below?

Note that AFAIK in OSPF (for example) the digest does cover the key-id and auth-data-len.

Suggested change:

       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

      UDP Header:
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |           Source Port         |       RIFT destination port   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |           UDP Length          |        UDP Checksum           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

      Outer Security Envelope Header:
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |           RIFT MAGIC          |         Packet Number         |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |    Reserved   |  RIFT Major   | Outer Key ID  |    Outer      |
      |               |    Version    |               | Fingerprint   |
      |               |               |               |    Length     |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      ~  Outer Security Fingerprint covers all content starting       ~
      |  at the Reserved field in the Outer Security Envelope Header  |
      |                                                               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Weak Nonce Local              | Weak Nonce Remote             |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            Remaining TIE Lifetime (all 1s in case of LIE)     |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

      TIE Origin Security Envelope Header:
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                               |  TIE Origin   |
      |              TIE Origin Key ID                |  Fingerprint  |
      |                                               |    Length     |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      ~  TIE Origin Security Fingerprint covers all content starting  ~
      |  at the TIE Origin Key ID field                               |
      |                                                               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

      Serialized RIFT Model Object
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      ~                Serialized RIFT Model Object                   ~
      |                                                               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

PS 1: I know that the rift-magic and packet-nr fields are left out of the fingerprint coverege on purpose, so I left it that way.

PS 2: I also updated the names have a clearer distinction between the two fingerprints.

— Bruno

---

From: Bruno Rijsman <[email-masked]>
Date: Sun, 21 Jul 2019 12:39:55 +0200

Fixing one important mistake in bold red below:

> On Jul 21, 2019, at 11:35 AM, Bruno Rijsman <[email-masked]> wrote:
> 
> Hi Tony and others,
> 
>       |  but excluding this fingerprint                               |
>       |  at the TIE Origin Key ID field but excluding this fingerprint|
>       |                                                               |

---

From: Antoni Przygienda <[email-masked]>
Date: Sun, 21 Jul 2019 14:22:35 +0000

1.  I don't like the change of inner security key name, we may one day protect something else than TIEs with it and on top it makes the naming less intuitive, the outer and inner works really well IMO
  2.  first, you can't extend the protection the way you like unless we have a complicated procedure where you protec with fingerprint 0, then you write fingerprint and on the other side you ahve to 0 out the fingerprint to check and so on. I wrote such code over protocols and it's very fragile.
  3.  second, even if we do I don't think we improve the attack envelope or actually worsen it. Let's go through it
     *   major version is attacked, any change will drop the packet due to mismatch. if we protect it, we calculate the hash and it fails and outcome is the same modulo we can be computationally overrun
     *   outer key id is attacked/fingerprint length is attacked, all same outcome ...

now, you can argue that an attacker can modify stuff _behind_ the fingerprint and with that attack protocol computationally but there is no way around that, we won't detect modification otherwise wheeras modifying major version/key id/fingerprint lenght basically leads to drops on any change and protecting them only exposes us computationally for no benefit

There is a very weird case where one could record version X, wait until protocol runs X+1 and having matched the nonces (age of the universe probably not enough to get all those conditions in place), modify packets recorded with X to major version X+1 and see whether the parser will croak. Given the parser is model-based and we saw that it deals with the issue of umatching models fine by omitting, filling in defaults etc even that is not really a viable attack

--- tony
________________________________
From: Bruno Rijsman <[email-masked]>
Sent: Sunday, July 21, 2019 6:39 AM
To: Tony Przygienda <[email-masked]>
Cc: [email-masked] <[email-masked]>
Subject: Re: RIFT fingerprint coverage

On Jul 21, 2019, at 11:35 AM, Bruno Rijsman <[email-masked]<mailto:[email-masked]>> wrote:

Hi Tony and others,

Suggested change:

       0                   1                   2                   3

      UDP Header:

— Bruno

---

From: Bruno Rijsman <[email-masked]>
Date: Sun, 21 Jul 2019 17:31:29 +0200

> I don't like the change of inner security key name, we may one day protect something else than TIEs with it and on top it makes the naming less intuitive, the outer and inner works really well IMO

I am perfectly fine with calling it inner security key instead of TIE origin security key.  Actually, I also like that better.

I only called it TIE origin security key because it is part of what is currently called (and AFAIK has always been called) the TIE Origin Security Envelope Header.  So, I was just trying to be consistent with what was already there.

Soooo…. if we agree to call it inner security key then we should consistently call it inner security key / inner security key id / inner security fingerprint / inner security envelope header everywhere and purge any mention of TIE origin key / TIE origin key id / TIE origin security fingerprint / TIE origin security envelope header completely from the draft (and code for that matter, at least the CLI and yaml files).

Speaking of consistency: we should decide whether it is “inner key” or “inner security key” or “inner authentication key” and be consistent about that as well.  In RIFT-Python followed your RIFT-Juniper example and used “authentication key” instead of just “key” or “security key”.

I propose we use the following going forward:

outer authentication key
outer authentication key id
outer authentication fingerprint
outer authentication envelope (or "outer authentication envelope header” but that seems redundant)

inner authentication key
inner authentication key id
inner authentication fingerprint
inner authentication envelope (or “inner authentication envelope header” but that seems redundant)

Final obsessive compulsive nitpick :-) :  in the Thrift data model we should be consistent in CamelCaseNamingStyle for types and underscore_naming_style for variables (for example: type InnerAuthenticationKeyID and constant invalid_inner_authentication_key_id).

— Bruno

---

From: Bruno Rijsman <[email-masked]>
Date: Sun, 21 Jul 2019 17:52:13 +0200

> On Jul 21, 2019, at 4:22 PM, Antoni Przygienda <[email-masked]> wrote:

> 2. first, you can't extend the protection the way you like unless we have a complicated procedure where you protec with fingerprint 0, then you write fingerprint and on the other side you ahve to 0…

I think we should focus on whether it is a security issue or not (which was #3 of your response) rather than whether the implementation is difficult or not.

That said, the implementation is actually quite trivial and not fragile. 

Every signing library that I ever worked with has an update method that allows you to incrementally compute a digest.

Python provides hmac.update() and hashlib.update(). Rust provides hmac.input().

Python example:

import hmac
packet = b'Just imagine for a second that this is a protocol packet.'
signer = hmac.new(key=b'top-secret', digestmod='sha256')
signer.update(packet[5:10])  # Digest over bytes 5-10
signer.update(packet[15:20]) # and also bytes 15-20
signer.update(packet[25:30]) # and also bytes 25-30
print(signer.digest())

You can use this feature to make re-computing the outer fingerprint for TIEs that are stored in the TIE database much cheaper, knowing that you have already computed (and saved the signer for) the inner fingerprint.

---

From: Bruno Rijsman <[email-masked]>
Date: Sun, 21 Jul 2019 18:03:59 +0200

> major version is attacked, any change will drop the packet due to mismatch. if we protect it, we calculate the hash and it fails and outcome is the same modulo we can be computationally overrun 
> outer key id is attacked/fingerprint length is attacked, all same outcome ...

The receiving RIFT router can drop packets with the wrong major version, with a wrong key-id (not in the accept key set), or a wrong fingerprint-length, without validating the fingerprint first.

This is for the same reason that a RIFT router can drop a packet with an out-of-range weak-nonce without validating the fingerprint first.

In general, it is safe to reject a packet when any field has some unacceptable value without validating the fingerprint first.

So, protecting these additional fields does not open up any new CPU denial of service attacks.

If a RIFT router is planning to accept a packet because all fields have an acceptable value, then it must validate the fingerprint first before doing so.

Or if a RIFT router is going to reject a packet but take some protocol action based on some field in that rejected packet anyway, then it would have to validate the fingerprint as well (happily, which currently don’t have this scenario in RIFT, as long as the packet-nr is truly only used for debugging and not for flow-control, for example).

So, the real question is: how sure are you that leaving the major-version, key-id, and fingerprint-lengths fields unprotected will not lead to some problems down the road?  (To quote Dirty Harry: “Do you feel lucky? Well, do ya, punk? :-)

— Bruno

---

From: Antoni Przygienda <[email-masked]>
Date: Mon, 22 Jul 2019 03:30:09 +0000

So dropping the "burden of proof" logical fallacy @ the end ;-) the only effect right now is to increase the difficulty of the implementation without a known attack if you access those fields without checking fingerprint first anyway. I can only imagine a very far fetched attack where someone is recording another version, matching up nonces and then wait for a major protocol version change and change version on old recordings to attack. Again, most likely as likely as age of universe to happen ;-)

Looking for more opinions ...

--- tony

________________________________
From: Bruno Rijsman <[email-masked]>
Sent: Sunday, July 21, 2019 12:03 PM
To: Antoni Przygienda <[email-masked]>
Cc: Tony Przygienda <[email-masked]>; [email-masked] <[email-masked]>
Subject: Re: RIFT fingerprint coverage

On Jul 21, 2019, at 4:22 PM, Antoni Przygienda <[email-masked]<mailto:[email-masked]>> wrote:

— Bruno

---

From: Tony Przygienda <[email-masked]>
Date: Mon, 22 Jul 2019 10:10:38 -0400

I think we misunderstood. If you validate things _before_ a fingerprint
(which we have here, without lenght in front of fingerprint we don't know
where to find it) you normally compute the fingerprint over empty
fingerprint & you have to validate the same way. This is normally the place
where problems occur due to unusal logic necessary in the right sequence

I agree, the discussion is is there an attack worth doing the extra work, I
don't see one + as I say we not really protect anything since we access the
fields before fingerprint anyway and any change, if not failing before
fingerprint will make fingerprint invalid anyway ...

--- tony

On Sun, Jul 21, 2019 at 12:04 PM Bruno Rijsman <[email-masked]>
wrote:

>    1. major version is attacked, any change will drop the packet due to
>       mismatch. if we protect it, we calculate the hash and it fails and outcome
>       is the same modulo we can be computationally overrun
>       2. outer key id is attacked/fingerprint length is attacked, all
>       same outcome ...
>
> now, you can argue that an attacker can modify stuff _behind_ the
> fingerprint and with that attack protocol computationally but there is no
> way around that, we won't detect modification otherwise wheeras modifying
> major version/key id/fingerprint lenght basically leads to drops on any
> change and protecting them only exposes us computationally for no benefit
>
>
> The receiving RIFT router can drop packets with the wrong major version,
> with a wrong key-id (not in the accept key set), or a wrong
> fingerprint-length, *without* validating the fingerprint first.
>
> This is for the same reason that a RIFT router can drop a packet with an
> out-of-range weak-nonce *without* validating the fingerprint first.
>
> In general, it is safe to reject a packet when any field has some
> unacceptable value *without* validating the fingerprint first.
>
> If a RIFT router is planning to accept a packet because all fields have an
> acceptable value, then it must validate the fingerprint first before doing
> so.
>
> Or if a RIFT router is going to reject a packet but take some protocol
> action based on some field in that rejected packet anyway, then it would
> have to validate the fingerprint as well (happily, which currently don’t
> have this scenario in RIFT, as long as the packet-nr is truly only used for
> debugging and not for flow-control, for example).
>
> So, the real question is: how sure are you that leaving the major-version,
> key-id, and fingerprint-lengths fields unprotected will not lead to some
> problems down the road?  (To quote Dirty Harry: “Do you feel lucky? Well,
> do ya, punk? :-)
>
> — Bruno
>
>