Summary: Has a DISCUSS. Has enough positions to pass once DISCUSS positions are resolved.
There seems to be an internal inconsistency regarding the format of the data payload: at the start of Section 4 we see that "When present, the data field contains a list of identifiers or assignments in the form <<identifier>>[=<<value>>],<<identifier>>[=<<value>>],... where <<identifier>> is the ASCII name of a system or peer variable specified in RFC 5905 and <<value>> is expressed as a decimal, hexadecimal or string constant in the syntax of the C programming language", but later on we see that the Read Status reply "contains a list of binary-coded pairs <<association identifier>> <<status word>>, one for each currently defined association. The "binary-coded" (with, apparently, implicit length) seems at odds with the ASCII key/value assignment pairs. The description of the Configure(8) command as "The command data is parsed and applied as if supplied in the daemon configuration file" lacks any reference to what that format is or how one might know what format the peer expects. This does not seem sufficiently specified so as to admit interoperable implementation. The description of the Read MRU(10) command also seems underspecified. What name=value pairs affect the selection of records (and how)? Is there a particular name used with name=value pair for the returned nonce, or how is the returned nonce framed? If it's implementation-specific, say so. The only currently specified authentication scheme for these commands appears to be DES-CBC-MAC, from Appendix C of RFC 1305. (RFC 5905 suggests that existing implementations, however, use a different keyed-MD5 scheme that is similarly flawed.) As a CBC-MAC, this mechanism is subject to an extension attack, allowing an attacker to observe an existing valid packet and construct a forged packet with valid MAC by appending additional data at the end. This, therefore, fails to actually provide the key property of authentication. There are additional fundamental issues with the NTP authentication format (independently of the DES-CBC-MAC scheme), which may not quite rise to DISCUSS-level, and accordingly are listed in the COMMENT section.
Depending on how my discuss points are resolved, I may have to switch to Abstain, as I cannot support publication of a document as status other than historic that makes a normative reference to the obsolete RFC 1305 for the authenticator format/procedures. In general, many of the descriptions for particlar codepoints are single-word or single-line descriptions that do not provide enough detail to give confidence of interoperable implementation; in some cases ("kernel loop discipline status changed") they are tied to particular implementation strategies that are not universally applicable. The considerations of BCP 201 with respect to algorithm agility appear to be quite relevant, with the 96-bit authenticator format locking us into at best 64 bits of MAC, which is fairly weak for Internet use, and 32 bits of key identifier. This presents its own problems, as 32 bits is too small for a value that is unilaterally assigned by many/all participants and expected to be globally unique, but is rather large for a key identifier space that is centrally managed for use within a single administrative domain. (The currently specified DES-CBC checksum from RFC 1305 is, of course, considered insecure at this point, as with all things single-DES.) I see we defer to RFC 1305 for key management, which also defers the matter as out of scope, but the considerations of BCP 107 seem to remain relevant even in that case. This document seems to be discussing a protocol over UDP that includes its own fragmentation mechanism; would the UDP Usage Guidelines of RFC 8085 (BCP 145) not be applicable? I see that there has been substantial previous discussion about this document's intended status, with the change to Informational from Historic occurring only recently. The body text, however, still has several references to "historical record" and other "historic" things that don't seem quite right in an Informational document. In light of the protocol flaws mentioned in my Discuss section, my personal stance is that Historic is the most appropriate status, though it would also be reasonable to publish an Informational document on "the Network Time Foundation's implementation of NTP mode 6 control commands". Section 1.1 Most control functions involve sending a command and receiving a response, perhaps involving several fragments. The sender chooses a distinct, nonzero sequence number and sets the status field and "R" and "E" bits to zero. [...] It may be worth a reference to draft-gont-numeric-ids-sec-considerations (I am AD-sponsoring this document) for the generation of these sequence numbers, though maybe the actual definition of the field in Section 2 is the better place from which to make such a reference. (Also for the Associate ID, it looks like.) Section 3 I note that the order of word formats in the figure does not match the subsection order. Section 3.1 nit: the table entry for System Event Code 9 seems to be missing a word ("end"?). System Event Counter (Count): This is a four-bit integer indicating the number of system events occurring since the last time the System Event Code changed. Upon reaching 15, subsequent events with the same code are not counted. I get the impression from reading this (and similar text in subsequent sections) that the "events" in question are exactly the behavior specific to the event code in question, but it is hard to be sure when it's left implicit. Section 3.2 A peer status word is returned in the status field of a response to a read status, read variables or write variables command and appears also in the list of association identifiers and status words returned by a read status command with a zero association identifier. The Given that this is the "Peer Status Word" section, shouldn't this be "non-zero association identifier"? Peer Status (Status): This is a five-bit code indicating the status of the peer determined by the packet procedure, with bits assigned as follows: We could probably spend a few more words saying that the bit being set to 1 indicates the "true"/"yes" form of the meaning. | 6 | no reply (only used with one-shot ntpd -q) | It doesn't really seem appropriate to hardcode implementation details ("ntpd -q") here. | 13 | popcorn spike suppressed by peer clock filter register | "popcorn spike" seems to appear only in the code skeleton of RFC 5905 and is not otherwise a defined term. I do see a note in draft-ietf-ntp-ntpv4-algorithms but we don't reference that document at all; as such, this sems like jargon that's not appropriate for the protocol spec. Section 3.3 | 0 | clock operating within nominals | In the vein of my previous comment about "event" ambiguity, what event would happen to cause the counter to increment while this is the clock code? Section 4 Commands consist of the header and optional data field shown in Figure 2. When present, the data field contains a list of I think this is actually Figure 1. identifiers or assignments in the form <<identifier>>[=<<value>>],<<identifier>>[=<<value>>],... where Please clarify whether the comma is a literal comma as separator (or what separator is used between key/value pairs), and also whether any escaping might be needed if certain characters need to appear in the <<value>> portion. language. Where no ambiguity exists, the <169>sys.<170> or <169>peer.<170> prefixes can be suppressed. Whitespace (ASCII Are the <169>/<170> mangled charset conversion issues? (I'm not sure which charset, though!) Also appears later around '.'. guidelines defined in [RFC5952]. Timestamps, including reference, originate, receive and transmit values, as well as the logical clock, are represented in units of seconds and fractions, preferably in hexadecimal notation. Delay, offset, dispersion and distance values are represented in units of milliseconds and fractions, preferably in decimal notation. All other values are represented as-is, preferably in decimal notation. (side note) a bit surprising to see the mixed hex/decimal preference. Read Clock Variables (4): The command data field is empty or contains a list of identifiers separated by commas. The association identifier selects the system clock variables or peer clock variables in the same way as in the Read Variables command. The response includes the requested clock identifier and status word and the data field contains a list of clock variables and values, including the last timecode message received from the clock. Where is the format of the "timecode message received from the clock" specified? Is it a binary or ASCII encoding? Is there a list or registry for what the variables in question are? significant. Implementations should include sanity timeouts which prevent trap transmissions if the monitoring program does not renew this information after a lengthy interval. Can we give some more concrete guidance on what constitutes a "lengthy interval"? Days? Save Configuration (9): Write a snapshot of the current configuration to the file name supplied as the command data. Further, the command is refused unless a directory in which to store the resulting files has been explicitly configured by the operator. This file is still on the NTP server, right? Read ordered list (11): Retrieves an ordered list. If the command data is empty or the seven characters "ifstats" the associated statistics, status and counters for each local address are returned. If the command data is the characters "addr_restrictions" then the set of IPv4 remote address restrictions followed by the set of IPv6 remote address restrictions (access control lists) are returned. This phrasing suggests that the command data is just the "name" part, not a full "name=value" expression that was previously claimed to be the format for the data section. Section 6 I think it may be worth specifically calling out the trap mechanism as a DoS vector as well as the generic "NTP control messages as DDoS vector" discussion -- AFAICT spoofed UDP packets can register any number of trap recipients and then a storm of outgoing packets can be prompted by causing the trap condition. (If there was only one registered trap address/port active at any given time, this would be less of a concern, but that doesn't seem to be the case.) (There is also a related risk in terms of causing the NTP server to hold all the state for so many trap registrations, but the state per address seems relatively small.) We should probably talk about the situation where a given message has multiple name=value pairs for the same name, as security issues can result when the parties disagree about which is to be used. (Having this rejected entirely as a protocol error is a fine option.) There are probably some privacy considerations for some of the commands' responses, e.g., the MRU gives information about who is using the server, etc. However, this document argues that an NTP host must authenticate all control queries and not just ones that overwrite these variables. Alternatively, the host can use a whitelist to explicitly list IP addresses that are allowed to control query the clients. These What does "this document argues that" mean? Is it a MUST-level requirement? (If so, then the text in the description of the Read ordered list(11) command about "this opcode requires authentication" is redundant and should be removed.) Also, authentication does not inherently imply authorization; I think we should say something about the server's authorization policy. Also, we generally do not consider IP ACLs to be a security mechanism (absent, e.g., IPsec), even if they can still have utility as a front-line filtering option. * In the client/server mode, the client stores its local time when it sends the query to the server in its xmt peer variable. This variable is used to perform TEST2 to non-cryptographically What is TEST2/where is it documented? o The mode 6 and 7 messages are vulnerable to replay attacks [CVE-Replay]. If an attacker observes mode 6/7 packets that modify the configuration of the server in any way, the attacker can apply the same change at any time later simply by sending the packets to the server again. The use of the nonce (Request Nonce command) provides limited protection against replay attacks. We seem to only document the usage of the Nonce for the Read MRU(10) command, but the anti-replay functionality is generally useful. Additional description of actual/intended nonce usage would be beneficial. NTP best practices recommend configuring ntpd with the no-query parameter. The no-query parameter blocks access to all remote This is written in an implementation-specific manner and should be rewritten to describe the protocol behavior -- "NTP best practices" relate to the protocol, not the specific implementation. configuration of the hosts in certain scenarios. Such hosts tend to use firewalls or other middleboxes to blacklist certain queries within the network. There has been quite a bit of recent discussion on ietf@ relating to terminology choices, as relates to "blacklist"/"whitelist" and other terms. I hope that the authors' usage of the term (both here and in subsequent paragraphs) is an informed decision in light of those community discussions. Appendix A The format of the NTP Remote Facility Message header, which immediately follows the UDP header, is shown in Figure 3. Following is a description of its fields. Bit positions marked as zero are reserved and should always be transmitted as zero. (I don't see any fields marked thusly; just the one "MBZ" field that has its own description.) Implementation : The version number of the implementation that defined the request code used in this message. An implementation number of 0 is used for a Request Code supported by all versions of the NTP daemon. The value 255 is reserved for future extensions. What is "the NTP daemon"? A particular implementation? Data : A variable-sized field containing request/response data. For requests and responses, the size in octets must be greater than or equal to the product of the number of data items (Count) and the size of a data item (Size). For requests, the data area is exactly 40 I suggest saying more specifically which size it is that "must be greater [...]".
** Section 4. Can “Whitespace (ASCII) non-printing format effectors)” be specified more precisely? ** Section 6. Per “… the host can use a whitelist to explicitly list IP addresses …”, consider s/whitelist/allow-list/ ** References -- Section 1.2. Add an information reference for ntpdc -- Section 6. Add an informative reference for ntpd ** Editorial Nits -- Global. Typo. s/developement/development/ -- Section 2. Typo. s/Conains/Contains/ -- Section 2. Per “… this contains the authenticator information defined in Appendix C of RFC 1305”, please make RFC1305 a reference. -- Section 4. Typo. s/managment/management/ -- Section 6. Typo. s/atacks/attacks/ -- Appendix A. Typo. s/reponse/response/ -- Appendix A. Editorial. s/more then one packet/more than one packet/
I support Murray’s DISCUSS. With a document of this type, I am not sure if all of these type codes and bit settings should be IANA registries or not. Sec 4 references Fig. 2 but I think you mean Fig. 1. In sec 6, please replace “whitelist” with “allowlist” or other equivalent term.
Thanks for resolving the document status confusion. I found the same typos Roman did. A quick pass with a spell checker might speed things through the RFC Editor slightly.
I support Murray’s DISCUSS. To add to the discussion: It’s not clear to me why either a Historic or an Informational RFC is needed, to repeat information that is in an Obsolete RFC. It would be one thing if these were for current implementation, but the draft is clear that they’re not. Then why isn’t it sufficient that they are documented in 1305? Why does there need to be a non-Obsolete RFC that has them?
I have noticed a number of additions but also changes compared to 1305, for example regarding the code points * Operation Code * System Event Code, * Peer Selection, * Peer Event Code, * Clock Status/Clock Code. I have read: The goal of this document is to provide a current, but historic, description of the control messages as described in RFC 1305 and any additional commands implemented in NTP. Which I interpret as 1305 being the main constituant of this document, plus some additional elements of specifications. Therefore, finding new stuff compared to 1305 isn't totally surprising but still, it is not clear to me where do the additions come from, and what explains the changes compared to 1305.
Thank you for the work put into this document. First, I must admit that this is the first time I see an IETF stream informational document for the specification of a control protocol used by an obsoleted RFC 1305. This document is much easier to read than the appendix B of RFC 1305 and the author takes care to write that this spec is not mandatory to implement but I really wonder why this document exists ? Moreover the abstract says "The goal of this document is to provide a current, but historic, " so why not publishing this document as 'historic' rather than 'informal' (datatracker seems to allow this modification). Please find below a couple of non-blocking COMMENTs (and I would appreciate a reply to each of my COMMENTs) and some NITs. I hope that this helps to improve the document, Regards, -éric == COMMENTS == -- Section 1.1 -- Suggest to replace 'IP' by 'IPv4' in 'IP hosts are not required to reassemble datagrams larger than 576' + add some text that this document applies the same limitation to IPv6. -- Section 2 -- Possibly linked to my lack of understanding of the purpose of this document, but, if applicable only to NTPv3, then should the Version number clearly specified to be 3 ? -- Section 3.2 -- Suggest to add 'bit' after 'Peer Status' in the table headings to make it clear. -- Section 4 -- It will probably be useful to expand 'MRU' at first use. In the "Read ordered list (11):" it is not clear how the entries are ordered in the case of "ifstats" is it per local address ? Are IPv4 addresses before IPv6 addresses ? -- Appendix A -- Is there a reason why the mode 7 is in the appendix and not in the main body ? == NITS == -- Section 2 -- s/Conains/Contains/ -- Section 4 -- Should there be a comma in 'seven characters "ifstats" the associated' before 'the associated' ?