Thank you for this detailed review, please see our comments below.
>
> From: Peter Yee via Datatracker <noreply@xxxxxxxx>
> Subject: Secdir last call review of draft-ietf-6man-hbh-processing-15
> Date: April 27, 2024 at 9:57:43 PM PDT
> To: <secdir@xxxxxxxx>
> Cc: draft-ietf-6man-hbh-processing.all@xxxxxxxx, ipv6@xxxxxxxx,
last-call@xxxxxxxx
> Resent-From: <alias-bounces@xxxxxxxx>
> Resent-To: bob.hinden@xxxxxxxxx, ek.ietf@xxxxxxxxx,
evyncke@xxxxxxxxx, furry13@xxxxxxxxx, gorry@xxxxxxxxxxxxxx,
otroan@xxxxxxxxxxxxx
> Reply-To: Peter Yee <peter@xxxxxxxxxx>
>
> 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
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
