Re: [DNSOP] Review of draft-dupont-dnsop-rfc2845bis-00.txt

[Francis Dupont <[e-mail]>]

 In your previous mail you wrote:

>  After a reading, I felt that this document needs the following:
>  
>  * Editing for clarity of sentences

=> I agree until the text comes from original RFCs (i.e. you are
17 year too late) or can only be clarified at the margin.

>  * Addressing insufficient protocol specification

=> these should be fixed if they have an impact on security
(a priori we already fixed them but perhaps we missed a few?)

>  Review follows that suggests changes. Some are nitpicking, but changes
>  are needed to be pedantically correct.
>  
>  >    This protocol allows for transaction level authentication using
>  >    shared secrets and one way hashing.  It can be used to authenticate
>  
>  I'd start as "This document describes a protocol for transaction level
>  authentication...".

=> 17y

>  >    shared secrets and one way hashing.  It can be used to authenticate
>  >    dynamic updates as coming from an approved client, or to authenticate
>  >    responses as coming from an approved recursive name server.
>  
>  s/recursive name server/name server/

=> 17y but removing extra/superfluous word is good: adopted

>  >    No provision has been made here for distributing the shared secrets:
>  >    it is expected that a network administrator will statically configure
>  >    name servers and clients using some out of band mechanism such as
>  >    sneaker-net until a secure automated mechanism for key distribution
>  >    is available.
>  
>  This paragraph may be misconstrued to imply that such an automated
>  mechanism is forthcoming. I'd leave it at just this:
>  
>  "This document does not describe how to distribute the shared secrets.
>  It is expected that a network administrator will statically configure
>  name servers and clients using some out of band mechanism."

=> adopted

>  >    This document includes revised original TSIG specifications
>  >    (RFC2845) and the extension for HMAC-SHA (RFC4635).
>  
>  s/and the extension/and its extension/

=> adopted

>  > 1.  Introduction
>  > 
>  >    In 2017, security problems in two nameservers strictly following
>  >    [RFC2845] and [RFC4635] (i.e., TSIG and HMAC-SHA extension)
>  
>  s/TSIG and HMAC-SHA extension/TSIG and its HMAC-SHA extension/

=> adopted

>  >    This document specifies use of a message authentication code (MAC),
>  >    either HMAC-MD5 or HMAC-SHA (keyed hash functions), to provide an
>  >    efficient means of point-to-point authentication and integrity
>  >    checking for transactions.
>  
>  I'd avoid listing the HMACs here and instead refer to the section below
>  that lists them.

=> I disagree: it is not a good idea to have forward references in
the introduction.

>  >    The second area where the secret key based MACs specified in this
>  >    document can be used is to authenticate DNS update requests as well
>  >    as transaction responses, providing a lightweight alternative to the
>  >    protocol described by [RFC3007].
>  
>  It isn't clear how this is different from "first area". DNS updates are
>  also transactions.

=> 17y

>  >    A further use of this mechanism is to protect zone transfers.  In
>  >    this case the data covered would be the whole zone transfer including
>  >    any glue records sent.  The protocol described by DNSSEC does not
>  >    protect glue records and unsigned records unless SIG(0) (transaction
>  >    signature) is used.
>  
>  I'd avoid including this whole sentence about DNSSEC.

=> 17y

>  >    The authentication mechanism proposed in this document uses shared
>  >    secret keys to establish a trust relationship between two entities.
>  
>  I'd s/shared secret keys/pre-shared secret keys/

=> I don't think pre-shared is really clearer.

>  >    Such keys must be protected in a fashion similar to private keys,
>  
>  s/fashion/manner/

=> 17y

>  >    Note that use of TSIG presumes prior agreement between the resolver
>  >    and server involved as to the algorithm and key to be used.
>  
>  Again this document seems to tie itself to the resolver case only.
>  
>  s/between the resolver and server/between the client and server/

=> I disagree and BTW 17y too.

>  >    Since the publication of first version of this document ([RFC2845]) a
>  >    mechanism based on asymmetric signatures using the SIG RR was
>  >    specified (SIG(0) [RFC2931]) when this document uses symmetric
>  >    authentication codes calculated by HMAC [RFC2104] using strong hash
>  >    functions.
>  
>  s/when this document/whereas this document/

