Re: [secdir] Secdir last call review of draft-ietf-6man-hbh-processing-15

[Gorry Fairhurst <[e-mail]>]

Thank you for this detailed review, please see our comments below.

 >
 >  From: Peter Yee via Datatracker <[e-mail]>
 > Subject: Secdir last call review of draft-ietf-6man-hbh-processing-15
 > Date: April 27, 2024 at 9:57:43 PM PDT
 > To: <[e-mail]>
 > Cc: [e-mail], [e-mail], 
[e-mail]
 > Resent-From: <[e-mail]>
 > Resent-To: [e-mail], [e-mail], 
[e-mail], [e-mail], [e-mail], 
[e-mail]
 > Reply-To: Peter Yee <[e-mail]>
 >
 > Reviewer: Peter Yee
 > Review result: Has Issues
 >
 > I reviewed this document as part of the Security Directorate's 
ongoing effort
 > to review all IETF documents being processed by the IESG. These 
comments were
 > written primarily for the benefit of the Security Area Directors.  
Document
 > authors, document editors, and WG chairs should treat these comments 
just like
 > any other IETF Last Call comments.
 >
 > Document: draft-ietf-6man-hbh-processing-15
 > Reviewer: Peter Yee
 > Review Date: 2024-04-27
 > IETF LC End Date: 2024-04-29
 > IESG Telechat date: Unknown
 >
 > Summary: This document updates RFC 8200 with regard to processing of IPv6
 > Hop-by-Hop options. This has been done with an eye towards reducing 
security
 > problems (mostly denial of service). There are some inconsistencies 
between the
 > main body of the document and its Security Considerations. The nits 
listed are
 > just because I can’t read a document without fixing the jarring editorial
 > things my eyes can’t unsee. [Has Issues]
 >
 > Major issues: None
 >
 > Minor issues:
 >
 > I feel the main body of the document and Security Considerations were 
almost
 > written by different authors who had a different outlook on the 
content from
 > mine. That might be because I’m not in the IPv6 space. Some of my 
thoughts
 > follow.

GF:  It is true there are two authors, each with their own version of 
English :-)

 >
 > Page 14, section 8, 4th paragraph, 1st sentence: “the document notes …
 > malformed/malicious protocols fields”. I find that a stretch. Is 
malformed
 > meant to be the small caveat that in the last paragraph of 5.2.1, 2nd 
sentence
 > about verify a protocol of interest? There’s not much I can see in 
the way of
 > malformedness in that. As for malicious, that term doesn’t appear in the
 > document other than in the security considerations. Further, outside 
of the
 > general concept of Denial-of-Service attacks, part of what this document
 > attempts to fix is generally not processing options if the control 
plane is
 > going to get bogged down, but that doesn’t necessarily indicate malice
 > expressed in the incoming packets.

GF: Not a stretch: a node can form a packet that includes an EH that 
incurs significant work by a router on-path - be it by accident or 
intent. It's not possible for a router to know why such packets were 
received, it simply has to decide what to do. We propose to add an 
example, hoping that helps:

"For example, a
packet with an excessive number of options could consume significant
resources; inclusion of a large extension header could potentially
cause an on-path router to be unable to utilise hardware
optimisations to process later headers (e.g., to perform ECMP, or
port filtering)."

---
 >
 > Page 14, section 8, 1st bullet item, 2nd sentence: Here it’s stated 
that the
 > first Hop-by-Hop option MUST be processed. I could find that nowhere 
in the
 > document in so many words. There was one part (page 8, last paragraph 
and one
 > over to the next page) that sort of implies that this had been an 
idea, but it
 > doesn’t appear to be fleshed out in the text preceding it nor in the 
rest of
 > the document.

GF: This was indeed inconsistent, thanks for the comment, the the 
security considerations will be updated to say:

         A router configuration needs to avoid vulnerabilities that
        arise when it cannot process a Hop-by-Hop option at full
        forwarding rate and SHOULD NOT therefore be configured to
        process the a Hop-by-Hop option if this adversely impacts the
        aggregate forwarding rate, instead it
        SHOULD behave in the way specified for an unrecognized Option 
