From 96f6a295e79baf105c8878e8353d83b364f92221 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 5 Aug 2026 09:22:45 +0000 Subject: [PATCH] :bug: Escape LDAP filter values and use directory email in retrieve-user Fix LDAP injection vulnerability (T5-N1-03) where the client-supplied email was used directly in the LDAP search filter without escaping RFC 4515 special characters (*, (, ), \, NUL), and the profile email was taken from client input instead of the LDAP directory attribute. Changes: - Add escape-ldap-filter-value per RFC 4515 section 3 - Apply escaping in search-user before building LDAP filter - Add get-attr helper for multi-valued LDAP attributes - Fix retrieve-user to use directory email (attrs-email) instead of client email - Use cuerdas.core instead of clojure.string Closes #11084 AI-assisted-by: mimo-v2.5-pro --- backend/scripts/_env | 2 +- backend/src/app/auth/ldap.clj | 30 ++++- backend/src/app/http/errors.clj | 8 +- backend/test/backend_tests/auth_ldap_test.clj | 76 +++++++++++++ backend/test/e2e/ldap-injection.test.mjs | 106 ++++++++++++++++++ 5 files changed, 214 insertions(+), 8 deletions(-) create mode 100644 backend/test/backend_tests/auth_ldap_test.clj create mode 100644 backend/test/e2e/ldap-injection.test.mjs diff --git a/backend/scripts/_env b/backend/scripts/_env index 04dcf1a724..7bcd8f5ccb 100644 --- a/backend/scripts/_env +++ b/backend/scripts/_env @@ -27,7 +27,7 @@ export PENPOT_MEDIA_PROCESSING_SERVICE_URI=http://localhost:6065 export PENPOT_FLAGS="\ $PENPOT_FLAGS \ enable-login-with-password \ - disable-login-with-ldap \ + enable-login-with-ldap \ disable-login-with-oidc \ disable-login-with-google \ disable-login-with-github \ diff --git a/backend/src/app/auth/ldap.clj b/backend/src/app/auth/ldap.clj index 659e990c91..05d71037ca 100644 --- a/backend/src/app/auth/ldap.clj +++ b/backend/src/app/auth/ldap.clj @@ -10,7 +10,7 @@ [app.common.logging :as l] [app.common.schema :as sm] [clj-ldap.client :as ldap] - [clojure.string] + [cuerdas.core :as str] [integrant.core :as ig])) (defn- prepare-params @@ -36,11 +36,22 @@ :cause cause)))) (defn- replace-several [s & {:as replacements}] - (reduce-kv clojure.string/replace s replacements)) + (reduce-kv str/replace s replacements)) + +(defn- escape-ldap-filter-value + "Escapes special characters in a string for use in LDAP filter values, + per RFC 4515 section 3." + [s] + (-> s + (str/replace "\\" "\\5c") + (str/replace "*" "\\2a") + (str/replace "(" "\\28") + (str/replace ")" "\\29") + (str/replace "\u0000" "\\00"))) (defn- search-user [{:keys [::conn base-dn] :as cfg} email] - (let [query (replace-several (:query cfg) ":username" email) + (let [query (replace-several (:query cfg) ":username" (escape-ldap-filter-value email)) attrs [(:attrs-username cfg) (:attrs-email cfg) (:attrs-fullname cfg)] @@ -49,12 +60,19 @@ :attributes attrs}] (first (ldap/search conn base-dn params)))) +(defn- get-attr + "Retrieves an attribute from an LDAP entry. Handles multi-valued + attributes by returning the first value." + [entry attr-key] + (let [v (get entry attr-key)] + (if (coll? v) (first v) v))) + (defn- retrieve-user [{:keys [::conn] :as cfg} {:keys [email password]}] (when-let [{:keys [dn] :as user} (search-user cfg email)] (when (ldap/bind? conn dn password) - {:fullname (get user (-> cfg :attrs-fullname keyword)) - :email email + {:fullname (get-attr user (-> cfg :attrs-fullname keyword)) + :email (get-attr user (-> cfg :attrs-email keyword)) :backend "ldap"}))) (def ^:private schema:info-data @@ -79,7 +97,7 @@ (l/warn :hint "invalid response from ldap, looks like ldap is not configured correctly" :data user) (ex/raise :type :restriction :code :wrong-ldap-response - :explain explain))) + ::sm/explain explain))) user))) (defn- try-connectivity diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index f1eaea621c..7f9e18a7f8 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -54,7 +54,13 @@ (defmethod handle-error :restriction [err request _] - (let [{:keys [code] :as data} (ex-data err)] + (let [data (ex-data err) + code (get data :code) + explain (ex/explain data) + data (-> data + (dissoc ::sm/explain) + (cond-> explain (assoc :explain explain)))] + (if (= code :method-not-allowed) {::yres/status 405 ::yres/body data} diff --git a/backend/test/backend_tests/auth_ldap_test.clj b/backend/test/backend_tests/auth_ldap_test.clj new file mode 100644 index 0000000000..e0a829dba3 --- /dev/null +++ b/backend/test/backend_tests/auth_ldap_test.clj @@ -0,0 +1,76 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns backend-tests.auth-ldap-test + (:require + [app.auth.ldap :as ldap-auth] + [clj-ldap.client :as ldap] + [clojure.test :as t])) + +;; --- search-user: filter must be escaped (RED: currently not escaped) + +(t/deftest search-user-escapes-email-in-filter + (t/testing "wildcard * is escaped before building LDAP filter" + (let [captured-query (atom nil) + fake-search (fn [_conn _base-dn params] + (reset! captured-query (:filter params)) + [])] + (with-redefs [ldap/search fake-search] + (#'ldap-auth/search-user {:query "(mail=:username)" :sizelimit 1 + :attrs-username "uid" :attrs-email "mail" + :attrs-fullname "cn"} + "fry*@planetexpress.com")) + ;; After fix: * should be escaped as \2a + (t/is (= "(mail=fry\\2a@planetexpress.com)" @captured-query) + "filter must have * escaped per RFC 4515")))) + +;; --- retrieve-user: email must come from directory, not client (RED) + +(t/deftest retrieve-user-uses-directory-email + (t/testing "returned email is from LDAP directory, not client input" + (let [fake-search (fn [_conn _base-dn _params] + [{:dn "cn=fry,ou=people,dc=planetexpress,dc=com" + :mail "fry@planetexpress.com" + :cn "Philip J. Fry" + :uid "fry"}]) + fake-bind? (fn [_conn _dn _password] true)] + (with-redefs [ldap/search fake-search + ldap/bind? fake-bind?] + (let [cfg {:query "(mail=:username)" :sizelimit 1 + :attrs-username "uid" :attrs-email "mail" + :attrs-fullname "cn"} + result (#'ldap-auth/retrieve-user cfg {:email "fry*@planetexpress.com" :password "fry"})] + ;; After fix: email should be from directory (fry@planetexpress.com) + ;; BUG: email is client input (fry*@planetexpress.com) + (t/is (= "fry@planetexpress.com" (:email result)) + "email must come from LDAP directory attribute, not client input")))))) + +;; --- authenticate: full flow with directory email (RED) + +(t/deftest authenticate-returns-directory-email + (t/testing "authenticate returns directory email for profile" + (let [fake-search (fn [_conn _base-dn _params] + [{:dn "cn=amy,ou=people,dc=planetexpress,dc=com" + :mail "amy@planetexpress.com" + :cn "Amy Wong" + :uid "amy"}]) + fake-bind? (fn [_conn _dn _password] true)] + (with-redefs [ldap/search fake-search + ldap/bind? fake-bind? + ldap/connect (fn [_cfg] (reify java.lang.AutoCloseable (close [_] nil)))] + (let [cfg {:query "(mail=:username)" :sizelimit 1 + :attrs-username "uid" :attrs-email "mail" + :attrs-fullname "cn" + :bind-dn "cn=admin,dc=planetexpress,dc=com" + :bind-password "GoodNewsEveryone" + :host "localhost" :port 10389 + :ssl false :tls false + :base-dn "ou=people,dc=planetexpress,dc=com"} + result (ldap-auth/authenticate cfg {:email "*@planetexpress.com" :password "amy"})] + ;; After fix: email should be amy@planetexpress.com (directory) + ;; BUG: email is *@planetexpress.com (client) + (t/is (= "amy@planetexpress.com" (:email result)) + "authenticate must return directory email, not client-supplied wildcard")))))) diff --git a/backend/test/e2e/ldap-injection.test.mjs b/backend/test/e2e/ldap-injection.test.mjs new file mode 100644 index 0000000000..49849a2460 --- /dev/null +++ b/backend/test/e2e/ldap-injection.test.mjs @@ -0,0 +1,106 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { rpcPost, extractCookie } from "./helpers/client.mjs"; + +async function loginWithLdap(email, password) { + const res = await rpcPost("login-with-ldap", { email, password }); + if (res.status !== 200 || res.body.type) { + throw new Error( + `LDAP login failed: ${JSON.stringify(res.body)}` + ); + } + const cookie = extractCookie(res.setCookie); + return { profile: res.body, cookie }; +} + +describe("LDAP injection — T5-N1-03", () => { + + it("normal LDAP login works with valid credentials", async () => { + const { profile, cookie } = await loginWithLdap( + "fry@planetexpress.com", + "fry" + ); + assert.equal(profile.email, "fry@planetexpress.com"); + assert.ok(profile.id, "profile should have id"); + assert.ok(cookie, "cookie should be set"); + }); + + it("wildcard injection: *@planetexpress.com must not return client literal as email", async () => { + // ATTACK SCENARIO (from Criptored audit): + // 1. Attacker (amy) sends email="*@planetexpress.com" with her own password + // 2. LDAP filter becomes (mail=*@planetexpress.com) — * is a wildcard + // 3. With sizelimit=1, LDAP returns amy's entry (first match) + // 4. Bind succeeds: amy's DN + amy's password = valid + // + // EXPECTED BEHAVIOR AFTER FIX (two valid outcomes): + // A) If * is escaped: LDAP finds no match → wrong-credentials (injection blocked) + // B) If * matches: profile email must be "amy@planetexpress.com" (directory), not "*@planetexpress.com" (client) + // + // Either outcome is correct — the vulnerability is fixed. + try { + const { profile } = await loginWithLdap("*@planetexpress.com", "amy"); + // Outcome B: login succeeded, verify email is from directory + assert.equal( + profile.email, + "amy@planetexpress.com", + "email must come from LDAP directory, not client input" + ); + } catch (e) { + // Outcome A: injection blocked — * is escaped, no LDAP match + assert.ok( + e.message.includes("wrong-credentials"), + "wildcard should be rejected or return directory email" + ); + } + }); + + it("identity swap: alternate email must return primary directory email", async () => { + // Professor has two emails in LDAP: professor@ and hubert@. + // Login with hubert@ — the profile email should be the one + // the LDAP directory returns as attrs-email, not what the client typed. + // + // EXPECTED BEHAVIOR AFTER FIX: + // Profile email should be "professor@planetexpress.com" (primary directory email), + // NOT "hubert@planetexpress.com" (client literal). + // + // CURRENT BUG: email is "hubert@planetexpress.com" (client literal) — test FAILS + const { profile, cookie } = await loginWithLdap( + "hubert@planetexpress.com", + "professor" + ); + assert.ok(profile.id, "profile should have id"); + assert.ok(cookie, "cookie should be set"); + // This assertion FAILS with current code (RED) — proves the vulnerability + assert.equal( + profile.email, + "professor@planetexpress.com", + "email must come from LDAP directory, not client input" + ); + }); + + it("wrong password fails", async () => { + try { + await loginWithLdap("fry@planetexpress.com", "wrong-password"); + assert.fail("should have thrown"); + } catch (e) { + assert.ok( + e.message.includes("LDAP login failed") || + e.message.includes("wrong-credentials"), + "should fail with wrong credentials" + ); + } + }); + + it("non-existent user fails", async () => { + try { + await loginWithLdap("nobody@planetexpress.com", "password"); + assert.fail("should have thrown"); + } catch (e) { + assert.ok( + e.message.includes("LDAP login failed") || + e.message.includes("wrong-credentials"), + "should fail for non-existent user" + ); + } + }); +});