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

[Benjamin Kaduk <[e-mail]>]

Hi Mahesh,

On Wed, Sep 26, 2018 at 11:25:37AM -0700, Mahesh Jethanandani wrote:
> Hi Benjamin,
> 
> > On Sep 26, 2018, at 10:31 AM, Benjamin Kaduk <[e-mail]> wrote:
> > 
> > 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.
> 
> How about this?
> 
> OLD:
>    Write operations (e.g., <edit-config>)
>    to these data nodes without proper protection can have a negative
>    effect on network operations.
> 
> 
> 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.

That looks great; thanks!

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

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

"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 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”
> 
> Ok. Will add the word ‘and’ after the comma. 
> 
> > 
> > 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”
> 
> Ok.
> 
> > 
> > Section 4.1
> > 
> > nit: The feature match-on-udp and -icmp descriptions should probably use
> > the plural "headers" to match the other features' descriptions.
> 
> Ok.
> 
> > 
> > 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.
> 
> 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.

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

> > 
> >   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.)
> 
> Ok.
> 
> > 
> > Section A.1
> > 
> > It's unclear that using [e-mail] (in particular, the @newco.com part)
> > 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.

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

Thanks for the quick fixes!

-Benjamin