Subject: [Pearg] Tsvart early review of draft-irtf-pearg-numeric-ids-generation-02

From: Michael Tüxen via Datatracker <[email-masked]>
Date: Fri, 07 Aug 2020 05:22:08 -0700

Reviewer: Michael Tüxen
Review result: Ready with Issues

The document is well written, provides algorithms which could be used to
address identified problems. One  could add some text covering TCP timestamps.

Section 1 states:
"Recent history indicates that when new protocols are standardized or
new protocol implementations are produced, the security and privacy
properties of the associated identifiers tend to be overlooked,..."
How does this related to recent/current activities like SCTP/DTLS or QUIC?

The Algorithms in 7.1.1 and 7.1.2 can be substantially simplified when
check_suitable_id() always returns true. Why are not the simplified algorithms
shown?

The algorithm in 7.3 (and later) uses a function F and it is stated that F must
be a cryptographically-secure hash function. Couldn't you also use something
like SipHash.

When reading the algorithm in 7.4.1, I had the impression that it should also
work with id_inc > 1 (by just changing that parameter). If that is true, I
guess you would need to change "if (next_id == max_id)" to "if (max_id -
next_id < id_inc) {" and also "next_id = min_id;" to "next_id = min_id +
(id_inc - (max_id - next_id + 1));".  If you don't want to be that generic, you
might want to remove id_inc and just use 1.

The algorithm in 7.4.1 also calls "store_counter(CONTEXT, next_id)" when
returning ERROR. Why?

The algorithms given before 9.4 are given in C code. The Algorithm in 9.4
breaks with this rule. Is this intended?

Nits:
Page 10: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 11: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 12: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 14: if(lookup_counter(CONTEXT) == ERROR){ -> if (lookup_counter(CONTEXT)
== ERROR) { Page 14: if (check_suitable_id(next_id)){ -> if
(check_suitable_id(next_id)) { Page 14: }\n else { -> } else { Page 16:
if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id)) Page 18:
if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id)) Page 24:
linear(CONTEXT_2)= linear(CONTEXT_2) + increment(); -> linear(CONTEXT_2) =
linear(CONTEXT_2) + increment(); Page 24: if(check_suitable_id(next_id)) -> if
(check_suitable_id(next_id))

---

From: Fernando Gont <[email-masked]>
Date: Thu, 27 Aug 2020 09:10:08 -0300

Hi, Michael,

Thanks a lot for your feedback! In-line...

> address identified problems. One  could add some text covering TCP timestamps.

You mean e.g. to spell out which of the proposed algorithms one might 
use for TCP timestamps?

> How does this related to recent/current activities like SCTP/DTLS or QUIC?

SCTP (RFC4960) is similar to TCP, in this respect. OTOH, I have only 
skimmed through the DTLS (RFC6347), and it seems that it initially sets 
sequence numbers to 0. -- while these are meant to be protected, I'm 
curious if they could have done with monotonically increasing sequence 
numbers ala 6528, or with a random origin.

> check_suitable_id() always returns true. Why are not the simplified algorithms

Are you referring to the case where an implementation need not check the 
generated ID against other existing I-Ds?

(FWIW, in order for check_suitable_id()to always return "true", it means 
that there are essentially no requirements on the ID -- just pick any 
random number... at which point, there's not really much of an algorithm 
( Section 7.1.1 and Section 7.1.2 cover "recovery" strategies for the 
cases where the algorithm finds collisions.

> be a cryptographically-secure hash function. Couldn't you also use something

Siphash *has* been employed for generating IDs.. although it seems that 
the 64-bits versions are considered too weak. see e.g. 
https://lore.kernel.org/patchwork/patch/1083206/

> might want to remove id_inc and just use 1.

You raise a very good point. I'm curious whether it might make sense to 
include the most generic version (as you suggest), and then also show 
the simplified version when id_inc=1  ? Thoughts?

Also, we may want to note that, while using increments of "1" reduces 
the reuse frequency, in principle id_inc does not even need to be 
constant... -- in this case, since the failure severity is "hard 
errors", one probably ones to keep id_inc small, to reduce the ID resuse 
frequency.

> The algorithm in 7.4.1 also calls "store_counter(CONTEXT, next_id)" when

At least with the current increment values, if the algorithm iterates 
over count = max_id - min_id + 1; values (i.e., between the range min_id 
and max_id, the value to be stored would be the same one as originally 
found, so it's actually a no-op. I will remove it.

> breaks with this rule. Is this intended?

Are you referring to this line:
            linear(CONTEXT_2)= linear(CONTEXT_2) + increment();
?

If so, I'll try to find a replacements that's C-compatible and that's 
still clear...

Thanks!

Regards,
-- 
Fernando Gont
SI6 Networks
e-mail: [email-masked]
PGP Fingerprint: 6666 31C6 D484 63B2 8FB1 E3C4 AE25 0D55 1D4E 7492

---

From: Eric Rescorla <[email-masked]>
Date: Thu, 27 Aug 2020 06:07:45 -0700

> numbers ala 6528, or with a random origin.

We perhaps could have, but to our knowledge the security functionality of
the protocol does not depend on unpredictability of the Record Sequence
Number. The same is true of QUIC. Note that both QUIC and DTLS 1.3 protect
this portion of the header (QUIC calls it the Packet Number).

-Ekr