[Ntp] Antw: [EXT] Benjamin Kaduk's Discuss on draft-ietf-ntp-mode-6-cmds-09: (with DISCUSS and COMMENT)

[Ulrich Windl <[e-mail]>]

See my comments inline... 

>>> Benjamin Kaduk via Datatracker <[e-mail]> schrieb am 26.08.2020
um
23:29 in Nachricht <[e-mail]>:
> Benjamin Kaduk has entered the following ballot position for
> draft‑ietf‑ntp‑mode‑6‑cmds‑09: Discuss
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss‑criteria.html 
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft‑ietf‑ntp‑mode‑6‑cmds/ 
> 
> 
> 
> ‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑
> DISCUSS:
> ‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑
> 
> 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.

There a more minor issues:
First one could assume that all identifier-value pairs are in one long line,
but actually there are line breaks in the output.
The other thing is: I the value itself contains a comma, how is that encoded?
It could be that current implementations just don't use commas in values...

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

I agree.

> 
> 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.
> 
> 
> ‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑
> COMMENT:
> ‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑‑
> 
> 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 was woindering on this as well. The implementation does that, but probably
it would make sense to incremewnt that number on every new event, so there's a
change 1 out of 16 to detect when new events have arrived since last polling.

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

Naturally the format depends on the clock being used, e.g.:
timecode="\x0227.08.20; 4; 08:48:57;    S   \x03".
In the example binary characters are encoded as ASCII. I don't know if that
escape mechanism is specified anywhere.

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

The file is written on the server, not on the client (if that was your
question).

> 
>    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 [...]".
> 
> 
> 
> _______________________________________________
> ntp mailing list
> [e-mail] 
> https://www.ietf.org/mailman/listinfo/ntp