Subject: Re: [manet] AD review of draft-ietf-manet-nhdp-olsrv2-sec

From: Ulrich Herberg <[email-masked]>
Date: Tue, 2 Jul 2013 09:10:12 -0700

Adrian,

thanks for your review. We have submitted a new revision that
hopefully addresses your review.

Best regards
Ulrich

On Mon, Jun 17, 2013 at 5:42 AM, Dearlove, Christopher (UK)
<[email-masked]> wrote:
> Some personal comments marked by >>> below.
>
> --
> Christopher Dearlove
> Senior Principal Engineer, Communications Group
> Communications, Networks and Image Analysis Capability
> BAE Systems Advanced Technology Centre
> West Hanningfield Road, Great Baddow, Chelmsford, CM2 8HN, UK
> Tel: +44 1245 242194 |  Fax: +44 1245 242124
> [email-masked] | http://www.baesystems.com
>
> BAE Systems (Operations) Limited
> Registered Office: Warwick House, PO Box 87, Farnborough Aerospace Centre, Farnborough, Hants, GU14 6YU, UK
> Registered in England & Wales No: 1996687
>
>
> -----Original Message-----
> From: Adrian Farrel [mailto:[email-masked]]
> Sent: 16 June 2013 23:42
> To: [email-masked]
> Cc: [email-masked]
> Subject: AD review of draft-ietf-manet-nhdp-olsrv2-sec
>
> ----------------------! WARNING ! ----------------------
> This message originates from outside our organisation,
> either from an external partner or from the internet.
> Keep this in mind if you answer this message.
> Follow the 'Report Suspicious Emails' link on IT matters
> for instructions on reporting suspicious email messages.
> --------------------------------------------------------
>
> Hi Authors,
>
> Thanks for this small but important addition to NHDP and OLSRv2. I
> found the document pretty clear and have only one technical issue with
> it.  The bunch of editorial comments below can be taken under
> advisement, but I think that fixing them will make the document more
> readable and will ease its passage through the next stages.
>
> If you can turn around a new revision (or tell me that none of my
> suggestions needs to be adopted) then I can kick of the IETF last call.
>
> Thanks for the work,
> Adrian
>
> ---
>
> Technical issue...
>
> I understand that this document specifies mandatory rules for NHDP (and
> OLSRv2) and your use of "updates" is correct.  However, there are
> obviously existing implementations of NHDP that are deployed.
>
> I am searching for the backward compatibility description. An existing
> implementation of NHDP may now receive messages containing the ICV or
> timestamp TLVs.  But these will not be used by the legacy implementation
> because they are unknown TLVs.
>
> How will they be processed by the legacy implementation? I think this
> needs just a single statement and a pointer to the correct rule in 6130.
>
>>>> The legacy implementation should be straightforward, nothing in NHDP discusses other TLVs. In fact that's the whole point of the TLV design, if you don't understand it, ignore it. I think this may already be said (there is a discussion of extensibility) but haven't checked.
>
>>>> There is one nit, that there is permission to refuse messages "for any other reason". That can always be abused if you want to create an implementation that is technically compliant, but useless. Failing because you have received a TLV you don't understand would be such a case. (Failing because you receive an expected TLV with an unexpected value is a subtlety that the authors are actually discussing, but is not relevant here.)
>
> How will the way that a legacy implementation behaves change the
> perception of security that the sender has?  This obviously depends on
> the answer to the previous question, and can hopefully be stated very
> briefly.
>
>>>> This is actually a bit of a red herring, in that such implementations are irrelevant, they can do what they like, communicate with each other, be friendly or unfriendly, whatever. They are just all ignored. (As far as routing is concerned. There are issues above and below the network layer, but we aren't solving them in a network layer protocol.)
>
>>>> (That these aren't real issues in practice is not to say that a sentence or two about them wouldn't go amiss. It should make the sentence or two easy. Usually the hard thing is to work out where best to put them.)
>
> ---
>
> I think that this document will also update [OLSRv2] when it is
> published as an RFC (even if that is simultaneous). Therefore, I
> recommend to top the document with...
>
> [RFC Editor note: Please replace "xxxx" throughout this document with
> the RFC number assigned to [OLSRv2], and remove this note.]
>
> Noting that the citation will be fixed by the RFC Editor in any case.
>
> ---
>
> The line in the document header should read
> "Updates: 6130, xxxx (if approved)"
>
> ---
>
> The Abstract should mention
> "This document updates RFC 6130 and RFC xxxx."
>
>>>> All the above good points. We've always discussed it in those terms, just failed to follow through.
>
> Additionally, I found the Abstract quite dense. Can I suggest some
> re-wording.
>
> OLD
>    This document specifies integrity and replay protection for required
>    implementation in the MANET Neighborhood Discovery Protocol (NHDP)
>    and the Optimized Link State Routing Protocol version 2 (OLSRv2).
>    This document specifies how an included integrity check value (ICV)
>    and a timestamp TLV, defined in RFC6622bis, are used by NHDP and
>    OLSRv2 for countering a number of security threats.  The ICV TLV uses
>    a SHA-256 based HMAC and one or more shared secret keys.  The
>    timestamp TLV is based on POSIX time, and assumes that the clocks in
>    all routers in the network can be synchronized with sufficient
>    precision.  The mechanism in this specification can also be used for
>    other MANET protocols using RFC5444.
> NEW
>    This document specifies integrity and replay protection for the MANET
>    Neighborhood Discovery Protocol (NHDP) and the Optimized Link State
>    Routing Protocol version 2 (OLSRv2).  This protection is achieved by
>    using the Integrity Check value (ICV) and the Timestamp TLV.
>
>>>> I don't like the last line's use of the definite article and TLV in one case but not the other. So some massaging needed.
>
>    The mechanism in this specification can also be used for protocols
>    that use the generalized packet/message format described in RFC 5444.
>
>>>> Protocols wants an "other" before it.
>
>    This document updates RFC 6130 and RFC xxxx by mandating the
>    implementation of integrity and replay protection in NHDP and OLSRv2.
> END
>
> I think this has thrown away what may be to some the key information of HMAC-SHA256 and POSIX timestamp, those being options for those TLVs. So while I agree with the separation of the statements about status, I think we may want to put something back.
>
> ---
>
> The Introduction should also say something like:
>
>    This document updates [RFC6130] and [OLSRv2] by mandating the
>    implementation of integrity and replay protection in NHDP and OLSRv2.
>
>>>> Goes with earlier.
>
> ---
>
> In the Introduction, I had to read the following paragraph several times
> and look at the figure in order to work out what you meant. At that point
> it was obvious, but can I suggest re-wording:
>
> OLD
>    The mechanism specified in this document exists between NHDP's and
>    OLSRv2's message processing/generation and the [RFC5444] packet
>    parsing/generation, as illustrated in Figure 1.
> NEW
>    The mechanism specified in this document is placed in the packet-
>    processing flow as indicated in Figure 1.
>
>>>> Not quite. It's the packet/message processing flow, in particular at the point where inserted, it's the message processing flow (but is packet elsewhere).
>
>  It exists between the
>    packet parsing/generation function of [RFC5444], and the message
>    processing/generation function of NHDP and OSLRv2.
> END
>
> ---
>
> Section 3
>
> I found the following hard:
>    o  Specifies an association of ICVs with messages, and for using
>       missing or invalid ICVs as such an additional reason for rejecting
>       a message as "badly formed and therefore invalid for processing".
>
> How about:
>    o  Specifies an association of ICVs with protocol messages, and
>       specifies how to use a missing or invalid ICV as an reason to
>       reject a message as "badly formed and therefore invalid for
>       processing".
>
>>>> I can live with that.
>
> ---
>
> Section 3
>
>    o  Specifies the implementation of an ICV Message TLV, defined in
>       [RFC6622bis], using a SHA-256 based HMAC applied to the
>       appropriate message contents (and for HELLO messages also
>       including the IP datagram source address).  Deployments of
>       [RFC6130] and [OLSRv2] using this framework should use an HMAC/
>       SHA-256 ICV TLV, but may use different algorithms if more
>       appropriate in a deployment.  An implementation may also use more
>       than one ICV TLV in a message, as long as they each use a
>       different algorithm to calculate the ICV.
>
> There is a "should" and two "may"s in this text. Given the use of 2119
> language higher up the section and in the next bullet, I wondered
> whether you wanted this in 2119-speak as well.
>
>>>> I'm not going to work out a definite answer here, except to agree that within this section we should be consistent. We usually use SHOULD etc. in normative parts (and non-normative appendices on the understanding that if you adopt them, then they kick in) but should etc. in early descriptive parts (like sections 1 and 4). Section 3 hovers on borderline, I'm not sure whuich way we usually go with it, but should be consistent.
>
> ---
>
> I think the RFC Editor would ask you for help determining the
> consistency of case in "timestamp TLV" and "TIMESTAMP TLV".  Can you
> check you are happy with what you have.
>
>>>> We can do that. We've usually tried, but sometimes edits may be made after last time you (we) remembered to check, so I can't at this moment guarantee all is correct, but it can be checked.
>
> ---
>
> The last sentence of Section 4 says:
>
>    This framework specifies that a
>    message MUST be rejected if the ICV Message TLV is absent, or its
>    value cannot be verified.
>
> This is equivalent to saying that a sender MUST include an ICV Message
> TLV in every message it sends. Initially, it seemed to me that this
> contradicted the statements about optional use in deployments: e.g.
> (from the introduction),
>
>    A deployment may choose
>    to use no such security mechanisms, but this is not recommended.
>
>
> So I think that what you are saying is actually:
>
>    This framework specifies that an implementation conforming to this
>    specification and with security enabled, MUST reject a received
>    message if the ICV Message TLV is absent, or its value cannot be
>    verified.
>
>>>> My view (and whether the text reflects that is a question) is that we have a framework. The rules say you MUST implement it, but then MAY use it. IF you use THEN there are various MUST/SHOULD/MAY rules. If you don't use then none of them apply.
>
> ---
>
> Section 5
>
> When you say "silently discarded" do you intend to preclude raising an
> alert or incrementing a count? It seems likely that over-age messages
> are indicative of something (like a replay attack, or an echo, or long
> delays, or a misconfigured constant) and some log of the fact might be
> useful. The usual way to handle this is "An implementation MAY log or
> count such events, but SHOULD be careful to avoid logging storms caused
> by a replay attack."
>
>>>> Silently discarded is a term used already in the other specifications, so nothing new here. I think it applies within the protocol framework - so no update of protocol state, messages sent etc. Outside the protocol (e.g. logging) is outside scope, so you can do what you like.
>
> ---
>
> Section 5
>
>    However it should also be noted that, in
>    the ideal case, the parameters SHOULD be significantly less than
>    those bounds.
>
> Some smart-arse on the IESG is going to ask what "significantly" means.
> Is there any way you can quantify this: e.g., 27.5%
>
>>>> I think the (obviously humorous) 27.5% illustrates exactly the problem. Yes we've has people ask this before,. but frankly they may be demonstrating a lack of smarts. There is no hard and fast rule, there is only ever a tradeoff, and it's false precision to actually say even " a quarter". Now because IESG people will do what they will, and one has to accommodate them, we have before now fudged a bit to hide this. No be fair, there is one justification for saying e.g. "a quarter", so that people at least are guided in the direction of something not entirely insane (like say a millionth). However at this time I'm not sure what "a quarter" should actually be.
>
> ---
>
> Section 7
>
> Should this section also note that routers should be able to be
> configured to use (or not use) the mechanisms described in this
> document, and that implementations should make use of these mechanisms
> the default?
>
>>>> Not sure about that. I don't thing default is the answer.  Given that things won't work with a mixture, we are in the area of administrative configuration. And as we need a key, and there is no default key (a bit of an oxymoron) I don't think saying on by default works. Off by default would work, but is dangerous. So I think I'd stick with that default is not relevant, it must be administratively configured.
>
> ---
>
> Section 9.1.2
>
> Will the readership know what is meant by Link Spoofing or should you
> give a reference?
>
>>>> Actually I think I raised this one, as it's not a term I use. Ulrich had an answer at the time, so I will defer to him.
>
>
> ********************************************************************
> This email and any attachments are confidential to the intended
> recipient and may also be privileged. If you are not the intended
> recipient please delete it from your system and notify the sender.
> You should not copy it or use it for any purpose nor disclose or
> distribute its contents to any other person.
> ********************************************************************
>