Type when the
        action bits were set to "00", as specified in XXX.

- we hope this addresses this issue.

---
 > Page 14, section 8, 3rd bullet item, 1st sentence: where is this 
definition?
 > The word “default” appears once in the document. In the security
 > considerations. I could not find text that made think I had seen a 
definition
 > of a default number. Perhaps I just didn’t understand an implication 
somewhere.

GF: I see this was not wisely worded - i.e. was hacked to many times, we 
suggest this should say:
...
         The document sets an expectation that if a packet
         includes a single Hop-by-Hop option that packet will be
         forwarded across the network path.

---
 > The issues I noted above would be helped if the security 
considerations pointed
 > out where these changes can be found.
 >
 > Nits/editorial comments:
 >
 > General:
 >
 > With the exception of the beginning of sentence or in the term 
“Source Address”
 > or “Destination Address”, the terms “source” and “destination” should be
 > written in lower case to match the usage in the other IPv6 documents 
(e.g., RFC
 > 8200, draft-ietf-6man-eh-limits, etc.). My actual preference would be 
to make
 > the terms lower case as well in the Terminology section and then use them
 > accordingly in the rest of the document. Some of the document’s own 
usage mixes
 > cases. Most of the other IPv6 documents use terms such as Control 
Plane in
 > lower case as a further justification for going to lower case here as 
well.

GF: RFC 8200 seems to mostly use: "source", but "Source Address" -we 
propose to copy this usage.

 > I noticed a mix of British and American English in the document 
(“unrecognized”
 > [American] and “behaviour” [British]). Choose one dialect for use 
throughout
 > the document. (See RFC 7322, section 3.1.)

GF: Thanks. We will change where we see issues, and assume the RFC-Ed 
will complete this review.

 > Change “Denial of Service” to “Denial-of-Service” in several places 
in the
 > document. Wouldn’t want anyone to think we were denying a service 
attack. ;-)
 >
 > Specific:
 >
 > Page 1, Abstract, 2nd sentence: change “(RFC8200)” to “(RFC 8200)”. 
(See RFC
 > 7322, section 3.5.)
 >
 > Page 1, Abstract, 3rd sentence: change “RFC8200” to “RFC 8200”.
 >
 > Page 4, 3rd bullet item last sentence: change “EH” to “Extension 
Header (EH)”
 > as this is the first use of “EH”. “EH” is not in the RFC Editor’s list of
 > abbreviations that do not need to be expanded. (See
 > https://www.rfc-editor.org/materials/abbrev.expansion.txt) Change 
“40B” to “40
 > B”.
 >
 > Page 4, last paragraph: append a comma after “[RFC2460]”.
 >
 > Page 6, 3rd bullet item, 1st sentence: I’d change “etc.) that” to “etc.),
 > which”. The “that” might otherwise be read (by the unwary reader, 
like me) to
 > refer to “router management protocols” and require some slow parsing 
to tease
 > out the correct meaning.
 >
 > Page 6, 3rd bullet item, 2nd sentence: change “forward” to “forwards”.
 >
 > Page 7, 1st full paragraph, 2nd sentence: I’m not sure what “was 
raised as an
 > issue” in this sentence. Neither “This document” nor “[RFC7045]” 
makes sense as
 > the subject of that predicate.
 >
 > Page 7, section 5.1, 2nd paragraph, 2nd sentence: I’d delete “can” before
 > “only” as superfluous.
 >
 > Page 7, section 5.1, 4th paragraph, 2nd sentence: consider changing 
“Extension
 > Header” to “Extension Header(s)” as there could be more than one 
Extension
 > Header after the Hop-by-Hop Option header.
 >
 > Page 8, 1st partial paragraph, 1st full sentence: change “that” to 
“whether”.
 >
 > Page 8, 1st full paragraph, 3rd sentence: insert “the” before “Hop-by-Hop
 > Options header”.
 >
 > Page 9, 1st partial paragraph, last sentence: Can't this second 
