Re: [Idr] Review of draft-abraitis-bgp-version-capability-07

["RFC ISE (Adrian Farrel)" <[e-mail]>]

Alvaro,

Very many thanks for this review.

I won't get into the discussion of whether these are valid points for a
conflict review, but will focus instead on the technical content which is
valuable.

[snip]

> (1) You do a very good job to highlight potential threats in the
> Security Considerations section.
> But you also Normatively recommend (SHOULD) the use of encryption in
> the BGP Session.
> While sensible, there are no Standards Track documents that currently
> do the same.
>
> This recommendation, even if the draft points out that it "is not an
> IETF Standards Track document", can create confusion and potentially
> disrupt other IETF work.

At this point, I don't see that as a problem. This is a recommendation
that *if* you use this extension you should also use encryption. The fact
that no standards track documents recommend encryption means that if you
don't use this extension you are not recommended to use encryption.

Can you supply examples of the confusion or disruption? (It is certainly
not our intention to confuse or disrupt!)

> The threats that you identify are not exclusive to this extension,
> so the solution/mitigation shouldn't be either.

Not sure I follow that. The threat identified is incremental. That is, it
applies to the use of this extension. If the threat also applies to other
pieces of BGP we would hope that the IDR working group addresses them, but
we would not expect this document to address them.

If there are already solutions available that this document should use to
mitigate the threat, then obviously we would want to use them.

> This issue should be easy to -- I suggested alternative text below.

So, response to that in line.

