summaryrefslogtreecommitdiff
path: root/doc/rfc-commentary.md
blob: e4d1a1568c77282341f333d231e47413b4707001 (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
# 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.

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

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 the 
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 once the response *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 message format (which also has a length field). The value 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 and it complicates implementations
which now need two hashing primitives (SHA-512/256 initialization is different than SHA-512).

Suggestion: use SHA-512 throughout and drop any 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