Re: [Ntp] Comments on draft-ietf-ntp-roughtime-02

[Marcus Dansarie <[e-mail]>]

Hi!

I am the one who originally suggested many of the changes to the draft
that you mention. I have worked closely with Aanchal and Watson in
creating a lot of the text in the draft.

This mail from February and the discussion that followed contains some
background and motivation to most of the changes in question:
https://mailarchive.ietf.org/arch/msg/ntp/4ZY0wIUYtG1tgYR_bgl-HfbWWq8/

Short comments inline follow below.

Kind regards,
Marcus Dansarie

On 2020-06-14 21:34, Stuart Stock wrote:

> ## Keep the "rough" in "Roughtime"

I'd like to start out with that I definitely agree with this. Roughtime
should not be "a different/better NTP".

> The authors of the original Roughtime protocol definition [roughtime] state that a 
> deliberate goal of the Roughtime protocol was "...time synchronisation to within 10 
> seconds of the correct time." Those authors go on to state that "[i]f you have 
> _serious_ time synchronisation needs you‘ll want the machinery in NTP or even PTP..."
> 
> The draft RFC proposes addition of the DUT, DTAI, and LEAP fields run contrary to this design 
> intent. These fields provide time synchronization features that duplicate those in NTP and PTP. 
> Precise time sync should be delegated to NTP and PTP and their ecosystems.
> 
> Simplicity of the Roughtime protocol encourages simple implementations. Simplicity
> in implementation is a key to assurance that implementations are correct and secure. 
> Increasing the number of fields and field types that must be implemented runs contrary 
> to ease of implementation. Simplicity--and by extension security--should be a deliberate 
> and top-of-mind design goal of the Roughtime standardization process.
> 
> Multiple existing IETF protocols address needs of highly accurate time synchronization 
> and should be used where timing precision requires. Keep Roughtime "rough".
> 
> Suggestion: remove DUT, DTAI, and LEAP fields along with int32 type

The DUT1, DTAI, and LEAP are not meant for precise time synchronization,
i.e. applications with very high accuracy. Instead, they are there to
provide a means for transferring offsets between timescales in a secure
manner. There is currently no time protocol that does this.

Note that the DUT1, DTAI, and LEAP fields are all OPTIONAL. Any server
or client implementation is free to not implement and ignore them.

> ## The LEAP field is not necessary 
> 
> Roughtime can handle leap seconds and time uncertainty/discontinuity complications without
> addition of new tags/fields. The RADI field already provides a method for a Roughtime 
> implementation to express time uncertainty in responses [draf02]: 
> 
>     The RADI tag value is a uint32 representing the server's estimate of
>     the accuracy of MIDP in microseconds.  Servers MUST ensure that the
>     true time is within (MIDP-RADI, MIDP+RADI) at the time they compose
>     the response message.
> 
> In the case of a leap second, a Roughtime server can increase the value returned in 
> the RADI field to account for the uncertainty introduced by a leap second. The increase 
> in RADI value can persist as long as necessary (the duration of a leap second "smear" 
> for example [AWS-smear] and [Google-smear]).
>      
> Suggestion: remove DUT, DTAI, and LEAP fields along with int32 type

Leap smearing is almost considered a bad word in the NTP WG...

As mentioned above, the purpose of LEAP is not just to provide
information about an imminent leap second, but also to provide access to
verifiable leap second information. Ignoring leap seconds risks causing
large problems further on. This is something that NTP suffers from.

Smearing leap seconds as you describe and ignoring/not sending LEAP
messages is still completely within the specification.

> ## Introduction of a signed int32 type creates a new security risk 
> 
> The addition of the signed int32 field type (required to support the DUT, DTAI, and 
> LEAP fields) exposes Roughtime implementations to a new *class* of software errors
> around sign/unsigned conversions. 
> 
> The Common Weaknesses Enumeration Top 25 Most Dangerous Software Errors [CWE-TOP25] 
> lists "Integer Overflow or Wraparound" as the #8 most frequent software weakness. The 
> detail page on integer overflow [CWE-190] identifies signed/unsigned confusion, 
> unintentional wrap-around, and lack of range checks as sources of software errors.
> 
> Introducing a signed integer type into Roughtime does not guarantee these issues
> occur. However it does introduces an entirely new area of concern that would not
> exist if these signed fields were omitted. 
> 
> Suggestion: remove DUT, DTAI, and LEAP fields along with int32 type

