Re: [Pearg] I-D Action: draft-irtf-pearg-numeric-ids-generation-02.txt

[Fernando Gont <[e-mail]>]

Hello, Chris,

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


On 20/5/20 00:04, Christopher Wood wrote:
> I reviewed this document with no hats on. My feedback is below (I
> hope it's useful)!
> 
> In general, this is a nicely written document with clear structure
> and guidance. I think there are some issues with the details, and
> perhaps a bit too much redundancy, but overall it's heading in the
> right direction. I especially like the appendix, which captures our
> lessons learned the hard way.
> 
> Before progressing this draft further, is it possible to get feedback
> from folks in the transport area? We might also try and request an
> additional review from secdir, if they're willing.

Is there some sort of procedure/guideline for this? Should we authors 
e.g. request comments on the tsv-area list? Contact the ADs? Something else?



> Document:
> https://datatracker.ietf.org/doc/html/draft-irtf-pearg-numeric-ids-generation-02
>
>  Summary: Has Issues
> 
> Comments:
> 
> - Section 1. I'd suggest rephrasing this text:
> 
> 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, and 
> inappropriate algorithms to generate transient numeric identifiers 
> are either suggested in the specification or selected by 
> implementers.
> 
> to something that emphasizes these problems in the past. Certainly,
> modern protocols such as QUIC paid very close attention to these
> details.

I haven't looked at QUIC myself yet -- hence I cannot speak for quic. 
But it would seem to me that the statement is generally true for both 
protocols and even more when it comes to implementations.

For example, I just skimmed through Section "5.1.  Connection ID" of 
"draft-ietf-quic-transport-28", and they don't seem to spell out the 
interoperability requirements of the connection IDs. -- I'd assume 
"uniqueness, hard failure", but I'm just guessing. This text also seems 
to fail to suggest a recommended algorithm:
"   Each
    endpoint selects connection IDs using an implementation-specific (and
    perhaps deployment-specific) method which will allow packets with
    that connection ID to be routed back to the endpoint and identified
    by the endpoint upon receipt."


Thoughts?



> - Section 2. The terminology is duplicated in the ids-history draft.
> Perhaps we can note this?

ids-history has removed everything by the definition of "transient 
numeric identifier". As such, maybe it's better to clarify this in the 
ids-history draft instead?

i.e., add something (to the ids-history draft) like:
"This document employs terminology defined in 
[draft-irtf-pearg-numeric-ids-generation], which is reproduced here for 
convenience:"

Or, if you prefer, just eliminate all definitions from the ids-history 
draft, and say:

"This document employs the term "transient numeric identifiers" as 
defined in [draft-irtf-pearg-numeric-ids-generation]"

Any preference?



> - Section 3. Can the attacker generate traffic at will,

Yes.

> or modify existing traffic? 

No -- although, thinking out loud, I'm not sure this would be relevant. 
(Should we clarify this?)


> I would reference RFC3552 for the threat model
> here. (The same should probably go for the ids-history draft.)

Could you please elaborate?



> - Section 4. This seems largely redundant with the history draft, no?
> Can we remove this section entirely?

Not really. Section 1 of ids-history essentially doesn't delve into e.g. 
why protocol specifications have led to vulnerable implementations. In a 
way, Section 4 of this document kind of explains what Section 1 provides 
as anecdotal evidence.



> - Section 7. I would drop either 7.1.1 or 7.1.2. We don't need two
> algorithms for generating random identifiers, do we? (7.1.2 seems
> simpler to reason about, given that it calls random during each loop
> iteration.) Can we also clarify the failure probability of this
> algorithm?

While presented as two different randomization algorithms, strictly 
speaking they are really two different ways of dealing with the 
collisions. In one case (I), success becomes ore deterministic, at the 
expense of decreased randomization.

So maybe one might want to describe these two as such?



> - Section 7.3. Is "F() must be a cryptographically-secure hash
> function" correct? What we really want, I think, is for F to be a PRF
> keyed on "secret_key". RFC6528 uses a PRF here, for example
> (https://tools.ietf.org/html/rfc6528).

I'm not a mathematician, but, what I seem to remember is that strictly 
speaking a hash function is one of the possible "implementations" of a 
PRF. So PRF would be a more correct (general) term, at the expense of a 
possible "What the heck is a PRF?" moment for the reader. :-)

I will update this as suggested.


> Similar to the comment above, can we clarify the failure probability?

Failure as in probability of collisions, or what?



> - Section 7.3. The "check_suitable_id" function is described inline
> here, yet is referenced in prior sections. Can we lift the utility
> functions used in these algorithms to a prior section to simplify the
> text that goes along with each function?

Maybe explain these in the Section 7 parent section?



> 
> - Section 7.4.1. Code nit (inconsistent whitespace between comments
> and the next expression, and inconsistent spacing around braces
> 
> /* Initialization code */ id_inc = 1;
> 
> /* Numeric ID selection function */ //----> count = max_id - min_id +
> 1;
> 
> ...
> 
> if(lookup_counter(CONTEXT) == ERROR){ <--- insert spaces between "if"
> and "(", for example
> 
> This should also apply to other code blocks, too. :-)

Will fix. Thanks!



> - Section 7.4.1. id_inc seems like it should be a parameter, rather
> than baked into the algorithm.

Yep. Maybe move it and explain it to (parent) Section 7?




> - Section 7.4.2. Again, I think we need a PRF here for F().

Will fix this.



> - Section 7.4.3. I would remove this section. Do any implementations
> use this particular algorithm?

IIRC Microsoft uses it for their IP ID generator



> - Section 8/9. I wonder if these sections in their entirety should be
> moved to the ids-sec-considerations draft, if only because they help
> motivate the need for these considerations. (In contrast, I see this
> document focusing on the *mechanisms* by which these considerations
> can be addressed.) What do you think?

You raise a very good and valid point, indeed!

These are my only two concerns:

1) We're still waiting for progress on ids-sec-considerations. If 
feasible, I think it would be perfectly okay to move Sections 8 and 9 to 
ids-sec-considerations. However, I'd prefer not to move these to 
Sections to that document before there is a concrete plan to progress 
ids-sec-considerations. Otherwise we run the risk of completely loosing 
the text.

2) Since the analysis in Section 9 refers to the different categories, I 
guess we should at least possibly reproduce Table 1 and its footnotes 
into ids-sec-considerations?

Thanks a lot!

Cheers,
-- 
Fernando Gont
e-mail: [e-mail] || [e-mail]
PGP Fingerprint: 7809 84F5 322E 45C7 F1C9 3945 96EE A9EF D076 FFF1