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

From: Stuart Stock <[email-masked]>
Date: Sun, 14 Jun 2020 19:34:58 +0000

# Comments on draft-ietf-ntp-roughtime-02

Author: Stuart Stock
Date: 2020-06-14

## Introduction

I am the original creator and current maintainer of two Roughtime implementations:

  * Roughenough (Rust) https://github.com/int08h/roughenough
  * Nearenough (Java) https://github.com/int08h/nearenough

Have written deep-dive articles about the Roughtime protocol: 

  * https://int08h.com/post/to-catch-a-lying-timeserver/
  * https://int08h.com/post/roughtime-message-anatomy/
  
And operate the longest running publicly accessible (non-Googler) Roughtime server 
on the internet

  * roughtime.int08h.com:2002
  
I offer my comments on draft-ietf-ntp-roughtime-02 as someone well versed in the Roughtime 
protocol and intimately familiar with its implementation and trade-offs. 

These comments are also committed to my Github at
https://github.com/int08h/roughenough/blob/master/doc/rfc-commentary.md

## Keep the "rough" in "Roughtime"

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 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

## 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

## 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

## 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

## 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 

## 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

## References 

* [AWS-smear]     https://aws.amazon.com/blogs/aws/look-before-you-leap-the-coming-leap-second-and-aws/
* [CF-DDoS]       https://www.cloudflare.com/learning/ddos/ntp-amplification-ddos-attack/
* [CWE-190]       https://cwe.mitre.org/data/definitions/190.html
* [CWE-TOP25]     https://cwe.mitre.org/top25/archive/2019/2019_cwe_top25.html
* [draft02]       https://tools.ietf.org/html/draft-ietf-ntp-roughtime-02
* [Google-smear]  https://developers.google.com/time/smear
* [int08h]        https://int08h.com/post/to-catch-a-lying-timeserver/#keeping-response-sizes-compact
* [roughtime]     https://roughtime.googlesource.com/roughtime

---

From: Marcus Dansarie <[email-masked]>
Date: Mon, 15 Jun 2020 00:18:31 +0200

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

> ## 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".

> 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.

> 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.

> 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.

> 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.

> 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.

> 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.

> 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.