Fix a security issue with LDAP usernames

LDAP usernames are directly concatenated into the filter query,
which opens up the risk of unauthenticated LDAP injection,
potentially allowing to bypass the authentication.

To solve this, apply escaping as per RFC 4515.

Co-authored-by: Shane Synan <digitalcircuit36939@gmail.com>
Co-authored-by: Manuel Nickschas <sputnick@quassel-irc.org>

diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
index dd1ddc7..ebcb5bc 100644 (file)
--- a/src/core/CMakeLists.txt
+++ b/src/core/CMakeLists.txt
@@ -32,6 +32,7 @@ target_sources(${TARGET} PRIVATE
     eventstringifier.cpp
     identserver.cpp
     ircparser.cpp
+    ldapescaper.cpp
     metricsserver.cpp
     netsplit.cpp
     oidentdconfiggenerator.cpp

diff --git a/src/core/ldapauthenticator.cpp b/src/core/ldapauthenticator.cpp
index 0eedc3c..ade3f42 100644 (file)
--- a/src/core/ldapauthenticator.cpp
+++ b/src/core/ldapauthenticator.cpp
@@ -28,6 +28,7 @@
 
 #include "ldapauthenticator.h"
 
+#include "ldapescaper.h"
 #include "network.h"
 #include "quassel.h"
 
@@ -241,7 +242,7 @@ bool LdapAuthenticator::ldapAuth(const QString& username, const QString& passwor
 
     LDAPMessage *msg = nullptr, *entry = nullptr;
 
-    const QByteArray ldapQuery = "(&(" + uidAttribute + '=' + username.toLocal8Bit() + ")" + _filter.toLocal8Bit() + ")";
+    const QByteArray ldapQuery = "(&(" + uidAttribute + '=' + LdapEscaper::escapeQuery(username).toLatin1() + ")" + _filter.toLocal8Bit() + ")";
 
     res = ldap_search_ext_s(_connection,
                             baseDN.constData(),

diff --git a/src/core/ldapescaper.cpp b/src/core/ldapescaper.cpp
new file mode 100644 (file)
index 0000000..fdf0a18
--- /dev/null
+++ b/src/core/ldapescaper.cpp
@@ -0,0 +1,42 @@
+/***************************************************************************
+ *   Copyright (C) 2005-2021 by the Quassel Project                        *
+ *   devel@quassel-irc.org                                                 *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) version 3.                                           *
+ *                                                                         *
+ *   This program is distributed in the hope that it will be useful,       *
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+ *   GNU General Public License for more details.                          *
+ *                                                                         *
+ *   You should have received a copy of the GNU General Public License     *
+ *   along with this program; if not, write to the                         *
+ *   Free Software Foundation, Inc.,                                       *
+ *   51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.         *
+ ***************************************************************************/
+
+#include "ldapescaper.h"
+
+QString LdapEscaper::escapeQuery(const QString& filter)
+{
+    QString result;
+    for (const auto character : filter) {
+        if (character > 0x7F ||
+            character == '*' ||
+            character == '(' ||
+            character == ')' ||
+            character == '\\' ||
+            character == '\0') {
+            for (uint8_t byte: QString(character).toUtf8()) {
+                result += "\\";
+                result += QString::number(byte, 16).rightJustified(2, '0');
+            }
+        } else {
+            result += character;
+        }
+    }
+    return result;
+}

diff --git a/src/core/ldapescaper.h b/src/core/ldapescaper.h
new file mode 100644 (file)
index 0000000..57f273a
--- /dev/null
+++ b/src/core/ldapescaper.h
@@ -0,0 +1,38 @@
+/***************************************************************************
+ *   Copyright (C) 2005-2021 by the Quassel Project                        *
+ *   devel@quassel-irc.org                                                 *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) version 3.                                           *
+ *                                                                         *
+ *   This program is distributed in the hope that it will be useful,       *
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+ *   GNU General Public License for more details.                          *
+ *                                                                         *
+ *   You should have received a copy of the GNU General Public License     *
+ *   along with this program; if not, write to the                         *
+ *   Free Software Foundation, Inc.,                                       *
+ *   51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.         *
+ ***************************************************************************/
+
+#pragma once
+
+#include "core-export.h"
+
+#include <QString>
+
+namespace LdapEscaper {
+
+/**
+ * Escapes a filter or query according to RFC 4515
+ *
+ * @note This does not handle escaping distinguished names (dn).
+ * @param filter
+ * @return
+ */
+CORE_EXPORT QString escapeQuery(const QString& query);
+
+}  // namespace LdapEscaper

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index e4717b2..a1916ca 100644 (file)
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -1 +1,2 @@
 add_subdirectory(common)
+add_subdirectory(core)

diff --git a/tests/core/CMakeLists.txt b/tests/core/CMakeLists.txt
new file mode 100644 (file)
index 0000000..0334eb0
--- /dev/null
+++ b/tests/core/CMakeLists.txt
@@ -0,0 +1 @@
+quassel_add_test(LdapEscapeTest LIBRARIES Quassel::Core)

diff --git a/tests/core/ldapescapetest.cpp b/tests/core/ldapescapetest.cpp
new file mode 100644 (file)
index 0000000..9d11be5
--- /dev/null
+++ b/tests/core/ldapescapetest.cpp
@@ -0,0 +1,47 @@
+/***************************************************************************
+ *   Copyright (C) 2005-2021 by the Quassel Project                        *
+ *   devel@quassel-irc.org                                                 *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) version 3.                                           *
+ *                                                                         *
+ *   This program is distributed in the hope that it will be useful,       *
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+ *   GNU General Public License for more details.                          *
+ *                                                                         *
+ *   You should have received a copy of the GNU General Public License     *
+ *   along with this program; if not, write to the                         *
+ *   Free Software Foundation, Inc.,                                       *
+ *   51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.         *
+ ***************************************************************************/
+
+#include "testglobal.h"
+
+#include "ldapescaper.h"
+
+TEST(LdapEscapeTest, unescaped)
+{
+    EXPECT_EQ("Babs Jensen",
+              LdapEscaper::escapeQuery("Babs Jensen"));
+    EXPECT_EQ("Tim Howes",
+              LdapEscaper::escapeQuery("Tim Howes"));
+    EXPECT_EQ("Babs J\\2a",
+              LdapEscaper::escapeQuery("Babs J*"));
+}
+
+TEST(LdapEscapeTest, escaped)
+{
+    EXPECT_EQ("Parens R Us \\28for all your parenthetical needs\\29",
+              LdapEscaper::escapeQuery("Parens R Us (for all your parenthetical needs)"));
+    EXPECT_EQ("\\2a",
+              LdapEscaper::escapeQuery("*"));
+    EXPECT_EQ("C:\\5cMyFile",
+              LdapEscaper::escapeQuery("C:\\MyFile"));
+    EXPECT_EQ("Lu\\c4\\8di\\c4\\87",
+              LdapEscaper::escapeQuery("Lu\xc4\x8di\xc4\x87"));
+    EXPECT_EQ("\u0004\u0002Hi",
+              LdapEscaper::escapeQuery("\x04\x02\x48\x69"));
+}