Subject: Gen-ART LC Review of draft-ietf-opsec-dhcpv6-shield-04

From: Ben Campbell <[email-masked]>
Date: Mon, 1 Dec 2014 17:58:02 -0600

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-opsec-dhcpv6-shield-04
Reviewer: Ben Campbell
Review Date: 2014-12-01
IETF LC End Date: 2014-12-01

Summary: This draft is almost ready for publication.I have some questions and comments that might should be considered prior to publication. (I am leaving off my usual "ready for publication as an XXX" part, because I have questions on that, below.)

Major issues:

(Note: This is not so much a "major issue" as a meta-concern that doesn't fit well in the major/minor/nit structure.)

I have questions about the intended status. The draft lists BCP. It's not clear to me if we intend to say that it's a "best practice" to implement DHCPv6 filtering, or that, if you do implement it, it's a best practice to do it like this. Given the strength of a BCP, I think it's worth clarifying that.

As far as I can tell, DHCP snooping for v4 is not documented in an RFC at all, and that RA-Guard, which this draft lists as analogous, is informational.

Minor issues:

-- abstract, last sentence:

I didn't find this assertion in the body itself. It would be nice to see support for it (perhaps with citations).

-- section 4:

Am I correct in understanding that this is opt in only? You would disallow a rule of the form of "allow on any port except [list]"?

Nits/editorial comments:

-- section 1, 2nd paragraph:

This is the first mention of "DHCP-Shield" I found, not counting the doc name and headers. It would be helpful to have an early mention that "DHCP-Shield" is the name of the thing that this draft defines.

-- section 1, 3rd paragraph:

It would be helpful to define what a "DHCP-Shield device" is, prior to talking about deploying and configuring them.

-- section 3, paragraph ending with  with "... used as follows [RFC7112]:"

I'm a bit confused by the citation. Are these defined "as follows", or in 7112?

-- section 3, "Extension Header"

-- these are IPv6 extension headers, right?

-- section 3, "IPv6 Header chain"

Is there a relevant citation for this?

Also, while this section talks about some aspects of header chains, it never actually defines the term.

-- section 3, "Upper-Layer Header"

Again, this section talks about the term without defining it.

-- section 5, list entry "1": "... the IPv6 entire header chain ..."

should this be "... the entire IPv6 header chain ..."?

-- section 3, rational for item 1: "An implementation that has such an implementation-specific limit MUST NOT claim compliance with this specification."

That's an odd use of 2119 language; I assume this to be true anytime an implementation violates a MUST/MUST NOT assertion.

-- section 3, rational for list item 3:

It would be helpful if this rational said why dropping unrecognized next header values is needed for the purpose, not just the fact that 7045 allows it to be dropped.

-- section 3, list item 4:

Am I correct in assuming that there might be other (non-dhcp) reasons a device might still drop packets that otherwise pass the dhcpv6-shield tests?

-- section 3, paragraph after list item 4:

The comment that dropped packets SHOULD be logged is redundant with the same statement in the numbered rules.

-- section 7, first paragraph:

Please expand DoS on first mention.

-- section 7, 2nd to last paragraph: "... on a port not allowed to send such packets ..." 

Should that be "forward" or "receive"?  (I assume this still talks about L2 switch "ports", not UDP ports?)

---

From: Fernando Gont <[email-masked]>
Date: Tue, 02 Dec 2014 08:03:34 -0300

Hi, Ben,

Thanks so much for your feedback! Please find my comments in-line...

On 12/01/2014 08:58 PM, Ben Campbell wrote:
> Document: draft-ietf-opsec-dhcpv6-shield-04 Reviewer: Ben Campbell 
> Review Date: 2014-12-01 IETF LC End Date: 2014-12-01
> 
> Summary: This draft is almost ready for publication.I have some
> questions and comments that might should be considered prior to
> publication. (I am leaving off my usual "ready for publication as an
> XXX" part, because I have questions on that, below.)
> 
> Major issues:
> 
> (Note: This is not so much a "major issue" as a meta-concern that
> doesn't fit well in the major/minor/nit structure.)
> 
> I have questions about the intended status. The draft lists BCP. It's
> not clear to me if we intend to say that it's a "best practice" to
> implement DHCPv6 filtering, or that, if you do implement it, it's a
> best practice to do it like this. Given the strength of a BCP, I
> think it's worth clarifying that.

BCP of implementing it, if you do.

How about adding this text to the intro:

"This document specifies a Best Current Practice for the implementation
of DHCPv6 Shield (DHCPv6)"

?

> I didn't find this assertion in the body itself. It would be nice to
> see support for it (perhaps with citations).

I guess one could provide references to some vendor's manuals? Is that
what you have in mind? (although I'd prefer not to do so).

> -- section 4:
> 
> Am I correct in understanding that this is opt in only? You would
> disallow a rule of the form of "allow on any port except [list]"?

Not sure what you mean.

The idea is that if you want to enable dhcpv6 shield, you need to
specify on which port(s) the dhcpv6 server(s) is/are connected.

> This is the first mention of "DHCP-Shield" I found, not counting the
> doc name and headers. It would be helpful to have an early mention
> that "DHCP-Shield" is the name of the thing that this draft defines.
> 
> It would be helpful to define what a "DHCP-Shield device" is, prior
> to talking about deploying and configuring them.

How about adding (in Section 1) the following text:

    This document specifies DHCPv6 Shield: a set of filtering
    rules meant to mitigate attacks that employ DHCPv6-server
    packets. Throughout this document we refer to a device
    implementing the DHCPv6 Shield filtering rules as the "DHCPv6
    Shield device"

?

> -- these are IPv6 extension headers, right?

Yes. Should we explicitly say so?

> Is there a relevant citation for this?

Other than RFC7112?

> it never actually defines the term.

Which one?

> -- section 5, list entry "1": "... the IPv6 entire header chain ..."

Not sure what you mean: Section 3 is all about defining the terms. Am I
missing something?

> should this be "... the entire IPv6 header chain ..."?

Yes. Will fix this.

> -- section 3, rational for item 1: "An implementation that has such
> an implementation-specific limit MUST NOT claim compliance with this
> specification."
> 
> That's an odd use of 2119 language; I assume this to be true anytime
> an implementation violates a MUST/MUST NOT assertion.

You're right. Should we just change this to lowercase, or do you think
we should remove the whole sentence?

> It would be helpful if this rational said why dropping unrecognized
> next header values is needed for the purpose, not just the fact that
> 7045 allows it to be dropped.

How about adding this at the end of the RATIONALE:

"An unrecognized Next Header value could possibly identify an IPv6
Extension Header, and thus be leveraged to conceal a DHCPv6-server packet".

> Am I correct in assuming that there might be other (non-dhcp) reasons
> a device might still drop packets that otherwise pass the
> dhcpv6-shield tests?

Yes. Why?

FWIW, the filtering rules in this document just talk about whether a
packet should be blocked or not based on these rules.

> The comment that dropped packets SHOULD be logged is redundant with
> the same statement in the numbered rules.

How about changing this:

   If a packet is dropped due to this filtering policy, then the packet
   drop event SHOULD be logged in an implementation-specific manner as a
   security fault.  The logging mechanism SHOULD include a drop counter
   dedicated to DHCPv6-Shield packet drops.

to:

   The above rules require that if a packet is dropped due to this
   filtering policy, the packet drop event be logged in an
   implementation-specific manner as a security fault.  The logging
   mechanism SHOULD include a drop counter
   dedicated to DHCPv6-Shield packet drops.

?

(and there are a few more requirements when it comes to *what* should be
logged, as noted by others during the IETF LC).

> Please expand DoS on first mention.

Will do. Thanks!

> about L2 switch "ports", not UDP ports?)

Yes. But we will clarify the terminology and use "is not allowed to
receive.." in all cases.

Thanks!

Best regards,
-- 
Fernando Gont
SI6 Networks
e-mail: [email-masked]
PGP Fingerprint: 6666 31C6 D484 63B2 8FB1 E3C4 AE25 0D55 1D4E 7492

---

From: Ben Campbell <[email-masked]>
Date: Thu, 11 Dec 2014 15:56:28 -0600

Thanks for your response. Here's some additional comments. I removed sections that didn't seem to need further discussion.

> On Dec 2, 2014, at 5:03 AM, Fernando Gont <[email-masked]> wrote:

[...]

> of DHCPv6 Shield (DHCPv6)"

That would help, thanks!

> what you have in mind? (although I'd prefer not to do so).

The citations part was more of a nice to have. But it would be worth putting some words around that in the body, even if there's nothing to reference.

> specify on which port(s) the dhcpv6 server(s) is/are connected.

Would a rule of the form "Allow DHCPv6 on all ports except port X" be allowed?

>    implementing the DHCPv6 Shield filtering rules as the "DHCPv6

Yes, thanks.

>> I'm a bit confused by the citation. Are these defined "as follows",

You did not respond to this one. I note that my next few comments might no longer apply if the 7112 reference is clarified. Is the point to say that 7112 contains the following definitions, which are repeated here for the sake of convenience?

> Yes. Should we explicitly say so?

I think it would help. The 7112 reference bit might fix it, but as written the first explicit mention of IPv6 in the section is the IPv6 Header Chain subsection.

>> Is there a relevant citation for this?

See 2nd comment back.

>> it never actually defines the term.

The term "header chain".  That is, something of the form of "The IPv6 header chain is a linked-list of IPv6 headers. It contains ...".

> Not sure what you mean: Section 3 is all about defining the terms. Am I

Again, the definition doesn't actually define the term. For example, "An upper-layer header is a header belonging to an upper-layer protocol"

[...]

> we should remove the whole sentence?

Either would be okay. I guess it comes down to whether you think this requirement (parsing the entire chain) needs to be called out more strongly than any other MUST level requirement. 

> Extension Header, and thus be leveraged to conceal a DHCPv6-server packet".

That helps, thanks.

> packet should be blocked or not based on these rules.

I guess it depends on whether "MUST pass the packet" means pass it for further processing (which could include other filtering rules), or pass it onto the wire. I see your point that the draft intends the former, but I think some implementers might read it as the latter.

>   dedicated to DHCPv6-Shield packet drops.

Works for me.

[...]

Thanks!

/Ben