+> adopted (I assume your English is better than mine :-)

>  > 4.1.  TSIG RR Type
>  > 
>  >    To provide secret key authentication, we use a new RR type whose
>  >    mnemonic is TSIG and whose type code is 250.  TSIG is a meta-RR and
>  >    MUST NOT be cached.  TSIG RRs are used for authentication between DNS
>  >    entities that have established a shared secret key.  TSIG RRs are
>  >    dynamically computed to cover a particular DNS transaction and are
>  >    not DNS RRs in the usual sense.
>  
>  The last sentence doesn't appear to be particularly accurate.
>  TSIG RRs are dynamically computed to cover specific DNS *messages* or
>  used possibly sparingly over a message sequence such as with zone
>  transfers (in this latter case, more than a single TSIG RR may be used).
>  
>  I'd rewrite it as: "TSIG RRs are dynamically computed to cover DNS
>  transactions and are not DNS RRs in the usual sense."

=> it was the original text so a priori clear enough...
I request a second opinion.

>  >    CLASS ANY
>  > 
>  >    TTL   0
>  
>  Please include text that TTL MUST be set to 0, class MUST be set to ANY.

=> 17y

>  >    The Algorithm Name field identifies the TSIG algorithm name in the
>  >    domain name syntax.
>  
>  Please clarify that this field needs to be in RFC 1035 DNS name wire
>  format without name compression. Also for clarity I'd first explicitly
>  specify that unlike with other algorithm fields (represented by
>  integers) in DNS, the TSIG algorithm identifier is represented as a DNS
>  name.

=> 17y

>  > 4.3.1.8.  The Other Data Field
>  > 
>  >    The Other Data field is empty unless Error == BADTIME.
>  
>  What is in the Other Data field if Error is equal to BADTIME? A person
>  reading this document sequentially will not understand this. Please
>  introduce what "Other Data" is useful for.

=> 17y

>  Is it an error to use Other Data when Error != BADTIME?

=> no and this has not changed since original RFC 2845.

>  > 
>  > 4.4.  Example
>  > 
>  >    NAME  HOST.EXAMPLE.
>  
>  Even this example does not follow the "should" recommendation under
>  section 4.3 above.

=> I agree but again you are 17 year too late...

>  >                     Algorithm Name SAMPLE-ALG.EXAMPLE.
>  
>  Please use a valid algorithm in example.

=> 17y

>  >    Once the outgoing message has been constructed, the keyed message
>  >    digest operation can be performed.
>  
>  I'd replace this with "Once the outgoing message has been constructed,
>  the HMAC computation can be performed."

=> 17y but it is a simplification: adopted

>  > The resulting message digest will
>  
>  I'd use s/message digest/MAC/ for clarity (even though the HMAC
>  operation outputs a message digest of some data).

=> adopted with the previous one.

>  >    then be stored in a TSIG which is appended to the additional data
>  >    section (the ARCOUNT is incremented to reflect this).
>  
>  When is it incremented? Before MAC computation or after? Please specify.

=> then means after

>  >    If an incoming message contains a TSIG record, it MUST be the last
>  >    record in the additional section.  Multiple TSIG records are not
>  >    allowed.  If a TSIG record is present in any other position, the
>  >    packet is dropped and a response with RCODE 1 (FORMERR) MUST be
>  >    returned.  Upon receipt of a message with a correctly placed TSIG RR,
>  >    the TSIG RR is copied to a safe location, removed from the DNS
>  >    Message, and decremented out of the DNS message header's ARCOUNT.  At
>  >    this point the keyed message digest operation is performed: until
>  
>  Here again, I'd replace keyed message digest with MAC computation for
>  consistency.

=> adopted (for consistency)

>  >    this operation concludes that the signature is valid, the signature
>  >    MUST be considered to be invalid.  If the algorithm name or key name
>  >    is unknown to the recipient, or if the message digests do not match,
>  >    the whole DNS message MUST be discarded.
>  
>  Perhaps s/discarded/rejected/ is a better wording here.

