QPID-3528

sasl_set_path() does no check on the given directory, so when you get bad behavior
later it can be hard to track down.  Especially bad is its policy of defaulting to
an alternate standard location if yours fails.  That's a potential security bug.
So this patch checks that your dir exists, and is readable, before calling
sasl_set_path().  Either you get the sasl config dir you were expecting,



git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk/qpid@1183121 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 247 insertions, 5 deletions

diff --git a/cpp/src/qpid/broker/SaslAuthenticator.cpp b/cpp/src/qpid/broker/SaslAuthenticator.cpp
index befe7eef25..d7adbd68ab 100644
--- a/cpp/src/qpid/broker/SaslAuthenticator.cpp
+++ b/cpp/src/qpid/broker/SaslAuthenticator.cpp
@@ -30,6 +30,7 @@
 #include <boost/format.hpp>
 
 #if HAVE_SASL
+#include <sys/stat.h>
 #include <sasl/sasl.h>
 #include "qpid/sys/cyrus/CyrusSecurityLayer.h"
 using qpid::sys::cyrus::CyrusSecurityLayer;
@@ -98,11 +99,33 @@ void SaslAuthenticator::init(const std::string& saslName, std::string const & sa
     //  Check if we have a version of SASL that supports sasl_set_path()
 #if (SASL_VERSION_FULL >= ((2<<16)|(1<<8)|22))
     //  If we are not given a sasl path, do nothing and allow the default to be used.
-    if ( ! saslConfigPath.empty() ) {
-        int code = sasl_set_path(SASL_PATH_TYPE_CONFIG,
-                                 const_cast<char *>(saslConfigPath.c_str()));
+    if ( saslConfigPath.empty() ) {
+        QPID_LOG ( info, "SASL: no config path set - using default." );
+    }
+    else {
+        struct stat st;
+
+        // Make sure the directory exists and we can read up to it.
+        if ( ::stat ( saslConfigPath.c_str(), & st) ) {
+          // Note: not using strerror() here because I think its messages are a little too hazy.
+          if ( errno == ENOENT )
+              throw Exception ( QPID_MSG ( "SASL: sasl_set_path failed: no such directory: " << saslConfigPath ) );
+          if ( errno == EACCES )
+              throw Exception ( QPID_MSG ( "SASL: sasl_set_path failed: cannot read parent of: " << saslConfigPath ) );
+          // catch-all stat failure
+          throw Exception ( QPID_MSG ( "SASL: sasl_set_path failed: cannot stat: " << saslConfigPath ) );
+        }
+
+        // Make sure the directory is readable.
+        if ( ::access ( saslConfigPath.c_str(), R_OK ) ) {
+            throw Exception ( QPID_MSG ( "SASL: sasl_set_path failed: directory not readable:" << saslConfigPath ) );
+        }
+
+        // This shouldn't fail now, but check anyway.
+        int code = sasl_set_path(SASL_PATH_TYPE_CONFIG, const_cast<char *>(saslConfigPath.c_str()));
         if(SASL_OK != code)
             throw Exception(QPID_MSG("SASL: sasl_set_path failed [" << code << "] " ));
+
         QPID_LOG(info, "SASL: config path set to " << saslConfigPath );
     }
 #endif
diff --git a/cpp/src/tests/sasl.mk b/cpp/src/tests/sasl.mk
index 20eaa7c7a5..69b24c3f8a 100644
--- a/cpp/src/tests/sasl.mk
+++ b/cpp/src/tests/sasl.mk
@@ -30,7 +30,7 @@ check_PROGRAMS+=sasl_version
 sasl_version_SOURCES=sasl_version.cpp
 sasl_version_LDADD=$(lib_client)
 
-TESTS += run_cluster_authentication_test sasl_fed sasl_fed_ex_dynamic sasl_fed_ex_link sasl_fed_ex_queue sasl_fed_ex_route sasl_fed_ex_route_cluster sasl_fed_ex_link_cluster sasl_fed_ex_queue_cluster sasl_fed_ex_dynamic_cluster
+TESTS += run_cluster_authentication_test sasl_fed sasl_fed_ex_dynamic sasl_fed_ex_link sasl_fed_ex_queue sasl_fed_ex_route sasl_fed_ex_route_cluster sasl_fed_ex_link_cluster sasl_fed_ex_queue_cluster sasl_fed_ex_dynamic_cluster sasl_no_dir
 LONG_TESTS += run_cluster_authentication_soak
 EXTRA_DIST += run_cluster_authentication_test \
               sasl_fed                        \
@@ -43,7 +43,8 @@ EXTRA_DIST += run_cluster_authentication_test \
               sasl_fed_ex_dynamic_cluster     \
               sasl_fed_ex_link_cluster        \
               sasl_fed_ex_queue_cluster       \
-              sasl_fed_ex_route_cluster
+              sasl_fed_ex_route_cluster       \
+              sasl_no_dir
 
 
 endif # HAVE_SASL
diff --git a/cpp/src/tests/sasl_no_dir b/cpp/src/tests/sasl_no_dir
new file mode 100755
index 0000000000..15a36014bb
--- /dev/null
+++ b/cpp/src/tests/sasl_no_dir
@@ -0,0 +1,218 @@
+#! /bin/bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+# 
+#   http://www.apache.org/licenses/LICENSE-2.0
+# 
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+source ./test_env.sh
+
+script_name=`basename $0`
+
+# This minimum value corresponds to sasl version 2.1.22
+minimum_sasl_version=131350
+
+sasl_version=`$QPID_TEST_EXEC_DIR/sasl_version`
+
+# This test is necessary because this sasl version is the first one that permits 
+# redirection of the sasl config file path.
+if [ "$sasl_version" -lt  "$minimum_sasl_version" ]; then
+  echo "sasl_fed: must have sasl version 2.1.22 or greater.  ( Integer value: $minimum_sasl_version )  Version is: $sasl_version"
+  exit 0
+fi
+
+
+sasl_config_dir=$builddir/sasl_config
+
+
+# Debugging print. --------------------------
+debug=
+function print {
+  if [ "$debug" ]; then
+    echo "${script_name}: $1"
+  fi
+}
+
+
+my_random_number=$RANDOM
+tmp_root=/tmp/sasl_fed_$my_random_number
+mkdir -p $tmp_root
+
+
+LOG_FILE=$tmp_root/qpidd.log
+
+# If you want to see this test fail, just comment out this 'mv' command.
+print "Moving sasl configuration dir."
+mv ${sasl_config_dir} ${sasl_config_dir}-
+
+
+#--------------------------------------------------
+print " Starting broker"
+#--------------------------------------------------
+$QPIDD_EXEC                                  \
+  -p 0                                       \
+  --no-data-dir                              \
+  --auth=yes                                 \
+  --mgmt-enable=yes                          \
+  --log-enable info+                         \
+  --log-source yes                           \
+  --log-to-file ${LOG_FILE}                  \
+  --sasl-config=$sasl_config_dir             \
+  -d 2> /dev/null  1> $tmp_root/broker_port
+
+
+
+# If it works right, the output will look something like this:  ( two lines long )
+# Daemon startup failed: SASL: sasl_set_path failed: no such directory: /home/mick/trunk/qpid/cpp/src/tests/sasl_config (qpid/broker/SaslAuthenticator.cpp:112)
+# 2011-10-13 14:07:00 critical qpidd.cpp:83: Unexpected error: Daemon startup failed: SASL: sasl_set_path failed: no such directory: /home/mick/trunk/qpid/cpp/src/tests/sasl_config (qpid/broker/SaslAuthenticator.cpp:112)
+
+result=`cat ${LOG_FILE} | grep "sasl_set_path failed: no such directory" | wc -l `
+
+#--------------------------------------------------
+print "Restore the Sasl config dir to its original place."
+#--------------------------------------------------
+mv ${sasl_config_dir}- ${sasl_config_dir}
+
+if [ "2" -eq ${result} ]; then
+  print "result: success"
+  rm -rf $tmp_root
+  exit 0
+fi
+
+
+# If this test fails, the broker is still alive.
+# Kill it.
+broker_port=`cat $tmp_root/broker_port`
+#--------------------------------------------------
+print "Asking broker to quit."
+#--------------------------------------------------
+$QPIDD_EXEC --port $broker_port --quit
+
+rm -rf $tmp_root
+
+print "result: fail"
+exit 1
+
+#! /bin/bash
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+# 
+#   http://www.apache.org/licenses/LICENSE-2.0
+# 
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+source ./test_env.sh
+
+script_name=`basename $0`
+
+# This minimum value corresponds to sasl version 2.1.22
+minimum_sasl_version=131350
+
+sasl_version=`$QPID_TEST_EXEC_DIR/sasl_version`
+
+# This test is necessary because this sasl version is the first one that permits 
+# redirection of the sasl config file path.
+if [ "$sasl_version" -lt  "$minimum_sasl_version" ]; then
+  echo "sasl_fed: must have sasl version 2.1.22 or greater.  ( Integer value: $minimum_sasl_version )  Version is: $sasl_version"
+  exit 0
+fi
+
+
+sasl_config_dir=$builddir/sasl_config
+
+
+# Debugging print. --------------------------
+debug=
+function print {
+  if [ "$debug" ]; then
+    echo "${script_name}: $1"
+  fi
+}
+
+
+my_random_number=$RANDOM
+tmp_root=/tmp/sasl_fed_$my_random_number
+mkdir -p $tmp_root
+
+
+LOG_FILE=$tmp_root/qpidd.log
+
+# If you want to see this test fail, just comment out this 'mv' command.
+print "Moving sasl configuration dir."
+mv ${sasl_config_dir} ${sasl_config_dir}-
+
+
+#--------------------------------------------------
+print " Starting broker"
+#--------------------------------------------------
+$QPIDD_EXEC                                  \
+  -p 0                                       \
+  --no-data-dir                              \
+  --auth=yes                                 \
+  --mgmt-enable=yes                          \
+  --log-enable info+                         \
+  --log-source yes                           \
+  --log-to-file ${LOG_FILE}                  \
+  --sasl-config=$sasl_config_dir             \
+  -d 2> /dev/null  1> $tmp_root/broker_port
+
+
+
+# If it works right, the output will look something like this:  ( two lines long )
+# Daemon startup failed: SASL: sasl_set_path failed: no such directory: /home/mick/trunk/qpid/cpp/src/tests/sasl_config (qpid/broker/SaslAuthenticator.cpp:112)
+# 2011-10-13 14:07:00 critical qpidd.cpp:83: Unexpected error: Daemon startup failed: SASL: sasl_set_path failed: no such directory: /home/mick/trunk/qpid/cpp/src/tests/sasl_config (qpid/broker/SaslAuthenticator.cpp:112)
+
+result=`cat ${LOG_FILE} | grep "sasl_set_path failed: no such directory" | wc -l `
+
+#--------------------------------------------------
+print "Restore the Sasl config dir to its original place."
+#--------------------------------------------------
+mv ${sasl_config_dir}- ${sasl_config_dir}
+
+if [ "2" -eq ${result} ]; then
+  print "result: success"
+  rm -rf $tmp_root
+  exit 0
+fi
+
+
+# If this test fails, the broker is still alive.
+# Kill it.
+broker_port=`cat $tmp_root/broker_port`
+#--------------------------------------------------
+print "Asking broker to quit."
+#--------------------------------------------------
+$QPIDD_EXEC --port $broker_port --quit
+
+rm -rf $tmp_root
+
+print "result: fail"
+exit 1
+