Qpid
  1. Qpid
  2. QPID-3528

qpid --help has wrong description of sasl-config parameter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.10
    • Fix Version/s: 0.13
    • Component/s: C++ Broker
    • Labels:

      Description

      Description of problem:
      qpidd --help shows sasl-config option to specify filename of SASL config file.
      That is wrong as it specifies directory (like /etc/sasl2) where qpidd.conf
      for SASL lies.
      man pages of qpid are correct:
      --sasl-config DIR
      gets sasl config info from nonstandard location

      Just qpidd --help is wrong.

      Version-Release number of selected component (if applicable):
      any (MRG 2.0 checked)

      How reproducible:
      100%

      Steps to Reproduce:
      1. qpidd --help | grep -A1 sasl

      Actual results:

      1. qpidd --help | grep -A1 sasl
        --sasl-config FILE gets sasl config from
        nonstandard location
        #

      Expected results:

      1. qpidd --help | grep -A1 sasl
        --sasl-config DIR (/etc/sasl2) gets sasl config from
        nonstandard directory
        #

      Additional info:
      Patch attached.

        Activity

        Justin Ross made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Ted Ross made changes -
        Fix Version/s 0.13 [ 12316854 ]
        Fix Version/s Future [ 12315490 ]
        michael j. goulish made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s Future [ 12315490 ]
        Resolution Fixed [ 1 ]
        Hide
        michael j. goulish added a comment -

        "Fixed" – sort of – in r1183121 .

        Well – actually, the most offensive word in the help message – "FILE" that should have been "DIR" – was fixed earlier.

        But this JIRA made me realize that there was what I consider to be a serious security flaw here, which I fixed. ( I probably should have made a new JIRA... )

        The SASL library call sasl_set_path(), which is a recent addition to the library, does not check the validity of the path when it is called. If you give it a bad path, or one for which you have insufficient permissions, then the library will discover this later, and will then use the default location.

        That's a gross security hole. That library should not default to anything. It should either use your intended SASL db, or fail noisily. We should never have a situation where a production user of our system starts up with a set of SASL usernames and passwords that is not what he expects.

        The code that I put in before the sasl_set_path() call has that effect. It checks for existence and accessibility of the given directory – and if it fails it will prevent broker start-up.

        Show
        michael j. goulish added a comment - "Fixed" – sort of – in r1183121 . Well – actually, the most offensive word in the help message – "FILE" that should have been "DIR" – was fixed earlier. But this JIRA made me realize that there was what I consider to be a serious security flaw here, which I fixed. ( I probably should have made a new JIRA... ) The SASL library call sasl_set_path(), which is a recent addition to the library, does not check the validity of the path when it is called. If you give it a bad path, or one for which you have insufficient permissions, then the library will discover this later, and will then use the default location. That's a gross security hole. That library should not default to anything. It should either use your intended SASL db, or fail noisily. We should never have a situation where a production user of our system starts up with a set of SASL usernames and passwords that is not what he expects. The code that I put in before the sasl_set_path() call has that effect. It checks for existence and accessibility of the given directory – and if it fails it will prevent broker start-up.
        Hide
        Gordon Sim added a comment -

        Fraser Adams also suggested on the user list that more helpful logging would be useful here. I don't know what we can do in terms of logging what cyrus-sasl actually does. The log statement at present says 'SASL: config path set to xyz' regardless of whether the path is valid or not. At the very least changing that to say e.g 'Added xyz to SASL path' might make what is actually happening a little clearer.

        Show
        Gordon Sim added a comment - Fraser Adams also suggested on the user list that more helpful logging would be useful here. I don't know what we can do in terms of logging what cyrus-sasl actually does. The log statement at present says 'SASL: config path set to xyz' regardless of whether the path is valid or not. At the very least changing that to say e.g 'Added xyz to SASL path' might make what is actually happening a little clearer.
        Gordon Sim made changes -
        Assignee michael j. goulish [ mgoulish2 ]
        Hide
        Gordon Sim added a comment -

        This was already raised and fixed as QPID-3117, however the patch attached here is I think a little more complete and the extra changes would be good to pick up.

        Show
        Gordon Sim added a comment - This was already raised and fixed as QPID-3117 , however the patch attached here is I think a little more complete and the extra changes would be good to pick up.
        Pavel Moravec made changes -
        Field Original Value New Value
        Attachment saslconfig-help.patch [ 12497981 ]
        Hide
        Pavel Moravec added a comment -

        Patch proposal. It fixes the help string and also tells what the default
        directory is (/etc/sasl2).

        Show
        Pavel Moravec added a comment - Patch proposal. It fixes the help string and also tells what the default directory is (/etc/sasl2).
        Pavel Moravec created issue -

          People

          • Assignee:
            michael j. goulish
            Reporter:
            Pavel Moravec
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development