=> 17y

>  >    If the message is a query, a response with RCODE 9 (NOTAUTH) MUST
>  >    be sent back to the originator with TSIG ERROR 17 (BADKEY) or TSIG
>  >    ERROR 16 (BADSIG).  If no key is available to sign this message it
>  >    MUST be sent unsigned (MAC size == 0 and empty MAC).  A message to
>  >    the system operations log SHOULD be generated, to warn the
>  >    operations staff of a possible security incident in progress.  Care
>  >    should be taken to ensure that logging of this type of event does
>  >    not open the system to a denial of service attack.
>  
>  This passage assumes that something is wrong with the message. It should
>  be separated into a different paragraph corresponding to what to do when
>  something is wrong with the message. Surely we must not send NOTAUTH for
>  messages that validate? The use of a single paragraph in 5.2 is
>  confusing.

=> even the original RFC has a single paragraph I agree so adopted
(I cut it after the (new) ..considered to be invalid.)

>  > 5.4.  TSIG Variables and Coverage
>  > 
>  >    When generating or verifying the contents of a TSIG record, the
>  >    following data are digested, in network byte order or wire format, as
>  >    appropriate:
>  
>  s/following data are digested/following data are passed as input to MAC
>  computation/

=> adopted (for consistency)

>  >    The RR RDLEN and RDATA MAC Length are not included in the hash since
>  >    they are not guaranteed to be knowable before the MAC is generated.
>  
>  s/hash/input to MAC computation/

=> adopted (for consistency)

>  >    For each label type, there must be a defined "Canonical wire format"
>  >    that specifies how to express a label in an unambiguous way.  For
>  >    label type 00, this is defined in [RFC4034], for label type 01, this
>  >    is defined in [RFC6891].  The use of label types other than 00 and 01
>  >    is not defined for this specification.
>  
>  What? This paragraph does not fit into this document. It seems to appear
>  in RFC 2845 too. What is this in relevance to?

=> it is where is the no compression for instance. So it is relevant!

>  > 5.4.3.  Request MAC
>  > 
>  >    When generating the MAC to be included in a response, the validated
>  >    request MAC MUST be included in the digest.  If the request MAC
>  
>  s/digest/MAC computation/

=> adopted (by consistency)

>  Please be careful when you use "digest" like this as it could be
>  misconstrued to mean "prepended/appended to the digest output".

=> 17y!

>  > 5.5.  Padding
>  > 
>  >    Digested components are fed into the hashing function as a continuous
>  >    octet stream with no interfield padding.
>  
>  Digested components? Are components digested before feeding them into
>  the MAC function?

=> yes, HMAC has two arguments and the second (i.ee. not the key)
is "digested". So if the term is not the best it is NOT incorrect.
BTW the goal is not to rewrite the whole TSIG specification, just
to fix it. So I added an explaination at the first occurrence of
"digested components".

>  > 6.1.  TSIG generation on requests
>  > 
>  >    Client performs the message digest operation and appends a TSIG
>  >    record to the additional data section and transmits the request to
>  >    the server.  The client MUST store the message digest from the
>  
>  s/message digest/MAC/

=> adopted

>  >    This allows the client to rapidly detect when the session has been
>  >    altered; at which point it can close the connection and retry.  If a
>  >    client TSIG verification fails, the client MUST close the connection.
>  >    If the client does not receive TSIG records frequently enough (as
>  >    specified above) it SHOULD assume the connection has been hijacked
>  >    and it SHOULD close the connection.  The client SHOULD treat this the
>  >    same way as they would any other interrupted transfer (although the
>  >    exact behavior is not specified).
>  
>  When reading RFC 2845, I felt this section was underspecified and this
>  document seems to replicate it.

=> of course it replicates it (:-)!

>  1. s/envelope/message/ for consistency. It is confusing to have such
>  synonyms without any explanation.

=> IMHO envelope is more accurate than message. Any second opinion?

