lib/api: Better handle %s templates in LDAP strings (fixes #9072) (#9155)

Also add some escaping for good measure.

lib/api/api_auth.go

@@ -260,7 +260,8 @@ func authLDAP(username string, password string, cfg config.LDAPConfiguration) bo
 
 	defer connection.Close()
 
-	err = connection.Bind(ldapTemplateBindDN(cfg.BindDN, username), password)
+	bindDN := formatOptionalPercentS(cfg.BindDN, escapeForLDAPDN(username))
+	err = connection.Bind(bindDN, password)
 	if err != nil {
 		l.Warnln("LDAP Bind:", err)
 		return false
@@ -280,7 +281,7 @@ func authLDAP(username string, password string, cfg config.LDAPConfiguration) bo
 	// the user. If this matches precisely one user then we are good to go.
 	// The search filter uses the same %s interpolation as the bind DN.
 
-	searchString := fmt.Sprintf(cfg.SearchFilter, username)
+	searchString := formatOptionalPercentS(cfg.SearchFilter, escapeForLDAPFilter(username))
 	const sizeLimit = 2  // we search for up to two users -- we only want to match one, so getting any number >1 is a failure.
 	const timeLimit = 60 // Search for up to a minute...
 	searchReq := ldap.NewSearchRequest(cfg.SearchBaseDN, ldap.ScopeWholeSubtree, ldap.DerefFindingBaseObj, sizeLimit, timeLimit, false, searchString, nil, nil)
@@ -298,13 +299,37 @@ func authLDAP(username string, password string, cfg config.LDAPConfiguration) bo
 	return true
 }
 
-func ldapTemplateBindDN(bindDN string, username string) string {
-	// Check if formatting directives are included in the ldapTemplateBindDN - if so add username.
-	// (%%s is a literal %s - unlikely for LDAP, but easy to handle here).
-	if strings.Count(bindDN, "%s") != strings.Count(bindDN, "%%s") {
-		bindDN = fmt.Sprintf(bindDN, username)
+// escapeForLDAPFilter escapes a value that will be used in a filter clause
+func escapeForLDAPFilter(value string) string {
+	// https://social.technet.microsoft.com/wiki/contents/articles/5392.active-directory-ldap-syntax-filters.aspx#Special_Characters
+	// Backslash must always be first in the list so we don't double escape them.
+	return escapeRunes(value, []rune{'\\', '*', '(', ')', 0})
+}
+
+// escapeForLDAPDN escapes a value that will be used in a bind DN
+func escapeForLDAPDN(value string) string {
+	// https://social.technet.microsoft.com/wiki/contents/articles/5312.active-directory-characters-to-escape.aspx
+	// Backslash must always be first in the list so we don't double escape them.
+	return escapeRunes(value, []rune{'\\', ',', '#', '+', '<', '>', ';', '"', '=', ' ', 0})
+}
+
+func escapeRunes(value string, runes []rune) string {
+	for _, e := range runes {
+		value = strings.ReplaceAll(value, string(e), fmt.Sprintf("\\%X", e))
+	}
+	return value
+}
+
+func formatOptionalPercentS(template string, username string) string {
+	var replacements []any
+	nReps := strings.Count(template, "%s") - strings.Count(template, "%%s")
+	if nReps < 0 {
+		nReps = 0
+	}
+	for i := 0; i < nReps; i++ {
+		replacements = append(replacements, username)
 	}
-	return bindDN
+	return fmt.Sprintf(template, replacements...)
 }
 
 // Convert an ISO-8859-1 encoded byte string to UTF-8. Works by the

lib/api/api_auth_test.go

@@ -46,22 +46,67 @@ func TestStaticAuthPasswordFail(t *testing.T) {
 	}
 }
 
-func TestAuthLDAPSendsCorrectBindDNWithTemplate(t *testing.T) {
+func TestFormatOptionalPercentS(t *testing.T) {
 	t.Parallel()
 
-	templatedDn := ldapTemplateBindDN("cn=%s,dc=some,dc=example,dc=com", "username")
-	expectedDn := "cn=username,dc=some,dc=example,dc=com"
-	if expectedDn != templatedDn {
-		t.Fatalf("ldapTemplateBindDN should be %s != %s", expectedDn, templatedDn)
+	cases := []struct {
+		template string
+		username string
+		expected string
+	}{
+		{"cn=%s,dc=some,dc=example,dc=com", "username", "cn=username,dc=some,dc=example,dc=com"},
+		{"cn=fixedusername,dc=some,dc=example,dc=com", "username", "cn=fixedusername,dc=some,dc=example,dc=com"},
+		{"cn=%%s,dc=%s,dc=example,dc=com", "username", "cn=%s,dc=username,dc=example,dc=com"},
+		{"cn=%%s,dc=%%s,dc=example,dc=com", "username", "cn=%s,dc=%s,dc=example,dc=com"},
+		{"cn=%s,dc=%s,dc=example,dc=com", "username", "cn=username,dc=username,dc=example,dc=com"},
+	}
+
+	for _, c := range cases {
+		templatedDn := formatOptionalPercentS(c.template, c.username)
+		if c.expected != templatedDn {
+			t.Fatalf("result should be %s != %s", c.expected, templatedDn)
+		}
 	}
 }
 
-func TestAuthLDAPSendsCorrectBindDNWithNoTemplate(t *testing.T) {
+func TestEscapeForLDAPFilter(t *testing.T) {
 	t.Parallel()
 
-	templatedDn := ldapTemplateBindDN("cn=fixedusername,dc=some,dc=example,dc=com", "username")
-	expectedDn := "cn=fixedusername,dc=some,dc=example,dc=com"
-	if expectedDn != templatedDn {
-		t.Fatalf("ldapTemplateBindDN should be %s != %s", expectedDn, templatedDn)
+	cases := []struct {
+		in  string
+		out string
+	}{
+		{"username", `username`},
+		{"user(name", `user\28name`},
+		{"user)name", `user\29name`},
+		{"user\\name", `user\5Cname`},
+		{"user*name", `user\2Aname`},
+		{"*,CN=asdf", `\2A,CN=asdf`},
+	}
+
+	for _, c := range cases {
+		res := escapeForLDAPFilter(c.in)
+		if c.out != res {
+			t.Fatalf("result should be %s != %s", c.out, res)
+		}
+	}
+}
+
+func TestEscapeForLDAPDN(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		in  string
+		out string
+	}{
+		{"username", `username`},
+		{"* ,CN=asdf", `*\20\2CCN\3Dasdf`},
+	}
+
+	for _, c := range cases {
+		res := escapeForLDAPDN(c.in)
+		if c.out != res {
+			t.Fatalf("result should be %s != %s", c.out, res)
+		}
 	}
 }