> (2) As currently specified ("SHOULD be no greater than 64" and "it is
> RECOMMENDED to exclude the Version Capability"), the Capabilities
> Length Overflow (§3.1) handling represents a significant threat to a
> BGP session.
> The recommendations are not enough to prevent the potential
> exclusion of other more critical Capabilities.
> The use of draft-ietf-idr-ext-opt-param could reduce the risk, but may
> result in the same problem.
>
> Because the current specification may significantly affect any BGP
> session where one of the peers supports the Version Capability, I
> believe that this document would require IETF review.
>
> This issue can be addressed by requiring the exclusion of the
> Version Capability (using MUST), *and* talking about the risk in the
> Security Considerations section.

While we consulted about the length limit and the risk of impact on other
Capabilities, and arrived at 64 bytes as a reasonable low to zero risk,
being future-proof is hard. It is unclear, for example, how any new
Standards Track Capabilities would avoid this issue.

However, I would be comfortable with:
- SHOULD be no greater than 64 bytes
- MUST be left out if would constrain inclusion of any other Standards
Track capabilities

At that point, I don't believe it is a security threat.

> See more comments about this in-line in §3.1.
>
>
> Note that this message is not the IESG's Conflict Review, just my
> review of the document. I have scheduled the balloting of the
> Conflict Review for Sep/10, so we should have plenty of time to talk
> about the two issues above.
>
>
> Thanks!
>
> Alvaro.
>
>
> [Line numbers from idnits.]
>
> ...
> 10 Abstract
> ...
> 15   This BGP capability is an optional advertisement. Implementations
> 16   are not required to advertise the version nor to process received
> 17   advertisements.
>
> [nit] All BGP Capabilities are optional.  No need to include this
> paragraph.  The same text is repeated in the Introduction...and
> similar text elsewhere.

You are right that all BGP Capabilities are optional. However, I would
like this text to remain to further emphasize that this Independent Stream
work is not something that implementations are required to consider
including.

> 79  Information about the version of the routing daemon could also be
> 80  exchanged in protocols such as LLDP and CDP. However, in
> 81  containerized environments, it is very hard and not recommended to
> 82  exchange this information between background processes. Therefore,
> 83  and to help minimize operational costs, it is helpful to exchange > 
   the
> 84  routing daemon information between BGP peers directly.
>
> [nit] For completeness, it would be nice to have a reference to LLDP/CDP.

Easily done

> 96  Although this document is not an IETF Standards Track document, it
> 97  makes use of the terminology from BCP 14 in order to clearly state
> 98  the implementation behaviors.
>
> 100  Capabilities advertisements with BGP are defined in [RFC5492].
>      They
> 101  utilize the BGP Capabilities Optional Parameter that contains one
>      or
> 102  more triples <Capability Code, Capability Length, Capability
>      Value>.
> 103  This document defines a new BGP capability, the Version
>      Capability,
> 104  with Capability Code TBD and Capability Length and Capability
>      Value
> 105  as described below.
>
> 107  The inclusion of the Version Capability is OPTIONAL.  If an
> 108  implementation supports the inclusion of the capability, the
> 109  implementation MUST include a configuration switch to enable or
> 110  disable its use, and that switch MUST be off by default.
>
> 112  The Version Capability is intended principally more to
>      non-production
> 113  environments where more visibility is needed for troubleshooting
> 114  purposes.  It is NOT RECOMMENDED for use outside single
>      Autonomous
> 115  System domain or Autonomous System Confederations, except you
>      have a
> 116  topology with a number of routers each with a separate Autonomous
> 117  Number.
>
> [nit] s/except you have/except if you have

ack

> [minor] A collection of several autonomous systems is often
> characterized as being "under a common administration".  That
> description covers not only "routers each with a separate Autonomous
> Number", but also other combinations where multiple ASNs may be
> present and where exchanging this information might be useful.
>
> Suggestion>
> The Version Capability is intended for environments where more
> visibility is needed for troubleshooting purposes.  It is NOT
> RECOMMENDED for use outside a single Autonomous System, or a set of
> Autonomous Systems under a common administration.

If you think that is clearer, then it is OK for me. I believe it covers
confederations and also DCs where each router has a different ASN, so it
would be OK.

> 119  An implementation that does not recognize or support the Version
> 120  Capability but which receives one MUST respond as described in
> 121  [RFC5492] by ignoring the option.  An implementation that wishes
>      to
> 122  complain that its neighbor does not support the Version
>      Capability
> 123  MAY use the 'Unsupported Capability' Error Subcode of a
>      Notification
> 124  message as described in [RFC5492].
>
> [major] The Normative behavior is already specified in rfc5492, please
> don't re-specify the behavior here.

Yes. This document is allowed to describe the behavior documented in 5492
without using normative language. But it is better to simply say:

An implementation that does not recognize or support the Version
Capability but which receives one will handle it as described in
[RFC5492].

...which is slightly different from your
[snip]
> Suggestion>
>  An implementation that does not recognize or support the Version
>  Capability but receives one must ignore it, as described in
>  [RFC5492].

...because "will" (rather than "must") indicates that the behavior is
pre-ordained.

> 132  Capability Length
>
> [minor] For completeness, please add the following from rfc5492:

Well, for consistency with the previous point, let's not redefine anything
here. So, yours...

>  Capability Length is a one-octet unsigned binary integer that
>  contains the length of the Capability Value field in octets.

...mine
As defined in [RFC5492], the Capability Length is a one-octet unsigned
binary integer that contains the length of the Capability Value field in
octets.

> 134  The Capability Length for the Version Capability MUST be greater
> 135  than zero.  A value of zero SHALL be treated as an encoding error
> 136  and the entire triple MUST be ignored.
>
> [major] s/the entire triple MUST be ignored/the Capability MUST be ignored

I'm not sure this is right, except that maybe you are defining "the
Capability" to be the triple {type, length, value}. Perhaps the answer is
to be even more specific and say...

The Capability Length for the Version Capability MUST be greater
than zero.  A Capability Length value of zero SHALL be treated as an
encoding error and the entire Capability (triple of code, length and
value) MUST be ignored.

> 138 The Capability Length SHOULD be no greater than 64.  This is the
> 139 limit to allow other capabilities as much space as they require.
>
> [minor] Please add a forward reference to §3.1.  See more comments
> there.

Easily done

> 141  Capability Value
> 142  The Capability Value field is encoded in UTF-8 [RFC3629].  It is
> 143  unstructured data and can be formatted in any way that the
> 144  implementor decides.
>
> 146                  +--------------------------------+
> 147                  |    Version Length (1 octet)    |
> 148                  +--------------------------------+
> 149                  |      Version (variable)        |
> 150                  +--------------------------------+
>
> [minor] Just to make sure, this Figure represents the contents of the
> Capability Value, right?  If so, it seems to me that the Version
> Length is not needed because the Capability Length already
> exists...and it takes up 1 octet from the Version.

Yes. Good point. Either include the Capability Code (which looks like
needless repetition of the definition of a Capability triple), or exclude
the Capability Length (which makes the figure a bit pointless).

> 154  Version Length:
>
> 156      The number of characters in the Version
>
> [minor] If I remember correctly, UTF-8 uses variable-length character
> encoding.  Is the variable-length part of the encoding?  A reference
> to rfc3639 would be useful here.

Agree that a reference to what UTF-8 is would be wise.

> [?] BTW, is there an advantage to signal the number of characters, vs
> simply the number of octets?

Author's choice on this one.
If you wanted 'number of characters' you would need a second length field,
and that adds structure to the Capability Value.

> 158  Version:
>
> 160   The Version encoded via UTF-8
>
> UTF-8 requires some extra considerations...borrowing from rfc8203:
>
> Suggestion>
> The Version field MUST be encoded using UTF-8.  A receiving BGP
> speaker MUST NOT interpret invalid UTF-8 sequences. This field is not >
NUL terminated.

This looks sensible to me.

> 164  As defined in [RFC5492] the total length of capabilities that can
> be
> 165  carried by the BGP Capabilities Optional Parameter is 255 bytes.
> If
> 166  an implementation is constructing a BGP Capabilities Optional
> 167  Parameter and its length exceeds 255 bytes, it is RECOMMENDED to
> 168  exclude the Version Capability.  An implementation may optimally
> 169  achieve this by making the Version Capability the last capability
> 170  triple to add to the Parameter, and only adding it if there is
> 171  sufficient space to do so.
>
> [?] Was the use of draft-ietf-idr-ext-opt-param considered?

Author to respond, but I believe that while this is available, it is seen
as "unlikely to become an RFC any time soon". That would mean that either
the author had to fold it in to this document (which I would think makes
it absolutely IDR work) or sit with a normative reference for ever.

> [major] Some of the other Capabilities are far more critical to the
> operation of a BGP session than this one: multiprotocol extensions,
> extended messages, add-path...to mention a few.  As specified ("SHOULD
> be no greater than 64" and "it is RECOMMENDED to exclude the Version
> Capability"), the result could easily be the inability of the session
> to properly function if other Capabilities are not included.
>
> Why is the behavior recommended and not required? I can imagine how a
> Version may need a long string...but excluding the Version Capability
> should be REQUIRED.

I think the authors decided on RECOMMENDED because an implementation
should be free to decide which capabilities are more or less important.
However, as indicated in my response to the comment at the top of this
email, I would be comfortable with:

- SHOULD be no greater than 64 bytes
- MUST be left out if would constrain inclusion of any other Standards
Track capabilities

> As mentioned at the top, text in the Security Considerations section
> should also highlight this risk.
>
> Suggestion (for the Security Considerations)>
>  A rogue node can prevent the proper operation of a BGP session, or
>  the advertisement of other Capabilities, by not excluding the Version
>  Capability as required in Section 3.1.  This risk is equivalent to a
>  rogue node simply not advertising a specific Capability and is not
>  new to BGP.

Sorry, but by your own text, this is at best unhelpful. A rogue node can
do anything! A rogue node can decide to omit or lie about capabilities; it
can add new and unknown capabilities that fill up the the attribute; it
can violate any MUST from a protocol spec; it can generate random streams
of bytes.

Adding your text would, I think, not be very harmful, but it gives the
appearance that this protocol extension introduces a new threat. It does
not. So if you wanted something added it would need to say "BGP includes
no protection against rogue or subverted implementations. This
specification does not introduce any security measures to address that
problem." Personally, I don't think that this is the place for a
commentary on the general deficiencies of BGP.

> [major] The length to be considered is not 255 octets of Capabilities,
> but of Optional Parameters -- take a look at rfc4271/§4.2.

Oh, this is a splendid point!
And here we have the issue that even modest constraint on the length of
the Capabilities may cause us to run out of space for other Optional
Parameters.

IANA helpfully shows us that there are not many optional parameters
defined
(https://www.iana.org/assignments/bgp-parameters/bgp-parameters.xhtml#bgp-parameters-11)
so this problem is not rampant, but it raises two questions for the
authors:
1. Why a capability code and not an optional parameter?
   (I suspect the registry assignment rules may have a lot to do with
   this!)
2. How do you stop the version capability crowding out other optional
   parameters?

> 186  Enabling (i.e., turning on) this capability requires bouncing all
> 187  existing BGP sessions and the feature MUST be explicitly
>      configured
> 188  before an implementation advertizes the Version Capability.
>
> [minor] There is already a specification in §3 about explicit
> configuration -- no need to specify the behavior twice.
>
> Suggestion>
> Enabling (i.e., turning on) this capability may require a restart of
> any existing BGP sessions and its explicit configuration (Section 3).

Fair enough.

> [] Note that some vendors support the ability to change the software
> version without restarting the BGP sessions...which means that the
> Capability may have stale information.

Yeah, I have some memory of that being discussed, but can't put my finger
on it. It would be useful to have some text on this case.

> 192  Below is an example of implementation in [FRRouting] how it looks
> 193  like with version advertised by a BGP sender:
>
> [] Suggestion>
> Below is an example from the [FRRouting] implementation showing both
> the received and advertised Version Capability:

Sounds reasonable.

> 195  :~# vtysh -c 'show ip bgp summary failed'

[snip]

> [] I'm not sure what this figure is showing.

Authors should add an explanation or pointer as appropriate

> 216  IANA maintains the "Border Gateway Protocol (BGP) Parameters"
> 217  registry with a subregistry called "Capabilities Codes".
>
> [major] The Capability Codes registry is a standalone registry.

Well, perhaps not major?

If you go to https://www.iana.org/protocols and search for "Border Gateway
Protocol (BGP) Parameters" you easily enough find a list of registries of
which one is clearly called "Capabilities Codes".

We can debate (endlessly?) whether the first is a registry and second a
subregistry. I am almost certain that not too many people will worry about
the result of our debate.

> 229	6.  Security Considerations
>
> 231 The Version Capability should be treated as sensitive information:
>     it
> 232 could be easier for an attacker to exploit the system if they know
> 233 the specific version of a BGP speaker.  This information could be
> 234 gathered by inspecting BGP OPEN messages that carry the Version
> 235 Capability defined in this document.  Using encryption to protect
>     the
> 236 information exchanged in BGP sessions SHOULD, therefore, be
>     carefully
> 237 considered when this feature is enabled.  Suitable encryption can
>     be
> 238 achieved by protecting the BGP session using TLS [RFC5246].
>
> [major] There is no Standards Track document that talks about
> encrypting a BGP session, much less recommending it (SHOULD).  This is
> not an IETF consensus document, so providing this type of guidance is
> out of place, and may cause unnecessary confusion that could disrupt
> other work.

As discussed at the top of this email, I don't see how recommending the
use of encryption when this feature is in use impacts other
implementations or deployments of BGP. In no way does this document
provide guidance for regular BGP deployments.

I suspect that TLS might be an available mechanism for protecting BGP
sessions. I have heard people talk about IPsec as an appropriate approach.
Some have claimed MACsec also works (although it seems there are
limitations).

It seems to me that pointing out the threat and recommending a mitigation
is exactly what this document should do.

> 240  Furthermore, knowledge of which versions of code is running on a
> 241  given router and from which vendor it comes may facilitate a
>      number
> 242  of social-engineering attacks. This further adds to the desire to
> 243  protect this information through encryption.
>
> 245  Modifying the information advertised by a router might lead to
> 246  attacks including bogus software upgrades and also might mask the
> 247  causes of faults in the network.  This can be mitigated by
>      protecting
> 248  the BGP session using TLS.
>
> [major] Confidentiality and integrity are issues that apply to BGP in
> general, and are not exclusive to this extension.

Agreed. And why is it wrong for this document to observe that the
extension in this document is vulnerable?

It is the job of the security considerations section to consider the
extension and the information it carries, and to set out any additional
threats and vulnerabilities that its use create. Where possible the
section should also describe mitigation.

> Suggestion (to replace the first 3 paragraphs -- borrowing heavily
> from rfc8203)>
>
> The Version Capability should be treated as sensitive information: it
> could be easier for an attacker to exploit the system if they know the
> specific software version and manufacturer of a BGP speaker.  This
> information could be gathered by inspecting BGP OPEN messages that
> carry the Version Capability defined in this document.  Furthermore,
> this knowledge may facilitate a number of social-engineering attacks.
>
> Modifying the information advertised by a router might lead to attacks
> including bogus software upgrades and also might mask the causes of
> faults in the network.
>
> Users of this mechanism should be aware that unless a transport that
> provides integrity is used for the BGP session in question, the
> Version Capability can be forged.  Unless a transport that provides
> confidentiality is used, the Version Capability could be snooped by
> an attacker.  These issues are common to any BGP message but may be of
> greater interest in the context of this extension as explained above.
> Refer to the related considerations in [RFC4271] and [RFC4272].
>
> Users of this mechanism should consider applying data minimization
> practices as outlined in Section 6.1 of [RFC6973], as appropriate
> within the deployment context.

I don't have any objection to this new text.

> 250  Many BGP implementations leave TCP port 179 open in order to be
> able
> 251  to establish sessions with new neighbors.  This could lead to an
> 252  attack where a rogue BGP implementation attempts to open a
>      session
> 253  and learns the version information from the responding peer.
>
> [] This is a general vulnerability.  See related comments after the
> last paragraph in this section.

Again, while agreeing that this is a general BGP threat, the information
gathered is specific to this document.

> 255  The Version Capability MUST be configurable with a vendor-
>      specific
> 256  knob to be able to enable or disable the capability.  The vendor
> 257  might implement to disable this capability per neighbor.
>
> [minor] The configuration specification is already detailed in §3 --
> no need to describe more here.  The only extra functionality (not in
> §3) is the ability to disable the capability per-neighbor -- include
> it in §3.

I agree to including the per-neighbor configuration in section 3.
I disagree to removing this material from this section as it applies to
the security of the implementation.
Adding a back pointer to section 3 and reducing this text would be fine.
Thus...
The Version Capability MUST be configurable within an implementation as
described in Section 3.

> 259 It would be safe to enable this for iBGP or inside the same tenant
> 260 where you have full control and the BGP speaker is behind the exit
> 261 points.
>
> [minor] This paragraph sounds very tentative ("would be safe") and not
> explicit about the meaning of "full control" or being "behind the exit
> points".  In general I understand what you mean, but it many be too
> vague for an RFC.  Take a look at RFC8799, which talks about
> "controlled environments" (aka limited domains) -- that might be a
> good reference for what you mean above.

It's the subjunctive.
It can be rewritten as "It is safe..."
Agree that clarifying "full control" would be valuable.

> [minor] This topic is related to the text already in §3 about
> deploying outside a single AS (see my comments there too).
> Suggestion: keep these operational considerations in one place.
> Perhaps §3 is not ideal -- consider §4 instead.

The authors may consider an Operational Considerations section, however, I
think it critical to the security of an implementation that this material
is at least referenced from the security considerations section.

> 263	   This capability is NOT RECOMMENDED for eBGP use.
>
> [minor] As mentioned above, put all operational/deployment
> considerations in one place.

As above.

> BTW, this is the only place where eBGP
> is used...perhaps the AS-related terminology is better.

Good point to consider.

> 265 Sensitive information leaks can be minimized by using the
>     [RFC5082]
> 266 mechanism or firewalls to filter out TCP 179 port from untrusted
> 267 networks. This capability can be disabled per neighbor, thus the
> 268 sensitive information can't be disclosed to untrusted neighbors.
>
> [minor] General vulnerabilities. These two techniques (and other
> general suggestions) are covered in rfc7454 (BGP Operations and
> Security). A reference to that document instead of mentioning
> specifics that are not exclusive to this extension may be better.

A reference *as*well*as* the current text would be good.

> [major] One more thing -- borrowing also from rfc8203 -- add the
> following:
>
> This document uses UTF-8 encoding. There are a number of security
> issues with Unicode.  Implementers and operators are advised to review
> Unicode Technical Report #36 [UTR36] to learn about these issues.
> UTF-8 "Shortest Form" encoding is REQUIRED to guard against the
> technical issues outlined in [UTR36].
>
> Here's the reference:
>
> [UTR36]  Davis, M., Ed. and M. Suignard, Ed., "Unicode Security
>          Considerations", Unicode Technical Report #36, August
>          2010, <http://unicode.org/reports/tr36/>.

That's good text, thanks (and highlights the point that the security
considerations section *should* include descriptions of generic security
concerns (in this case a UTF-8 issue) that are described elsewhere.

Best,
Adrian
-- 
Adrian Farrel (ISE),
[e-mail]