>  2. The first paragraph is not properly written. A "DNS TCP session" can
>  be a single DNS connection. It can also accept mutiple pipelined
>  messages (separate transactions) which are not continuations.
>  
>  AIUI, the sparse signing of intermediate messages in a TSIG TCP
>  continuation is only for the zone transfer case, and not for pipelined
>  TCP messages (separate transactions). Please correct me if I'm wrong,
>  and if I'm not wrong, then please rewrite this text specifically for
>  TSIG for zone transfers.

=> I agree about only one thing: the text applies only to zone
transfer. How to fix it?

>  3. "Prior Digest (running)" is inadequately specified. A new reader of
>  this document will not understand what it is.
>  
>  Also, s/The first envelope is processed as a standard answer, and
>  subsequent messages have the following digest components/The first
>  message is processed normally generating a TSIG RR, and the TSIG RR in
>  subsequent messages is generated with the MAC computation over the
>  following components in sequence/

=> 17y (note I replaced Digest by MAC)

>  4. I'd remove the text "It is expensive to include it on every
>  envelopes".
>  
>  As an example, BIND and Knot currently includes TSIG RR in every DNS
>  message in a zone transfer, whereas NSD currently includes it
>  sparsely. I don't think any users notice it as particularly
>  expensive. It is after all just hashing below.
>  
>  Whether something is expensive in space or time also doesn't stand the
>  test of time, so I'd not include a statement that states definitely that
>  it is expensive.

=> it is a matter of taste so I apply the 17y argument here.

>  > 6.5.2.  Specifying Truncation
>  
>  Shouldn't this section be moved to appear after the time checks below
>  (6.5.4) in logical order?

=> it is not so logical because it is about how MAC is computed
and checked so should be before MAC and time checks.

>  >    1.  If "MAC size" field is greater than HMAC output length:
>  > 
>  >        This case MUST NOT be generated and, if received, MUST cause the
>  >        packet to be dropped and RCODE 1 (FORMERR) to be returned.
>  
>  s/packet/DNS message/

=> changed all "packet"s to "DNS message"s

>  >        for authentication.  The request MAC used when calculating the
>  >        TSIG MAC for a reply is the truncated request MAC.
>  
>  How about the "Prior Digest (running)" (section 6.4) ? Please specify.

=> I don't understand your concern.

>  >    SHA-1 truncated to 96 bits (12 octets) SHOULD be implemented.
>  
>  I don't know if it is outside the scope of this revision to alter this
>  table but I see two things:

=> it is outside the scope.

>  1. SHA-512 is a faster hash than SHA-256, and so it would be better to
>  make support for SHA-512 and its truncated SHA-384 mandatory.

=> note it is true only on 64 bit CPUs.

>  2. It makes little sense to support SHA-384 and SHA-224 when we already
>  support MAC truncation in the protocol itself.

=> SHA-224 is not the same than SHA-256/224 and some implementations
do not support HMAC truncation. So it is clearly a 17y case.

>  3. HMAC-MD5 is still a secure HMAC currently, but it is discouraged for
>  new applications because it is built on top of MD5 which is barred for
>  new applications (RFC 6151).
>  
>  I'm not sure if this revision constitutes a new application (as it is
>  still the backwards compatible protocol), but would it be allowed to
>  specify the use of MD5 on the path to RFC?

=> interesting question...

>  > 8.  TSIG Truncation Policy
>  > 
>  >    Use of TSIG is by mutual agreement between a resolver and server.
>  
>  Please remove this limitation of TSIG being between resolver and server
>  everywhere in the document.

=> I don't understand: in DNS transactions the initiator is named resolver
and the responder the server. Can you exhibit another case? Or
do you simply want to use more general client and server?

>  >    A name server usually runs privileged, which means its configuration
>  >    data need not be visible to all users of the host.  For this reason,
>  >    a host that implements transaction-based authentication should
>  >    probably be configured with a "stub resolver" and a local caching and
>  >    forwarding name server.  This presents a special problem for
>  >    [RFC2136] which otherwise depends on clients to communicate only with
>  >    a zone's authoritative name servers.
>  
>  This seems use-case specific.

=> 17y

>  >    SHOULD be at least as long as the keyed message digest, i.e., 16
>  >    bytes for HMAC-MD5 or 20 bytes for HMAC-SHA1.
>  
>  I'd remove this example from here and add a field for minimum secret
>  length to the table of supported algorithms above.

