mirror of
https://github.com/penpot/penpot.git
synced 2026-08-06 21:08:34 +00:00
🐛 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
This commit is contained in:
Andrey Antukh
parent
636bc22cc4
commit
96f6a295e7
@ -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 \
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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}
|
||||
|
||||
76
backend/test/backend_tests/auth_ldap_test.clj
Normal file
76
backend/test/backend_tests/auth_ldap_test.clj
Normal file
@ -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"))))))
|
||||
106
backend/test/e2e/ldap-injection.test.mjs
Normal file
106
backend/test/e2e/ldap-injection.test.mjs
Normal file
@ -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"
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
x
Reference in New Issue
Block a user