Subject: [netmod] Benjamin Kaduk's Discuss on draft-ietf-netmod-acl-model-19: (with DISCUSS and COMMENT)

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 26 Sep 2018 10:31:37 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-netmod-acl-model-19: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-netmod-acl-model/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I think this is good work to have, overall, and the document pretty easy to read.
That said, I think the Security Considerations need to be expanded a bit more before
this document get published:

                                  Write operations (e.g., <edit-config>)
   to these data nodes without proper protection can have a negative
   effect on network operations.

I think the effects can be on more than just *network* operations, there
can be negative effects for end systems that (e.g.) experience DoS attacks
that would otherwise have been blocked, receive maliciously crafted packets
that trigger application bugs, are used as part of (e.g.) UDP amplification
attacks, etc.

      /acls/acl/aces: This list specifies all the configured access
      control entries on the device.  Unauthorized write access to this
      list can allow intruders to access and control the system.
      Unauthorized read access to this list can allow intruders to spoof
      packets with authorized addresses thereby compromising the system.

I agree with the secdir reviewer that "the system" needs to be clarified,
and that the consequences of unauthorized write and read access need to be
more clearly described.
His proposed text is much better than the present text, though there are
other ways to convey the needed information.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I tried to call out the editorial nits as such; there are a couple non-editorial
comments embedded within.

Section 1

   The match criteria allows for definition of packet headers and
   metadata, all of which must be true for the match to occur.

nit: Is this missing a word like "contents"?

   The matching of filters and actions in an ACE/ACL are triggered only
   after application/attachment of the ACL to an interface, VRF, vty/tty
   session, QoS policy, routing protocols amongst various other config
   attachment points.

nit: I think the end of this list needs some clarification/termination,
like "and routing protocols, amongst"

Section 3

                                                                  The
   match criteria allows for definition of packet headers or metadata,
   if supported by the vendor.  [...]

(same nit as above re "contents")

   Metadata matching applies to fields associated with the packet, but
   not in the packet header such as input interface, packet length, or
   source or destination prefix length.  The actions can be any sort of

nit: comma after "not in the packet header"

Section 4.1

nit: The feature match-on-udp and -icmp descriptions should probably use
the plural "headers" to match the other features' descriptions.

The mixed-<blah> features seem to implicitly assume that if features X and
Y are individually supported, then the combination is also supported.  I
could imagine that there might exist hardware for which that assumption is
not true, but don't know if there actually is any such hardware or it's
common enough to be worth caring about here.

   grouping acl-counters {
     leaf matched-packets {
      [...]
          An implementation should provide this counter on a
          per-interface per-ACL-entry if possible.

nit: missing "basis"?  (Also in subsequent instances.)

Section A.1

It's unclear that using [email-masked] (in particular, the @newco.com part)
in an example is reasonable; @newco.example would be better.

---

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 26 Sep 2018 11:25:37 -0700

Hi Benjamin,

> On Sep 26, 2018, at 10:31 AM, Benjamin Kaduk <[email-masked]> wrote:
> 

How about this?

OLD:

NEW:
   Write operations (e.g., <edit-config>)
   to these data nodes without proper protection can have a negative
   effect on network operations and end systems. The end systems, for
   example, can experience DoS attacks that would otherwise have been blocked,
   and receive maliciously crafted packets that trigger applications bugs.

>      packets with authorized addresses thereby compromising the system.

Back in July we went through this section, and here was the change that was proposed, that Steve had accepted. Since they were provided for the current version of the draft (-19), they were not applied till we had received all the reviews. Were you looking for changes in addition to this?

OLD:

NEW:
              /acls/acl/aces: This list specifies all the configured access
      control entries on the device.  Unauthorized write access to this
      list can allow intruders to modify the entries so as to permit traffic
      that should not be permitted, or deny traffic that should be permitted.
      The former may result in a DoS attack, or compromise the device.
      The latter may result in a DoS attack. The impact of an unauthorized 
      read access to the list will allow the attacker to determine which rules
      are in effect, to better craft an attack.

> nit: Is this missing a word like "contents”?

I am not sure if we are, possibly because I do not understand what you mean by “contents”.  ACL rules are written to match against packet headers and metadata. They are not written to match against the “contents” of the packet.

> like "and routing protocols, amongst”

Ok. Will add the word ‘and’ after the comma. 

> nit: comma after "not in the packet header”

Ok.

> the plural "headers" to match the other features' descriptions.

Ok.

> common enough to be worth caring about here.

The individual feature statements exist to allow for the server to pick what the hardware supports. If the hardware does not support the combination, the server will choose not to advertise the feature statements for the combinations.

> nit: missing "basis"?  (Also in subsequent instances.)

Ok.

> in an example is reasonable; @newco.example would be better.

I do not know if a contact e-mail address, in an example of a YANG model, is significant.

Thanks.

> 
> 
> _______________________________________________
> netmod mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netmod

Mahesh Jethanandani
[email-masked]

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 28 Sep 2018 11:40:09 -0500

Hi Mahesh,

>    and receive maliciously crafted packets that trigger applications bugs.

That looks great; thanks!

>       are in effect, to better craft an attack.

Ah, I was just looking at the -19 as-is and didn't read to the end of the
secdir thread.  This text is also good; I'll clear my discuss.

> I am not sure if we are, possibly because I do not understand what you mean by “contents”.  ACL rules are written to match against packet headers and metadata. They are not written to match against…

"definition of packet headers and metadata" sounds like something that a
protocol designer or hardware vendor specifies; it's the values of those
fields that a firewall device is using for its logic.

> The individual feature statements exist to allow for the server to pick what the hardware supports. If the hardware does not support the combination, the server will choose not to advertise the fea…

I must have been misreading the YANG, then; sorry for the confusion.

> I do not know if a contact e-mail address, in an example of a YANG model, is significant.

I don't either, so this was just a non-blocking comment.

Thanks for the quick fixes!

-Benjamin

---

From: Mahesh Jethanandani <[email-masked]>
Date: Fri, 28 Sep 2018 18:22:53 -0700

[Removing the resolved items]

> On Sep 28, 2018, at 9:40 AM, Benjamin Kaduk <[email-masked]> wrote:
> 
>>> Section 1
>>> 

Ahh! I see what you mean. How about this?

OLD:

NEW:
   The match criteria allows for definition of packet headers and
   metadata, the contents of which must match the definitions.

Thanks.

Mahesh Jethanandani
[email-masked]