sentence be
 > folded into the first? Then the two would read: A router SHOULD NOT 
therefore
 > be configured to process any Hop-by-Hop options that adversely 
impacts the
 > aggregate forwarding rate. The only reason that it wouldn’t make sense to
 > combine them would relate to my issue with the Security 
Considerations (see
 > above) about the assertion that the first option must be processed.

GF: As a small change, the current text could be better perhaps as:

A router SHOULD process additional Hop-by-Hop
options, if configured to do so, providing that these also do not
adversely impact the aggregate forwarding rate.


---
 >
 > Page 10, value 10: append a comma after “discarded”.
 >
 > Page 10, value 11: append a comma after “multicast address”.
 >
 > Page 11, 1st paragraph, 2nd sentence: I’d suggest changing “results” 
to “can
 > result”. I don’t think use of the Router Alert Option has to result 
in the
 > concerns discussed in section 4, although they may very well do so in 
some
 > circumstances.
 >
 > Page 11, 1st paragraph, 3rd sentence: delete the comma after “option”.
 >
 > Page 11, 2nd paragraph, last sentence: These what? Perhaps append
 > “restrictions” after “These”.
 >
 > Page 11, 3rd paragraph: Start the paragraph with a single quotation 
mark and
 > end the quoted text with a single quotation mark since there’s already an
 > embedded bit with double quotation marks. In any case, there were 
only three
 > quotation marks in the whole paragraph, so a match is needed of a 
deletion of
 > the closing quotation mark is in order. I’ll note that the quotation 
is not
 > exact anyhow: “Implementation” is capitalized, and “option” is not, 
unlike the
 > source material.
 >
 > Page 11, 5th paragraph, 2nd sentence: I’d qualify this sentence by 
appending
 > “that does recognize the IP Router Alert Option” after “An 
implementation”.

GF: We capitalized “Option” in the manner that RFC 6938 does, but 
avoided doing this where the text is quoted.

 > Page 11, 5th paragraph, last sentence: delete the extraneous close 
parenthesis
 > at the end of the sentence.
 >
 > Page 12, 1st partial paragraph, 1st full sentence: change “inSection” 
to “in
 > Section”.
 >
 > Page 12, section 6, 1st bullet item, 2nd sentence: delete the comma 
and put
 > “see Section 5.2” in parentheses.
 >
 > Page 12, 1st paragraph after bullet items, 1st sentence: change “can 
not” to
 > “cannot”, to match “Chicago Manual of Style” usage. (This is the 
default for
 > the RFC Editor.)
;-)
 >
 > Page 13, section 6.1, 2nd paragraph, 1st sentence (yeah, the really 
long one):
 > delete the comma after the second occurrence of “options”. The subject of
 > “otherwise refrains” is not entirely clear to me, so it’s possible it 
should be
 > “otherwise refrain” if you’re talking about the “applications” but 
should be
 > left alone if the subject is “the path”.
 >
 > Page 13, section 6.1, 3rd paragraph: delete the extraneous space after
 > “[RFC9268]”.
 >
 > Page 13, section 6.1, 4th paragraph, 1st sentence: consider deleting 
the comma
 > after “option” and deleting the comma after “options”. Change “sends” 
to “send”
 > before “other packets”. This sentence is a bit of a mess.
 >
 > Page 14, section 8, 3rd paragraph, 2nd sentence: change “Firewall” to
 > “firewall”.
 >
 > Page 14, section 8, 3rd paragraph, 3rd sentence: delete the comma 
after “all
 > Extension Headers”.
 >
 > Page 14, 1st bullet item, 1st sentence: append “options” after 
Hop-by-Hop. Be
 > explicit about what the one exception is. I’m assuming “IP Router Alert
 > Option”, but it would be easier and clearer if this was just stated.
 >
 > Page 15, 1st bullet item: I’d delete “Although” in the second 
sentence and
 > capitalize “nodes”.
 >
 >
 >
Thanks also for the list of editorial corrections, we plan to prepare a 
new revision that addresses the points above and all the other editorial 
issues,

Best wishes,
Gorry & Bob