Re: [Netconf] draft-ietf-netconf-rfc6536bis: one week review of a specific change

[Andy Bierman <[e-mail]>]

On Fri, Nov 10, 2017 at 12:36 PM, Robert Wilton <[e-mail]> wrote:

> Hi Andy,
>
> Yes, I think those changes look OK.
>
> In addition, in 3.3.4, does it make sense to clarify that descendant data
> nodes are excluded on get requests:
>
> OLD:
>
>   Data nodes to which the client does not have read access are silently
>    omitted from the <rpc-reply> message.
>
>
> NEW:
>
>   Data nodes to which the client does not have read access are silently
>    omitted, along with any descendants, from the <rpc-reply> message.
>
>
> Also, rules 9 and 10 of 3.4.5 may also need to be updated to indicate that
> the operation is rejected if the data node, or any ancestor parent data
> node, has a default-deny-write/all statement.
>
>

OK -- I will look through the rules for related updates




> ---
>
> I think that these changes above fix the NACM draft to make it consistent
> with the examples.
>
> The remaining issue is the one first raised in this thread, i.e. the
> change of operation 'none' now requiring 'read' permissions.  If that
> change is retained to mitigate the security issue, then we also need to
> think about how to reduce the impact of the change (i.e. the issue in my
> original response to this thread still applies.)
>
>

I agree now that data-rule is changed that the original change makes NACM
more difficult to use.
There is much less ACL maintenance required if the "none" edit-op
translates to "no-check"
rather than if it is a "read" request.

It is very implementation-specific what instance-info can be derived from
the error-tag
or other error information, allowing an attacker to guess the ancestor
instances
(i.e, nodes where the access check is "none" instead of "read")

In our server, field validation is done before instance validation in most
cases,
so an 'invalid-value' can be returned regardless of the instances actually
existing or not.

I propose that the draft revert to the old text, and a note added in the
security considerations
that servers should try not to expose instance information via error
reporting.


I think your enhancement for ancestors is not the only way to solve the
"/home read permit" problem.
I agree that we don't want a permit rule for read, just the user-dir

   path=/home/user1, operation=*, group=user1, action=permit


IMO the safe way to do that is to say that a read request on a normal
NP-container
is implicitly granted if no rule is found to prevent it.  The same
free-pass is not granted
to P-containers or lists (they need explicit rules). They convey data model
information
but NP containers do not.




> Thanks,
> Rob
>
>

Andy


