[Ntp] Danny's Review (was Re: draft-ietf-ntp-roughtime-05: tag change makes implementation more complex)

[Watson Ladd <[e-mail]>]

On Mon, Sep 27, 2021 at 9:31 AM Danny Mayer <[e-mail]> wrote:
>
> I have been reviewing the draft again and it's not ready for
> publication. Here are a number of additional issues.

Thanks for your review! We shall revise soon.
>
> 1. Section 6: "Servers SHOULD support the UDP transport mode, while TCP
> transport is OPTIONAL."
>
> Unless you specify some other transport one of these needs to be
> mandatory. "SHOULD" is probably a "MUST". There is no explanation about
> why TCP might be used instead of UDP, so that needs clarification. If
> there is a good reason to be sending multiple requests and responses
> over TCP, please explain why this is preferable.

We can add explanatory text: TCP is for supporting Tor. I think making
UDP a MUST works, although chances are not every server will
necessarily have as good reachability over UDP.

>
> 2. The message format (Figure 1) seems bizarre. Number of pairs
>
> The number of pairs shouldn't need 32 bits, I suspect that 16-bits is
> more than enough. You can use the rest of the 16 bits for something
> else, like version information.

We want to minimize the number of numeric types. There's plenty of
room for a version number in its own tag which we do.

>
> 3. The message format (Figure 1). You seem to have tag identifiers after
> offsets and then values. Why not have the tag followed by a length of
> the value followed by the label? Why are you going to all the trouble of
> using offsets? Even if you are going to use offsets why isn't the tag
> identifier before the offset or even a tag identifier with the offset?
> The offsets don't need to be 32-bits in size, 16-bits is more than
> enough unless you are intending to send megabytes of packets. Note that
> is seems to be assumed that the offsets are in exactly the same order as
> the tag identifier specified separately. That's a bad assumption to
> make. It's not clear why the offsets have to be in strict ascending
> order, it shouldn't matter as long as you can correlate it to the tag
> identifier.

Nested TLV protocols are a major source of vulnerablities and
surprises. By having the tags always sorted the same way, and the
offsets in ascending order, we ensure that the data of different
sections is nonoverlapping and that identical tag->data maps have
identical representations. The 4 byte alignment is sometimes
convenient: we're not hurting for space here.

>
> 4. TAG values
>
> Here's another problem. Consider this from section 6.1.1: "the VER tag
> contains a list of versions". Actually the tag appears to be just a tag.
> If I interpreted the format from Figure 1 correctly, the TAG identifier
> is separate from the values and in this case the list of versions. None
> of the TAGs specified in section 6 specify the format of these values
> and what the list in the case of VER would look like. The length of
> these values are not specified anywhere except in the offset section
> which has to then interpret how long the value is which may or may not
> be a multiple of 4 bytes.

Perhaps contains is not the right verb. We mean that the value
associated to the tag is the list of versions. All values are
multiples of 4 bytes. The message formats delinates the length of all
values.

>
> 5. VER values
>
> That is the meaning of the version? What does the version depend on? Is
> this the version of the protocol? Of the message? Of something else?

Of the protocol. This will be clarified.
>
> 6. Secton 6.2.5 SREP
>
> This is totally unclear. If references other tags: ROOT, MIDP, RADI,
> DUT1, DTAI, LEAP, but they seem to be part of the SREP values rather
> then part of the overall message. Are they a separate set of tags or do
> they only appear under SREP and if only under SREP then how is SREP
> formatted.

SREP is formated as Roughtime message. That is the tags and their
associated values are serialized in the same offset tag data format.
The rest of the paragraphs in this section define each of these
values.

>
> 7. INDX tag
>
> It's unclear what the need for this tag is. Section 6.2.7 says that it
> identifies the postion of the NONC but since the NONC is already in the
> message this seems to be a duplicate. The reference to the Merkle tree
> doesn't clarify this at all. Confusingly then Section 6.3.1 goes on to
> say: "The bits of INDX are ordered from least to most significant in
> this algorithm" which seems to be unrelated to the NONC.

The NONC is inserted into a Merkel tree that contains the NONC of
other messages. This is then signed. INDX indicates where in the
Merkel tree NONC is. We will add text to clarify this.

>
> 8. CERT
>
> The contents seem to include other tags. It's not clear why these, like
> the ones mentioned in SREP are not separately specified.
>
> 9. Hashing algorithm in Section 6.3
>
> It's not clear why the algorithm can only be SHA-512. This really should
> be separately referenced so that if SHA-512 is considered too week in
> can be replaced by a different hashing algorithm like say HMAC-SHA-512,
> without having to update the proposed RFC.

Part of what is required for roughtime is interoperability between all
servers, auditors, and clients. Using only a single hash function and
signature, and then migrating entire versions, is a better approach
than permitting everything.

>
> 10. Grease (Section 8)
>
> This seems to be garbled: "Servers MUST NOT send back responses with
> incorrect times and valid signatures.  Either signature MAY be invalid
> for this application." What does that even mean?

We mean that servers may send back invalid signatures. This text will
be clarified.

>
> 10. Examples of each and every tag and value would be helpful. It's hard
> to understand what each tag is providing and how the values are
> formatted and interpreted.

We shall add test vectors.

>
> This is enough for now.

Thank you for your detailed comments.

>
> Danny
>
>
>


-- 
Astra mortemque praestare gradatim