Repository: syncthing/syncthing (language: go)

Issue:
Omitting %s from LDAP search filter results in corrupt search filter
## Syncthing Version

Syncthing version: v1.23.7, AMD64 Docker Container

## What Happened

This seems similar to #8899, but for the LDAP search filter.
Configuring an LDAP search filter without the %s placeholder results in a warning message in the log file and a corrupt search filter sent to the LDAP server. Logging in with LDAP fails. I could not find the search filter in the LDAP log files though, but that might be due to the behavior of my LDAP server when receiving a corrupt search filter.

If %s is used in LDAP search filter, everything works.

Does __not__ work:
```
(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com)
```
Message in Syncthing log reads
```
WARNING: LDAP Search: LDAP Result Code 201 "Filter Compile Error": ldap: finished compiling filter with extra at end: %!(EXTRA string=myUser)
```

__Works__:
```
(&(sAMAccountName=%s)(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com))
```

## What Is Expected

If %s is omitted from LDAP search filter, send it as configured without any processing.

## Reproduction
  - Configure an LDAP search filter without a %s placeholder
  - Enable LDAP auth
  - Login fails, find warning in Syncthing logs, possibly in LDAP logs depending on LDAP server



Discussion / hints:
While I can agree this could behave better, `(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com)` isn't a useful search filter -- it just says that there should exist a member of that group, but states nothing about the relation between the group and the user logging in. What's the point?
My scenario is as follows:
LDAP users that are not explicitly granted search privileges cannot see other users. 
Since Syncthing performs the search with the user trying to log in instead of an explicit search bind DN, that user can only see itself. So the search filter is only there to ensure correct group membership.
Also, users can bind with user name or mail address, so users can choose whether to log in by user or mail.

The obvious workaround is to use a filter like 
```(&(|(mail=%s)(cn=%s))(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com))``` which works just fine, but given the circumstances overcomplicates things.
I figured I'd report that abnormal behavior anyway.
> The obvious workaround is to use a filter like
```(&(|(mail=%s)(cn=%s))(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com))``` which works just fine

Correction: It seems that multiple uses of ```%s``` are not supported in the filter, but it must occur exactly once. All following ```%s``` are replaced with ```%!s(MISSING)```, resulting in a corrupt filter:
```(&(|(mail=myuser@domain.com)(cn=%!s(MISSING)))(memberOf=CN=Syncthing,CN=Users,DC=example,DC=com))```

Fix patch:
diff --git a/lib/api/api_auth.go b/lib/api/api_auth.go
index 73560a60d03..ee3bb88e6e1 100644
--- a/lib/api/api_auth.go
+++ b/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


Test patch:
diff --git a/lib/api/api_auth_test.go b/lib/api/api_auth_test.go
index ed3677eeb25..e4e207a09a5 100644
--- a/lib/api/api_auth_test.go
+++ b/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)
+		}
 	}
 }