From 37f3c64d38c2316fb38675e1ecc187e6e46fb98d Mon Sep 17 00:00:00 2001 From: Janne Koschinski Date: Tue, 21 May 2019 00:32:05 +0200 Subject: [PATCH] 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 Co-authored-by: Manuel Nickschas --- src/core/CMakeLists.txt | 1 + src/core/ldapauthenticator.cpp | 3 ++- src/core/ldapescaper.cpp | 42 ++++++++++++++++++++++++++++++ src/core/ldapescaper.h | 38 +++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/core/CMakeLists.txt | 1 + tests/core/ldapescapetest.cpp | 47 ++++++++++++++++++++++++++++++++++ 7 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 src/core/ldapescaper.cpp create mode 100644 src/core/ldapescaper.h create mode 100644 tests/core/CMakeLists.txt create mode 100644 tests/core/ldapescapetest.cpp diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt index dd1ddc71..ebcb5bce 100644 --- 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 0eedc3c9..ade3f42d 100644 --- 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 index 00000000..fdf0a183 --- /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 index 00000000..57f273a3 --- /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 + +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 e4717b2d..a1916ca4 100644 --- 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 index 00000000..0334eb0d --- /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 index 00000000..9d11be56 --- /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")); +} -- 2.20.1