=> I am against to remove it but I'd like the idea to add it in
the table. BTW the SHOULD is a change from original RFCs.

>  > 11.  Security Considerations
>  > 
>  >    The approach specified here is computationally much less expensive
>  >    than the signatures specified in DNSSEC.  As long as the shared
>  >    secret key is not compromised, strong authentication is provided for
>  >    the last hop from a local name server to the user resolver.
>  
>  This is again use-case specific. I'd replace this with "is provided
>  between client and server."

=> another case of client/server.

>  >    Secret keys should be changed periodically.
>  
>  How periodically? I suggest referring to RFC 2104 section 3 rather than
>  recommending best practice here.

=> RFC 2104 is referenced for truncation so we can also reference it
for secret key management in general, can't we?

>  >    A fudge value that is too large may leave the server open to replay
>  >    attacks.  A fudge value that is too small may cause failures if
>  >    machines are not time synchronized or there are unexpected network
>  >    delays.  The recommended value in most situation is 300 seconds.
>  
>  I have felt that 300s (5 minutes) is too long to make up for clock skew
>  and message transmission delays. It becomes possible for a
>  man-in-the-middle to replay traffic (such as DNS UPDATEs) during this
>  opportunity window.

=> we all know this replay protection is very weak. If you have a
note to add here please propose its text.

>  It appears that the fudge time is not configurable at least in BIND
>  (without patching the source code).

=> does this change really something (:-)?

>  >    Significant progress has been made recently in cryptanalysis of hash
>  >    functions of the types used here, all of which ultimately derive from
>  >    the design of MD4.
>  
>  After SHA-3 (though it is not yet in the table of TSIG algorithms as
>  HMAC currently), saying that all hash functions derive from the design
>  of MD4 would be incorrect, so I'd remove the last part of the sentence
>  above for future proofing.

=> there is a reference to SHA-3 (which is not in the table because
it is out of scope and BTW HMAC-SHA3 is broken in OpenSSL so nobody
wants to add it...). And the text does not say "all hash functions"
but "hash functions of the types used here" so is IMHO correct.

>  >    While the results so far should not effect HMAC,
>  >    the stronger SHA-1 and SHA-256 algorithms are being made mandatory
>  >    due to caution.  Note that today SHA-3 [FIPS202] is available as an
>  >    alternative to SHA-2 using a very different design.
>  
>  Available as TSIG algorithm? Unless it is added to the table above, I
>  suggest removing this reference.

=> I simply disagree.

>  > 11.1.  Issue fixed in this document
>  > 
>  >    To bind an answer with its corresponding request the MAC of the
>  >    answer is computed using the MAC request.  Unfortunately original
>  >    specifications [RFC2845] failed to clearly require the MAC request to
>  >    be successfully validated.
>  
>  I suggest rewriting this:
>  
>  When signing a DNS reply message using TSIG, its MAC computation uses
>  the request message's MAC as an input to cryptographically relate the
>  reply to the request.  Unfortunately, the original TSIG specification
>  [RFC2845] failed to clearly require the request MAC to be successfully
>  validated before using it. This created a security vulnerability [add
>  reference to Synacktiv paper].

=> I don't have the reference to the paper (and I asked so I shan't
add one). Adopted without the last sentence.

>  >    This document proposes the principle that the MAC must be considered
>  >    to be invalid until it was validated.  This leads to the requirement
>  >    that only a validated request MAC is included in a signed answer.  Or
>  >    with other words when the request MAC was not validated the answer
>  >    must be unsigned with a BADKEY or BADSIG TSIG error.
>  
>  "This document makes it a requirement that the request MAC must be
>  considered to be invalid until it passes validation."

=> no, the principle with its consequence is better. Now what about
replacing "it is validated" by "it passes validation"?

>  > 12.2.  Informative References
>  
>  Please cite Synacktiv paper on vulnerability in RFC 2845 here.

=> I asked Synacktiv and put the proposed text from the answer in
the I-D. So no reference to the paper.

Thanks

[e-mail]

PS: don't include the whole I-D. And as you have access to the XML
look at the comments in it.