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

[Alvaro Retana <[e-mail]>]

Dear Donatas:


Thank you for your work on this document!

I am reviewing your document prompted by the IESG Conflict Review
(rfc5742) requested by the ISE; I am the Shepherding AD.  I cc'd the
idr WG in case there are any comments on the document or my review.

In general, the document is mostly clear, and the functionality may be
of interest to some.  From that point of view, I think there are no
conflicts that prevent publication.  I have several comments below
(in-line).


However, I have a couple of points that would prevent me from
declaring the document as conflict-free.  I am cc'ing the ISE for
awareness of my current thinking.

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

    The threats that you identify are not exclusive to this extension, so the
    solution/mitigation shouldn't be either.  This issue should be easy to
    address -- I suggested alternative text below.


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


...
66	1.  Introduction
...
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.


...
94	3.  Version Capability

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

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


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.  Note that rfc5492 specifies a
different behavior, specifically about sending a Notification:

   If a BGP speaker receives from its peer a capability that it does not
   itself support or recognize, it MUST ignore that capability.  In
   particular, the Unsupported Capability NOTIFICATION message MUST NOT
   be generated and the BGP session MUST NOT be terminated in response
   to reception of a capability that is not supported by the local
   speaker.


Suggestion>
   An implementation that does not recognize or support the Version
   Capability but receives one must ignore it, as described in [RFC5492].



...
132	   Capability Length

[minor] For completeness, please add the following from rfc5492:

         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


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.


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

152	                                 Figure 1

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


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.

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


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.


162	3.1.  Capabilities Length Overflow

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?


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

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.


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


173	4.  Operation
...
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).


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


190	4.1.  Example Usage

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:


195	     :~# vtysh -c 'show ip bgp summary failed'
196	     ...
197	     Neighbor EstdCnt DropCnt ResetTime Reason
198	     ens192         3       3  00:00:35 Waiting for peer OPEN (n/a)
199	     ens224         3       3  00:01:12 Waiting for NHT (FRRouting 7.2)
200	     eth0           3       3  00:00:14 Neighbor deleted (FRRouting 7.3)
201	     ...

203	                                 Figure 2

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



...
214	5.  IANA Considerations

216	   IANA maintains the "Border Gateway Protocol (BGP) Parameters"
217	   registry with a subregistry called "Capabilities Codes".  IANA is
218	   requested to assign a capability number from the First Come First
219	   Served range for the Version Capability in this document as follows:

[major] The Capability Codes registry is a standalone registry.


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


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.

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.


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.


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.


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.


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


263	   This capability is NOT RECOMMENDED for eBGP use.

[minor] As mentioned above, put all operational/deployment
considerations in one place.  BTW, this is the only place where eBGP
is used...perhaps the AS-related terminology is better.


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.


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