All problems you describe here, except for "signed/unsigned confusion"
apply to all integer representations. Unsigned integers also wrap
around. I fail to see the problem here.

> ## Restore the 1024 byte minimum request size requirement
> 
> Draft 02 states that: 
> 
>     Responding to requests shorter than 1024 bytes is OPTIONAL 
>     and servers MUST NOT send responses larger than the requests 
>     they are replying to.
> 
> The minimum request size requirement exists to prevent a roughtime server from becoming
> a DDoS amplifier [CF-DDoS]. A minimum request size of of 1024 bytes ensures that even 
> a busy roughtime server signing a Merkle tree of 64 requests generates a response *smaller* 
> than the 1024 byte request minimum (response would be 744 bytes, see [int08h]).
> 
> If requests smaller than 1024 bytes are permitted, how small could a request be? A valid 
> Roughtime request *without* a PAD field would be 72 bytes long: 
> 
>      4 bytes number of fields 
>      4 bytes NONC tag 
>     64 bytes NONC value
>     
> Given that the *minimum* Server response size is 360 bytes [int08h], a minimal size request 
> presents a DDoS attacker with a potential 5x gain in size.  
> 
> Making the minimum response size OPTIONAL requires Roughtime server operators to decide 
> "how small is too small" and a wrong choice will create more DDoS amplifiers in the world.
> 
> Suggestion: mandate requests >= 1024 bytes

I think you may misunderstand the text in the draft. It is always wrong
for a server to send a response shorter than the request.

The minimum guaranteed IPv4 MTU is 576 bytes (RFC 791). By setting a
fixed minimum request size at 1024 bytes, we risk making it impossible
to use Roughtime over some IPv4 networks. The current text at least
makes it possible to write implementations that work on all IPv4 networks.


> ## Checking response vs. request size complicates server implementation
> 
> In Draft 02: 
> 
>     Responding to requests shorter than 1024 bytes is OPTIONAL 
>     and servers MUST NOT send responses larger than the requests 
>     they are replying to.
> 
> Roughtime servers can batch multiple requests into a single response making response 
> size a function of server load/batching parameters plus concurrent requests. Roughtime 
> Server implementations may gather or buffer client requests prior to constructing the 
> response. 
> 
> "...servers MUST NOT send responses larger than the requests..." will require implementations
> to perform additional tracking of per-request sizes and then compute the resulting response
> size *after* batch size has been determined. 
> 
> This is more complex and incurs additional processing compared to simply rejecting all 
> requests <1024 bytes.
> 
> Suggestion: mandate requests >= 1024 bytes

Again, I think you may misunderstand the text in the draft. There is no
requirement to perform any additional processing. It is completely fine
for a server to have a fixed >= 1024 byte requirement.

In my implementation, I have relaxed the minimum request size
requirement to the maximum size of a response, which is less than 1024
bytes. This increases interoperability while not increasing program
complexity.

> ## The "ROUGHTIM" packet format is redundant
> 
> The addition of the constant "ROUGHTIM" plus additional length field is redundant to 
> the Roughtime message format (which also has a length field). The reasoning for this 
> additional packet format is not clear.
> 
> Suggestion: use "bare" Roughtime messages as the packet format 

The ROUGHTIM magic number serves two purposes:
1. Synchronization on TCP streams.
2. Packet identification. It is not uncommon to receive garbage UDP
packets. A magic number makes it easy to identify and ignore such
packets. Network analyzers are also helped by magic numbers as they are
a simple way to identify the protocol used.

> ## Stick with SHA-512; eliminate use of truncated SHA-512/256 
> 
> Truncated SHA-512/256 is performed by a) compute SHA-512, then b) truncate the result. 
> The resulting computational effort of SHA-512 and SHA-512/256 is equivalent. 
> 
> The draft utilizes SHA-512/256 for its 32 byte output, as opposed to 64 bytes for SHA-512. The 
> motivation for this change is unclear as it complicates implementations which now need two 
> hashing primitives (SHA-512/256 initialization parameters are different than SHA-512) without
> demonstrable performance or security benefit. 
> 
> Suggestion: use SHA-512 throughout and remove use of SHA-512/256

Agree. SHA-512/256 and truncated SHA-512 have identical properties.
Using SHA-512/256 only protects against scenarios where an attacker
knows a SHA-512 value for some data and wishes to guess the SHA-512/256
value, or vice versa. I don't see how that could apply here. There is a
definite benefit to using a single hash function throughout.