Qpid
  1. Qpid
  2. QPID-4631

C++ Broker interbroker links should be protected by ACL

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20
    • Fix Version/s: 0.23
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      This issue addresses CVE-2012-4446

      Federated interbroker links may be opened by client programs and not just by brokers. By default the creation of these links is not protected any formal authorization.

      Users concerned about this issue may immediately lock their systems down by creating ACL rules that allow links to be created only by authorized users. For instance the following ACL rules on each broker would provide the lockdown necessary:

      group proxies <id1> <id2> ...
      acl allow proxies create link
      acl deny-log all create link

      A better solution is for the ACL module to deny the creation of links unless ACL rules are specified to specifically allow them.
      In pseudo code the solution is in two parts. Part one observes CREATE LINK rules in the acl file. Part two authorizes link creation only if ACL is loaded, CREATE LINK ACL rules are specified, and the specific user is authorized to create the link in question:

      function readAclFile()
      ...
      if (CREATE LINK rules are specified)
      set acl->createLinkFlag
      endif
      ...
      end function

      function brokerCreateLink()
      if (aclLoaded)
      if (acl->createLinkFlag)
      if (acl->authorise(user, create, link, properties))
      <create link allowed>
      else
      <create link denied - not authorized>
      endif
      else
      <create link denied - acl did not specify a create link rule>
      endif
      else
      <create link denied - acl module not loaded>
      endif
      end function

      This Jira will track the implementation of this restriction.

        Activity

        Hide
        ASF subversion and git services added a comment -

        Commit 1574291 from chug@apache.org in branch 'qpid/trunk'
        [ https://svn.apache.org/r1574291 ]

        QPID-5599: C++ Broker silently ignores --max-connections option when no ACL file is loaded

        Simply installing a null and permissive rule file trips up the 'create link'
        security check. The security check from
        https://issues.apache.org/jira/browse/QPID-4631 reasons that if authentication
        is enabled and no ACL rule file is specified then interbroker links are
        denied. The check for 'ACL rule file is loaded' is simply the existence of
        the ACL object. That check is voided by always having an ACL object regardless
        of whether the ACL rule file was specified or not.

        One fix considered was adding an ACL rule "acl deny-log all create link" to
        the formerly null rule set when no ACL file is specified. This solution has
        too much complexity in several places and is too hard.

        The fix implemented here is a boolean flag indicating if the ACL rule set
        in force is specified by the user or not. Then the security check tests
        that the acl exists (always true) and that the rule set is specified by the
        user.

        Show
        ASF subversion and git services added a comment - Commit 1574291 from chug@apache.org in branch 'qpid/trunk' [ https://svn.apache.org/r1574291 ] QPID-5599 : C++ Broker silently ignores --max-connections option when no ACL file is loaded Simply installing a null and permissive rule file trips up the 'create link' security check. The security check from https://issues.apache.org/jira/browse/QPID-4631 reasons that if authentication is enabled and no ACL rule file is specified then interbroker links are denied. The check for 'ACL rule file is loaded' is simply the existence of the ACL object. That check is voided by always having an ACL object regardless of whether the ACL rule file was specified or not. One fix considered was adding an ACL rule "acl deny-log all create link" to the formerly null rule set when no ACL file is specified. This solution has too much complexity in several places and is too hard. The fix implemented here is a boolean flag indicating if the ACL rule set in force is specified by the user or not. Then the security check tests that the acl exists (always true) and that the rule set is specified by the user.
        Hide
        Justin Ross added a comment -
        Show
        Justin Ross added a comment - Released in Qpid 0.24, http://qpid.apache.org/releases/qpid-0.24/index.html
        Hide
        Chuck Rolke added a comment -

        Fixed at Committed revision 1477112.

        Show
        Chuck Rolke added a comment - Fixed at Committed revision 1477112.
        Hide
        Chuck Rolke added a comment -
        I suggest we add the new requirements only if auth=yes.
        

        This is a great suggestion and I've put it in for the latest review. Particularly it solves getting all broker features without specifying an ACL, which is a performance drain.

        https://reviews.apache.org/r/10658/

        Show
        Chuck Rolke added a comment - I suggest we add the new requirements only if auth=yes. This is a great suggestion and I've put it in for the latest review. Particularly it solves getting all broker features without specifying an ACL, which is a performance drain. https://reviews.apache.org/r/10658/
        Hide
        Alan Conway added a comment -

        What is the motivation for this change? Many applications (not least most of our tests) live on trusted networks and do not require security or need/want to be encumbered with security related config. We should not require an ACL file simply to use links or any other feature. I suggest we add the new requirements only if auth=yes. That makes them required by default (auth=yes) but easy to opt-out by setting auth=no.

        Show
        Alan Conway added a comment - What is the motivation for this change? Many applications (not least most of our tests) live on trusted networks and do not require security or need/want to be encumbered with security related config. We should not require an ACL file simply to use links or any other feature. I suggest we add the new requirements only if auth=yes. That makes them required by default (auth=yes) but easy to opt-out by setting auth=no.
        Hide
        Gordon Sim added a comment -

        So the issue is that for 'allow all' style ACLs the 'all' includes creating links but the user may not realise that? Is an acl now required for creating links (i.e. if you have no acl file specified at all, does that disable all link creation)?

        Show
        Gordon Sim added a comment - So the issue is that for 'allow all' style ACLs the 'all' includes creating links but the user may not realise that? Is an acl now required for creating links (i.e. if you have no acl file specified at all, does that disable all link creation)?
        Hide
        Chuck Rolke added a comment -

        To your question: Yes, a 'deny all all' rule prevents creation of a federation link by anyone not explicitly granted that permission.

        What we are trying to prevent is users innocently allowing link creation in the absence of a CREATE LINK rule granting that permission.

        Prior to this patch link creation is allowed if no ACL file is loaded. Also, if an ACL file is loaded then link creation is allowed by the usual ACL rules even though a CREATE LINK rule does not exist.

        In this patch a rule like 'allow all all' or 'deny all all' does not contribute to the presence of a CREATE LINK rule.

        The patch forces the create link path in the broker to refer to the ACL file for a decision and that the ACL file has at least one explicit CREATE LINK rule. If links are allowed then the customer's rule set explicitly allowed them and link creation did not happen through a passive ACL approval.

        Show
        Chuck Rolke added a comment - To your question: Yes, a 'deny all all' rule prevents creation of a federation link by anyone not explicitly granted that permission. What we are trying to prevent is users innocently allowing link creation in the absence of a CREATE LINK rule granting that permission. Prior to this patch link creation is allowed if no ACL file is loaded. Also, if an ACL file is loaded then link creation is allowed by the usual ACL rules even though a CREATE LINK rule does not exist. In this patch a rule like 'allow all all' or 'deny all all' does not contribute to the presence of a CREATE LINK rule. The patch forces the create link path in the broker to refer to the ACL file for a decision and that the ACL file has at least one explicit CREATE LINK rule. If links are allowed then the customer's rule set explicitly allowed them and link creation did not happen through a passive ACL approval.
        Hide
        Gordon Sim added a comment -

        I'm not sure I understand this issue correctly. Does a deny all all rule not prevent the creation of a federation link by anyone not explicitly granted that permission?

        Show
        Gordon Sim added a comment - I'm not sure I understand this issue correctly. Does a deny all all rule not prevent the creation of a federation link by anyone not explicitly granted that permission?
        Hide
        Chuck Rolke added a comment -

        Work on this issue is in qpid/branches/qpid-4631

        The acl and broker code are complete and checked in
        http://svn.apache.org/viewvc?view=revision&revision=r1459822
        This includes fixes for ha_tests(ha_test.py).

        Code for self tests federation_tests and federation_sys_tests is in
        http://svn.apache.org/viewvc?view=revision&revision=r1459854

        Scanning the sources of cpp/src/tests it looks like these files need some Acl help to make the work properly:
        cli_tests.py
        federated_topic_test
        ipv6_test
        sasl_fed
        sasl_fed_ex
        sasl_fed_ex_dynamic
        sasl_fed_ex_link
        sasl_fed_ex_queue
        sasl_fed_ex_route
        run_federation_tests.ps1
        run_ha_tests
        run_headers_federation_tests

        Show
        Chuck Rolke added a comment - Work on this issue is in qpid/branches/qpid-4631 The acl and broker code are complete and checked in http://svn.apache.org/viewvc?view=revision&revision=r1459822 This includes fixes for ha_tests(ha_test.py). Code for self tests federation_tests and federation_sys_tests is in http://svn.apache.org/viewvc?view=revision&revision=r1459854 Scanning the sources of cpp/src/tests it looks like these files need some Acl help to make the work properly: cli_tests.py federated_topic_test ipv6_test sasl_fed sasl_fed_ex sasl_fed_ex_dynamic sasl_fed_ex_link sasl_fed_ex_queue sasl_fed_ex_route run_federation_tests.ps1 run_ha_tests run_headers_federation_tests

          People

          • Assignee:
            Chuck Rolke
            Reporter:
            Chuck Rolke
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development