Re: [Lsr] AD Review of draft-ietf-isis-reverse-metric-11

[Alvaro Retana <[e-mail]>]

On September 27, 2018 at 6:36:29 PM, Naiming Shen (naiming) (
[e-mail]) wrote:

Naiming:

Hi!

Some answers below.  Please take a look and publish an update.

Thanks!

Alvaro.

...

Note that I read -11, but -12 was published in the interim -- so I'm
putting this comment up here.  The only change in -12 is the addition (in
the IANA Considerations section) of "a new registry for sub-TLVs of the
Reverse Metric TLV".  Why do we need this new registry?  The description
(in §2) of the use of this sub-TLV already references rfc5305 (where a TE
Default Metric sub-TLV is already defined), so it seemed somewhat natural
to me to simply reuse that sub-TLV here.  From the discussion on the list,
I understand the intent to "future proof", even if no other applications
come to mind.  If that is the path we want to go, then we'll need a
complete description of the new sub-TLV as well. [Some of my comments
bellow assume the existing sub-TLV from rfc5305.]


<NS> With the email exchange from you and Les, I assume I keep the same
as in the version 12 on this sub-TLV registry.

Yes.

However, I need you to add a description of the sub-TLV in this document.
Right now there is a reference to rfc5305, but as Les explained, even
though these sub-TLVs look the same, they are really different.  When doing
so, the references to rfc5305 (for the sub-TLV) would not be needed.


<NS> ok, modified. see diff.

The first MUST (below) sounds as if there must always be one of these
sub-TLVs.  However, if other sub-TLVs are defined in the future, and this
one is not needed, the MUST would make it mandatory.

OLD>

"This sub-TLV is optional, and MUST appear once at most in the Reverse
Metric TLV, otherwise the entire Reverse Metric TLV MUST be ignore on the
receive."

NEW>

This sub-TLV is optional, if it appears more than once then the entire
Reverse Metric TLV MUST be ignored.


...


178   The Reverse Metric TLV is optional.  The Reverse Metric TLV may be
179   present in any IS-IS Hello PDU.  A sender MUST only transmit a single
180   Reverse Metric TLV in a IS-IS Hello PDU.  If a received IS-IS Hello
181   PDU contains more than one Reverse Metric TLV, an implementation
182   SHOULD ignore all the Reverse Metric TLVs and treat it as an error
183   condition.

[nit] The first two sentences sound redundant to me.


<NS> OK.


[major] The text above is not specific about which PDUs can include the
Reverse Metric TLV.  The text does say that it is optional and that it may
be in an IIH once...but it doesn't say anything about other PDUs.  The IANA
Considerations section contains the attributes to be included in the
registry, but those are not Normative.


<NS> Added other PDU MUST ignore it.

I saw the new text.  Thanks for being explicit about the TLV being used in
an IIH.

Les also pointed to the following in ISO 10589: “TLVs received in a
non-permitted PDU MUST be ignored.”  That means that the last sentence in
the new text ("If an IS-IS node received of IS-IS Reverse Metric TLV in the
PDU other than the IS-IS Hello PDU, this TLV MUST be ignored.”) is
redundant.


<NS> ok, removed:-) but it used to be “.. may be optional present in …”,
changed to ".. MAY be optional present in …”, the formally define into
Hello PDU.

The text now reads:

   The Reverse Metric TLV MAY be optionally present in any IS-IS Hello

   PDU. A sender MUST only transmit a single Reverse Metric TLV in a

   IS-IS Hello PDU. If a received IS-IS Hello PDU contains more than

   one Reverse Metric TLV, an implementation MUST ignore all the Reverse

   Metric TLVs.


MAY and optional are the same thing, so the first sentence is redundant.

s/MAY be optionally/MAY



...



...

 418     4.  Security Considerations

420       The enhancement in this document makes it possible for one IS-IS
421       router to manipulate the IS-IS default metric and, optionally,
422       Traffic Engineering parameters of adjacent IS-IS neighbors.
Although
423       IS-IS routers within a single Autonomous System nearly always are
424       under the control of a single administrative authority, it is
highly
425       RECOMMENDED that operators configure authentication of IS-IS PDUs
to
426       mitigate use of the Reverse Metric TLV as a potential attack
vector,
427       particularly on multi-access LANs.

[major] Authentication doesn't prevent a subverted router from using the
reverse metric.


<NS> Right. But authentication is all IS-IS has.

:-(


[minor] I would love to see more text on what the threat really is.  I
think that includes being able to divert traffic, sent traffic over less
preferred paths, etc. -- in short, this extension can have a significant
effect on routing decisions.


<NS> Right. But this is no different from any other IS-IS operations, such
as changing local interface metric. reverse-metric
is no worse than those. all of them can change the routing behavior in the
network.

True.  However, this is a new mechanism being defined in this document.  At
least recognizing the potential effect and that it “is no worse” is
important.

Note that the difference between, for example, locally changing the metric
and the reverse-metric is that the local change is like shooting yourself
on the foot.  The reverse-metric allows someone else to influence you.
That’s the reason form my comment below:


<NS> Make sense, but I think one of the main reason of this draft is to
simplify
the operation of maintenance. If an operator (within the same local domain)
not
only needs to raise a ‘reverse metric’ on the local device, but also to
hunt down
all the other devices on the same LAN and to enable this feature one by
one, that
would defeats the purpose of this important usage.

Even though this is sent over to neighbor to act, but most provider network
those
devices are under the same admin domain.

Yeah…that’s the ongoing discussion with the Sec ADs/SecDir: what security
guarantees are provided by the “same admin domain”?

Even in the same domain, a router could be subverted and cause problems.
Yes, if that happens then maybe the operators have bigger problems — which
probably means that the “same domain” assumptions are not met.

In any case, we can wait for the SecDir/Sec AD to bring up the point before
adding text...



 [major] Is there anything (besides authenticating) that can be done to
mitigate the threat?  The text talks about local configuration having the
ability to override a reverse metric -- but it seems like the intent is for
the default to be "accept the reverse metric"..  Making the default be
"don't accept reverse metrics" (i.e. having to explicitly configure the
willingness to accept the new TLV) would help by only allowing the reverse
metric where the operator knows it wants it.  Was there any discussion
about this in the WG?


<NS> I don’t recall there is such a discussion. This document does not
specify which is the default
behavior. An implemetation can use either opt-in or opt-out, which does not
break the
inter-operability.

It would be great if the document took a stance on that.

<NS> although I would certainly want this mechanism to be enabled by
default,
but I don’t want to express this in this document since there may generate
some
security concern.

added in the “operational consideration” section that:

It is RECOMMENDED that the network operators disable the capability
when this Reverse Metric feature or mechanism is not being used in
the network where in the case an IS-IS implementation has this
mechanism enabled by default.

to see if you are ok with that.

NEW>

If an implementation enables this mechanism by default, it is RECOMMENDED
that it be disabled by the operators when not explicitly using it.