>
> On 10/11/2017 18:23, Andy Bierman wrote:
>
> Hi,
>
> Here are some proposed edits to make the data rule consistent with the
> examples.
> Note that this issue is not related to the edit in the original 1-week
> change.
>
>
> sec. 3.3.5:
>
> OLD:
>
>
>       data node rule:  controls access for a specific data node, identified
>       by its path location within the conceptual XML document for the
>       data node.
>
>
> NEW:
>
>       data node rule:  controls access for a specific data node and its
> descendants,
>       identified by its path location within the conceptual XML document
> for the
>       data node.
>
>
> sec 3.4.5, step 6, bullet 2:
>
>
> OLD:
>
>         *  The rule does not have a "rule-type" defined or the "rule-
>            type" is "data-node" and the "path" matches the requested
>            data node, action node, or notification node.
>
>       NEW:
>
>         *  The rule does not have a "rule-type" defined or the "rule-
>            type" is "data-node" and the "path" matches the requested
>            data node, action node, or notification node. A path is
>            considered to match if the current data node is the data node
>            specified by the path, or is a descendant data node of this data node.
>
> appendix B.4: (2 bugs in explanation)
>
> OLD:
>
>       deny-nacm:  This rule denies the "guest" group any access to the
>       <nacm> subtree.  Note that the default namespace is only
>       applicable because this subtree is defined in the same namespace
>       as the <data-rule> element.
>
>  NEW:
>
>       deny-nacm:  This rule denies the "guest" group any access to the
>       <nacm> subtree.
>
> Andy
>
>
> On Fri, Nov 10, 2017 at 9:24 AM, Robert Wilton <[e-mail]> wrote:
>
>>
>>
>> On 10/11/2017 16:33, Andy Bierman wrote:
>>
>>
>>
>> On Fri, Nov 10, 2017 at 8:16 AM, Robert Wilton <[e-mail]> wrote:
>>
>>>
>>>
>>> On 10/11/2017 15:49, Andy Bierman wrote:
>>>
>>>
>>>
>>> On Fri, Nov 10, 2017 at 5:07 AM, Per Hedeland <[e-mail]> wrote:
>>>
>>>> On 2017-11-10 11:42, Robert Wilton wrote:
>>>> >
>>>> >
>>>> > On 10/11/2017 10:02, Mahesh Jethanandani wrote:
>>>> >>
>>>> >>
>>>> >>
>>>> >>
>>>> >> Mahesh Jethanandani
>>>> >> [e-mail] <mailto:[e-mail]>
>>>> >> On Nov 10, 2017, at 10:07 AM, Andy Bierman <[e-mail]
>>>> <mailto:[e-mail]>> wrote:
>>>> >>
>>>> >>> Hi,
>>>> >>>
>>>> >>> The term "data node" is used in the document to refer to the
>>>> top-level node
>>>> >>> of the specified object, not the entire subtree (if any).
>>>> >>>
>>>> >>> The data-rule /foo does not match /foo/child1 in the text below.
>>>> >>> The child nodes are omitted because of step 11.
>>>> >>> The admin has to explicitly permit individual child nodes (or
>>>> modules).
>>>> >>> This seems correct if the read-default is "deny".
>>>> >>>
>>>> >>> Should any text be added or changed to make this more clear?
>>>> >>
>>>> >> I would agree with Robert that it was not entirely clear that rule
>>>> applied on the parent data-node does not apply to the child nodes. So yes,
>>>> it would help to clarify it.
>>>> > We should be doing more than clarifying it.  We need to fix it so
>>>> that it works in a sensible way.  I.e. to make the normative text
>>>> consistent with the behaviour currently described in the examples in
>>>> > the appendix B.4.
>>>> >
>>>> > If we follow Andy's interpretation that a data rule doesn't match
>>>> child nodes then those examples are completely wrong.  E.g. the 4th rule is
>>>> described as "This rule gives the 'admin' group read-write
>>>> > access to all acme <interface> entries."  But If the path only
>>>> strictly matches "/acme:interfaces/acme:interface" then the admin
>>>> group rule achieves nothing useful at all.  The admin is not even
>>>> > allowed to create an interface because they would not even have
>>>> permission to write to the list key 'name' node required to create a list
>>>> entry!  Instead, a separate rule would be required for every
>>>> > single possible schema node under "/acme:interfaces/acme:interface"!
>>>> I think that this makes "permit" data-node rules completely unusable.
>>>>
>>>> I strongly agree with this, and I would say that it isn't only the case
>>>> for "permit" rules - e.g. denying some access to a subtree of the data
>>>> model that would otherwise be permitted due to defaults is at least as
>>>> common, and the rules would be just as unusable for that.
>>>>
>>>> Besides the examples, I think that the very use of the term "match",
>>>> though unfortunately not defined, strongly suggests that it is something
>>>> other than use of e.g. the term "identify" would imply. Additionally,
>>>> this text in the description of the 'path' leaf is consistent with the
>>>> match being a prefix match:
>>>>
>>>>        The special value '/' refers to all possible
>>>>        datastore contents.";
>>>>
>>>> FWIW, our NACM implementation, available to (and used by) customers
>>>> since 2012, follows the prefix match logic, and I have yet to hear of
>>>> any user expecting it to do otherwise.
>>>>
>>>> > To fix this properly, we need to make the data-node path rule a
>>>> prefix match.  In particular, we need text that specifies:
>>>> >
>>>> > (i) that a data-node path match succeeds if it matches the path
>>>> prefix from the root of the tree.  I.e. so the data-rule "/foo" matches
>>>> "/foo" and all of foo's descendant children nodes.
>>>>
>>>> Strongly agree.
>>>>
>>>>
>>>
>>> I do not see how the text can be interpreted this way.
>>>
>>>
>>> Because otherwise the path match part of the NACM solution is really
>>> broken, and the path based examples in the appendix are entirely misleading
>>> and wrong.  The only way those examples make sense is the paths match
>>> descendant children nodes as well.
>>>
>>>
>>
>> IMO the text does not support this interpretation.
>>
>> The examples in B.4, and the definition of "/" matching all nodes
>> supports this interpretation.
>>
>> Hence, my opinion is that it is the text in 3.4.5 that is incorrectly
>> specified; and that the examples, definition of "/" and standard practice
>> are right.
>>
>> Otherwise, how did IETF manage to publish an RFC where the path based
>> examples are so completely wrong?   Whoever wrote and reviewed those
>> examples clearly had a different interpretation of how these path based
>> ACLs worked.
>>
>>
>> There is nothing said about inheriting state from the parent data node.
>> I think no matter how the permissions are derived, one can
>> find examples that work better or worse because of it.
>>
>> No.  If the rules apply to descendant children, all normal examples work
>> well (including the ones in the appendix).
>>
>>
>> IMO the number of rules required to implement a use-case is not
>> very relevant or objective criteria.
>>
>> Yes it is, particularly when the difference is between needing a 1 line
>> rule, and a 100+ line rule.
>>
>>
>> Using the previous example of /home and /home/user1,
>> if the user1 is given read access to /home, then (according to you)
>> it also has read access to every user subtree under /home.
>>
>> Instead of 1 rule per user, 2 rules are needed
>>
>> No, just 1 rule per user:
>>    read-default=deny
>>    group=user1, path=/home/user1, action=permit
>>
>> This is because of my two proposed changes:
>>
>> (i) that a data-node path match succeeds if it matches the path prefix
>> from the root of the tree.  I.e. so the data-rule "/foo" matches "/foo" and
>> all of foo's descendant children nodes.
>> <- This means that you only need 1 entry instead of 100 entries.
>>
>> (ii) if a data-node rule has action "permit" then it implicitly allows
>> read access for all ancestor parent nodes up to the root.  (I.e. to
>> mitigate the original change proposed on this thread.)
>> <- This means that you don't need a separate read rule for "/home".  Read
>> access to that node it is implicitly given via "group=user1,
>> path=/home/user1, action=permit", hence meaning that the rule works the
>> same way as it does on an rfc6536 compliant implementation.
>>
>>
>>    read-default=deny
>>    group=*, path=/home, action=permit
>>    group=user1, path=/home/user1, action=permit
>>
>> The above rules would allow access for every user to every other user.
>> The 2nd rule has no effect, which is counter-intuitive.
>> Every user dir would need 2 rules
>>
>>    read-default=deny
>>    group=*, path=/home, action=permit
>>    group=user1, path=/home/user1, action=permit
>>    group=*, path=/home/user1, action=deny
>>
>> There is nothing that says this is how it works.
>>> If it did, once could never have privileged sub-fiolders
>>>
>>>      /var/log -> permit
>>>      /var/log/apache2  -> deny
>>>
>>>
>>> Yes, you can, you just list the longest path first in the list of rules:
>>>
>>>  (1)  /var/log/apache2  -> deny
>>>   (2) /var/log -> permit
>>>
>>> Any requests that attempt to access anything under /var/log/apache2
>>> would match rule (1) and be denied.
>>> Any requests that attempt to access anything under /var/log, but not
>>> under /var/log/apache2, would fail to match rule (1), but would match rule
>>> (2) instead and be permitted.
>>>
>>>
>>>
>> I do not see any text in the draft or RFC 7950 that
>> suggests that /var/log and /var/log/apache2 represent the same data node.
>>
>> They are different data nodes, but I don't see how that is relevant.
>>
>> Thanks,
>> Rob
>>
>>
>>
>>
>> Andy
>>
>>
>>
>>>
>>>
>>>
>>>> > (ii) if a data-node rule has action "permit" then it implicitly
>>>> allows read access for all ancestor parent nodes up to the root.  (I.e. to
>>>> mitigate the original change proposed on this thread.)
>>>>
>>>> This seems reasonable to me, although I haven't at this point evaluated
>>>> the suggestion in detail. In any case I think the main point both
>>>> regarding this and the prefix match is that this update to 6536 can't
>>>> make radical changes to the semantics compared to a "reasonable
>>>> interpretation" (hard to define, I know) of the under-specified
>>>> original.
>>>>
>>>
>>> The text does not say this at all so I do not approve of this change
>>>
>>>
>>> In the RFC version of the NACM this wasn't required because operation
>>> 'none' didn't require read access.  Now read access is required even for
>>> operation 'none', then this change makes sense to make the NACM changes
>>> backwards compatible, whilst still closing the security hole.
>>>
>>> Thanks,
>>> Rob
>>>
>>>
>>>
>>>
>>>>
>>>> --Per
>>>>
>>>>
>>>
>>> Andy
>>>
>>>
>>>> > Thanks,
>>>> > Rob
>>>> >
>>>> >
>>>> >>
>>>> >> Thanks
>>>> >>
>>>> >>>
>>>> >>>
>>>> >>> Andy
>>>> >>>
>>>> >>>
>>>> >>> On Thu, Nov 9, 2017 at 2:44 AM, Robert Wilton <[e-mail]
>>>> <mailto:[e-mail]>> wrote:
>>>> >>>
>>>> >>>     Hi Andy,
>>>> >>>
>>>> >>>     It isn't clear to me whether matching a path in NACM either:
>>>> >>>       (i) Only applies to the specific node, and not any children,
>>>> or
>>>> >>>       (ii) Applies to the specific node and all descendant children
>>>> nodes as well.
>>>> >>>
>>>> >>>     As an example, using the tree below.  if I have a rule that
>>>> matches path "A/B" then does that apply to only the specific node "A/B", or
>>>> does it also apply to all descendant children of "A/B" as
>>>> >>>     well?
>>>> >>>
>>>> >>>     In rfc6536bis-08, section "3.4.5.  Data Node Access
>>>> Validation", step 6 states:
>>>> >>>
>>>> >>>             *  The rule does not have a "rule-type" defined or the
>>>> "rule-
>>>> >>>                type" is "data-node" and the *"path" matches the
>>>> requested data node*, action node, or notification node.
>>>> >>>
>>>> >>>
>>>> >>>     My reading of this is that it implies that the interpretation
>>>> of the path rule is (i), but this is not how I would normally expect an ACL
>>>> rule to apply in a tree like object (e.g a directory
>>>> >>>     file system).
>>>> >>>
>>>> >>>     However, the examples in Appendix B.4. imply that the path rule
>>>> is to be interpreted like (ii), or otherwise the example rules seem to be
>>>> mostly pointless.
>>>> >>>
>>>> >>>     E.g. taking this example from appendix B.4:
>>>> >>>
>>>> >>>            <rule>
>>>> >>>              <name>permit-dummy-interface</name>
>>>> >>>              <path xmlns:acme="http://example.com/ns/itf" <
>>>> http://example.com/ns/itf>>
>>>> >>>                /acme:interfaces/acme:interface[acme:name='dummy']
>>>> >>>              </path>
>>>> >>>              <access-operations>read update</access-operations>
>>>> >>>              <action>permit</action>
>>>> >>>              <comment>
>>>> >>>                Allow the limited and guest groups read
>>>> >>>                and update access to the dummy interface.
>>>> >>>              </comment>
>>>> >>>            </rule>
>>>> >>>
>>>> >>>
>>>> >>>     If the rule is (i)  then the access rule allows the client to
>>>> read the specific node "/acme:interfaces/acme:interface[acme:name='dummy']"
>>>> but not any child leafs/containers of that interface,
>>>> >>>     this doesn't seem useful.
>>>> >>>
>>>> >>>     Further comments inline below ...
>>>> >>>
>>>> >>>     On 08/11/2017 20:05, Andy Bierman wrote:
>>>> >>>>     Hi,
>>>> >>>>
>>>> >>>>     This change has no impact on the server if /nacm/read-default
>>>> is "permit".
>>>> >>>>     In that case, the extra read rules for /A and /A/B are not
>>>> needed.
>>>> >>>     I agree.
>>>> >>>
>>>> >>>>     An operator worried about read access should set read-default
>>>> to "deny".
>>>> >>>     I agree.  This is the scenario that I'm considering.
>>>> >>>
>>>> >>>>     In that case, explicit rules to read /A and /A/B would be
>>>> needed
>>>> >>>>     in the new NACM.
>>>> >>>     Yes, if the interpretation of the rule is (i) above.
>>>> >>>     Otherwise if the interpretation is (ii) then you only need
>>>> "read /A" since that implies "read A/B" as well (as long as the rules are
>>>> listed in the correct order).
>>>> >>>
>>>> >>>
>>>> >>>>       The deny rules would not be needed.
>>>> >>>     Only if the interpretation of the rule is (i) above.  In which
>>>> case the  "read/write 'A/B/J' rule would not be sufficient.  It would be
>>>> necessary to define an Xpath expressions that contains
>>>> >>>     all children nodes as well.  Perhaps 'A/B/J//*'?
>>>> >>>
>>>> >>>     If the interpretation of the rule is (ii) then you would also
>>>> need all the explicit deny statements as well, otherwise they would be
>>>> allowed by the "read /A" rule above.
>>>> >>>
>>>> >>>     Thanks,
>>>> >>>     Rob
>>>> >>>
>>>> >>>
>>>> >>>>
>>>> >>>>
>>>> >>>>     Andy
>>>> >>>>
>>>> >>>>
>>>> >>>>     On Wed, Nov 8, 2017 at 8:33 AM, Robert Wilton <
>>>> [e-mail] <mailto:[e-mail]>> wrote:
>>>> >>>>
>>>> >>>>         Hi,
>>>> >>>>
>>>> >>>>         I'm not sure about this change.
>>>> >>>>
>>>> >>>>         I'm not that familiar with NACM, but if you want to give a
>>>> particular set of users read/write access to a subtree, but not allow them
>>>> to have any other access to the configuration in the
>>>> >>>>         running datastore then with the existing RFC, that could
>>>> be expressed with a single rule (example in 6536bis, appendix B.4)
>>>> >>>>
>>>> >>>>         With this new change, I think that you may need to
>>>> configure many more rules to achieve the same thing.  I think that you
>>>> would need to give read access to the top node in the desired
>>>> >>>>         path, and then separate explicit "deny" rules for every
>>>> sibling child node walking from the top of the tree down to the data node
>>>> that read/write access is actually being given to.  The
>>>> >>>>         example below may explain my understanding better:
>>>> >>>>
>>>> >>>>         E.g. For a tree of data nodes, rooted at A, if we wanted
>>>> to give read/write access only to "J" subtree, and no access for the rest
>>>> of the tree then:
>>>> >>>>
>>>> >>>>                          A
>>>> >>>>                          |
>>>> >>>>                 --------------------
>>>> >>>>                 |      |     |     |
>>>> >>>>                 B      C     D     E
>>>> >>>>                 |
>>>> >>>>            -----------
>>>> >>>>            |   |  |  |
>>>> >>>>            F   G  H  J
>>>> >>>>                      |
>>>> >>>>                     ...
>>>> >>>>
>>>> >>>>
>>>> >>>>         In the old model, I think that the ACL rules would be 1
>>>> rules long (assuming default deny all):
>>>> >>>>            "read/write 'A/B/J'
>>>> >>>>
>>>> >>>>         In the new model, I think that the equivalent ACL rules
>>>> would need to be 8 rules long (assuming default deny all):
>>>> >>>>            "read/write 'A/B/J'
>>>> >>>>            "read A"
>>>> >>>>            "deny C"
>>>> >>>>            "deny D"
>>>> >>>>            "deny E"
>>>> >>>>            "deny F"
>>>> >>>>            "deny G"
>>>> >>>>            "deny H"
>>>> >>>>
>>>> >>>>         Note, I am assuming that a "path" rule matches for the
>>>> given path and all descendant nodes.  The draft doesn't seem to be
>>>> particularly clear on this point (it states that the rule applies
>>>> >>>>         when the path matches, but this would seem to be counter
>>>> intuitive), and perhaps it could be clarified.
>>>> >>>>
>>>> >>>>         If this change is allowed, then the example in appendix
>>>> B.4 looks like it would need to be fixed, since the "limited-acl" probably
>>>> wouldn't give any access at all, unless default read
>>>> >>>>         access had been given.
>>>> >>>>
>>>> >>>>         But, possibly I'm misunderstanding how this all works!  If
>>>> so, apologies for the noise :-)
>>>> >>>>
>>>> >>>>         Thanks,
>>>> >>>>         Rob
>>>> >>>>
>>>> >>>>
>>>> >>>>         On 02/11/2017 14:18, Benoit Claise wrote:
>>>> >>>>>         Dear all,
>>>> >>>>>
>>>> >>>>>         Here is a major change in draft-ietf-netconf-rfc6536bis,
>>>> suggested by the Security AD Eric Rescola part of the IESG review, which I
>>>> would like to validate with the WG. See
>>>> >>>>>         https://tools.ietf.org/rfcdif
>>>> f?url2=draft-ietf-netconf-rfc6536bis-08.txt <
>>>> https://tools.ietf.org/rfcdiff?url2=draft-ietf-netconf-rfc6
>>>> 536bis-08.txt>
>>>> >>>>>
>>>> >>>>>         <dfpcfioondggippe.png>
>>>> >>>>>         The NETCONF WG was cc'ed for the entire discussion.
>>>> >>>>>         What do you think? I will draw the conclusions by Friday
>>>> Nov 10th.
>>>> >>>>>
>>>> >>>>>         Note: If the WG is fine, the next step is to approve this
>>>> document.
>>>> >>>>>
>>>> >>>>>         Regards, Benoit
>>>> >>>>>
>>>> >>>>>
>>>> >>>>>         _______________________________________________
>>>> >>>>>         Netconf mailing list
>>>> >>>>>         [e-mail] <mailto:[e-mail]>
>>>> >>>>>         https://www.ietf.org/mailman/listinfo/netconf <
>>>> https://www.ietf.org/mailman/listinfo/netconf>
>>>> >>>>
>>>> >>>>
>>>> >>>
>>>> >>>
>>>> >>> _______________________________________________
>>>> >>> Netconf mailing list
>>>> >>> [e-mail] <mailto:[e-mail]>
>>>> >>> https://www.ietf.org/mailman/listinfo/netconf
>>>> >>
>>>> >> Mahesh Jethanandani
>>>> >> [e-mail] <mailto:[e-mail]>
>>>> >
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > Netconf mailing list
>>>> > [e-mail]
>>>> > https://www.ietf.org/mailman/listinfo/netconf
>>>> >
>>>>
>>>>
>>>
>>>
>>
>>
>
>