CVE-2022-24832: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: GoCD is an open source a continuous delivery server. The bundled gocd-ldap-authentication-plugin included with the GoCD Server fails to correctly escape special characters when using the username to construct LDAP queries. While this does not directly allow arbitrary LDAP data exfiltration, it can allow an existing LDAP-authenticated GoCD user with malicious intent to construct and execute malicious queries, allowing them to deduce facts about other users or entries within the LDAP database (e.g alternate fields, usernames, hashed passwords etc) through brute force mechanisms. This only affects users who have a working LDAP authorization configuration enabled on their GoCD server, and only is exploitable by users authenticating using such an LDAP configuration. This issue has been fixed in GoCD 22.1.0, which is bundled with gocd-ldap-authentication-plugin v2.2.0-144.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 6.8
Published: 2022-04-11T21:15Z
Repository: https://github.com/gocd/gocd-ldap-authentication-plugin
Language: Java
Files changed: src/main/java/cd/go/apacheds/ApacheDsLdapClient.java, src/main/java/cd/go/authentication/ldap/LdapClient.java, src/main/java/cd/go/framework/ldap/JNDILdapClient.java, src/test/java/cd/go/apacheds/ApacheDsLdapClientTest.java
Commit date: 2022-03-06 18:40:29 +0800
Commit message: Escape/encode values when building search filters.

Diff: diff --git a/src/main/java/cd/go/apacheds/ApacheDsLdapClient.java b/src/main/java/cd/go/apacheds/ApacheDsLdapClient.java
index eaeb55f..156425c 100644
--- a/src/main/java/cd/go/apacheds/ApacheDsLdapClient.java
+++ b/src/main/java/cd/go/apacheds/ApacheDsLdapClient.java
@@ -23,10 +23,11 @@ import cd.go.authentication.ldap.model.LdapConfiguration;
 import org.apache.directory.api.ldap.codec.api.LdapApiService;
 import org.apache.directory.api.ldap.codec.api.LdapApiServiceFactory;
 import org.apache.directory.api.ldap.extras.controls.ppolicy_impl.PasswordPolicyDecorator;
 import org.apache.directory.api.ldap.model.entry.Entry;
 import org.apache.directory.api.ldap.model.exception.LdapException;
+import org.apache.directory.api.ldap.model.filter.FilterEncoder;
 import org.apache.directory.api.ldap.model.message.*;
 import org.apache.directory.api.ldap.model.name.Dn;
 import org.apache.directory.ldap.client.api.LdapConnectionConfig;
 import org.apache.directory.ldap.client.api.LdapNetworkConnection;
 import org.apache.directory.ldap.client.template.AbstractPasswordPolicyResponder;
@@ -93,11 +94,11 @@ public class ApacheDsLdapClient implements LdapClient {
             }
         });
     }
 
     @Override
-    public <T> List<T> search(final String filter, final Object[] filterArgs, final Mapper<T> mapper, final int maxResultCount) {
+    public <T> List<T> search(final String filter, final String[] filterArgs, final Mapper<T> mapper, final int maxResultCount) {
         final List<T> searchResults = new ArrayList<>();
         for (String searchBase : ldapConfiguration.getSearchBases()) {
             int resultsToFetch = resultsToFetch(maxResultCount, searchResults.size());
 
             if (resultsToFetch == -1) {
@@ -107,11 +108,11 @@ public class ApacheDsLdapClient implements LdapClient {
             try {
                 final SearchRequest searchRequest = new SearchRequestImpl()
                         .setScope(SearchScope.SUBTREE)
                         .addAttributes("*")
                         .setSizeLimit(resultsToFetch)
-                        .setFilter(format(filter, filterArgs))
+                        .setFilter(FilterEncoder.format(filter, filterArgs))
                         .setTimeLimit(ldapConfiguration.getSearchTimeout())
                         .setBase(new Dn(searchBase));
 
                 searchResults.addAll(ldapConnectionTemplate.search(searchRequest, mapper));
             } catch (LdapException e) {
@@ -122,15 +123,15 @@ public class ApacheDsLdapClient implements LdapClient {
         return searchResults;
     }
 
     @Override
     public void validate() {
-        final String filter = format(ldapConfiguration.getUserSearchFilter(), "test");
+        final String filter = FilterEncoder.format(ldapConfiguration.getUserSearchFilter(), "test");
         ldapConnectionTemplate.searchFirst(ldapConfiguration.getSearchBases().get(0), filter, SearchScope.SUBTREE, entry -> entry);
     }
 
-    public List<Entry> search(final String filter, final Object[] filterArgs, final int maxResultCount) {
+    public List<Entry> search(final String filter, final String[] filterArgs, final int maxResultCount) {
         return search(filter, filterArgs, resultWrapper -> (Entry) resultWrapper.getResult(), maxResultCount);
     }
 
     public <T> List<T> searchGroup(List<String> searchBases, String filter, Mapper<T> mapper) {
         final List<T> searchResults = new ArrayList<>();
diff --git a/src/main/java/cd/go/authentication/ldap/LdapClient.java b/src/main/java/cd/go/authentication/ldap/LdapClient.java
index 5dc2af4..3f05644 100644
--- a/src/main/java/cd/go/authentication/ldap/LdapClient.java
+++ b/src/main/java/cd/go/authentication/ldap/LdapClient.java
@@ -22,9 +22,9 @@ import javax.naming.NamingException;
 import java.util.List;
 
 public interface LdapClient {
     <T> T authenticate(String username, String password, Mapper<T> mapper);
 
-    <T> List<T> search(String userSearchFilter, Object[] filterArgs, Mapper<T> mapper, int maxResult);
+    <T> List<T> search(String userSearchFilter, String[] filterArgs, Mapper<T> mapper, int maxResult);
 
     void validate() throws NamingException;
 }
diff --git a/src/main/java/cd/go/framework/ldap/JNDILdapClient.java b/src/main/java/cd/go/framework/ldap/JNDILdapClient.java
index bf9196f..23363fc 100644
--- a/src/main/java/cd/go/framework/ldap/JNDILdapClient.java
+++ b/src/main/java/cd/go/framework/ldap/JNDILdapClient.java
@@ -69,11 +69,11 @@ public class JNDILdapClient implements LdapClient {
         } finally {
             closeContextSilently(dirContext);
         }
     }
 
-    public <T> List<T> search(String filter, Object[] filterArgs, Mapper<T> mapper, int maxResult) {
+    public <T> List<T> search(String filter, String[] filterArgs, Mapper<T> mapper, int maxResult) {
         List<T> results = new ArrayList<>();
         DirContext dirContext = getDirContext(ldapConfiguration, ldapConfiguration.getManagerDn(), ldapConfiguration.getPassword());
 
         try {
             List<SearchResult> searchResults = search(dirContext, filter, filterArgs, maxResult, false);
diff --git a/src/test/java/cd/go/apacheds/LdapTest.java b/src/test/java/cd/go/apacheds/ApacheDsLdapClientTest.java
similarity index 88%
rename from src/test/java/cd/go/apacheds/LdapTest.java
rename to src/test/java/cd/go/apacheds/ApacheDsLdapClientTest.java
index 8851964..c35c919 100644
--- a/src/test/java/cd/go/apacheds/LdapTest.java
+++ b/src/test/java/cd/go/apacheds/ApacheDsLdapClientTest.java
@@ -36,11 +36,11 @@ import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.*;
 
-class LdapTest {
+class ApacheDsLdapClientTest {
 
     @Test
     void shouldBeAbleToSearchUsers() throws ParseException {
         final LdapConfiguration ldapConfiguration = new LdapConfigurationBuilder()
                 .withSearchTimeout(10)
@@ -63,10 +63,30 @@ class LdapTest {
         assertThat(searchRequest.getSizeLimit()).isEqualTo(1L);
         assertThat(searchRequest.getFilter()).isEqualTo(FilterParser.parse("(uid=foo)"));
         assertThat(searchRequest.getTimeLimit()).isEqualTo(10);
     }
 
+    @Test
+    void shouldEscapeSearchFilterValues() throws ParseException {
+        final LdapConfiguration ldapConfiguration = new LdapConfigurationBuilder()
+                .withSearchTimeout(10)
+                .withSearchBases("ou=foo,dc=bar")
+                .build();
+
+        final LdapConnectionTemplate ldapConnectionTemplate = mock(LdapConnectionTemplate.class);
+        final ApacheDsLdapClient ldap = new ApacheDsLdapClient(ldapConfiguration, ldapConnectionTemplate);
+        final ArgumentCaptor<SearchRequest> argumentCaptor = ArgumentCaptor.forClass(SearchRequestImpl.class);
+
+        when(ldapConnectionTemplate.search(argumentCaptor.capture(), ArgumentMatchers.<EntryMapper<Entry>>any())).thenReturn(Collections.singletonList(new DefaultEntry()));
+
+        String injectionUserName = "*)(objectclass=*";
+        ldap.search("(uid={0})", new String[]{injectionUserName}, 1);
+
+        assertThat(argumentCaptor.getValue().getFilter())
+                .isEqualTo(FilterParser.parse("(uid=\\2A\\29\\28objectclass=\\2A)"));
+    }
+
     @Test
     void shouldAbleToFetchResultsFromMultipleSearchBase() {
         final LdapConfiguration ldapConfiguration = new LdapConfigurationBuilder()
                 .withSearchBases("ou=foo,dc=bar", "ou=baz,dc=bar")
                 .build();

Vulnerable code:     public <T> List<T> search(final String filter, final Object[] filterArgs, final Mapper<T> mapper, final int maxResultCount) {
                        .setFilter(format(filter, filterArgs))
        final String filter = format(ldapConfiguration.getUserSearchFilter(), "test");
    public List<Entry> search(final String filter, final Object[] filterArgs, final int maxResultCount) {
    <T> List<T> search(String userSearchFilter, Object[] filterArgs, Mapper<T> mapper, int maxResult);
    public <T> List<T> search(String filter, Object[] filterArgs, Mapper<T> mapper, int maxResult) {
class LdapTest {
Fixed code: import org.apache.directory.api.ldap.model.filter.FilterEncoder;
    public <T> List<T> search(final String filter, final String[] filterArgs, final Mapper<T> mapper, final int maxResultCount) {
                        .setFilter(FilterEncoder.format(filter, filterArgs))
        final String filter = FilterEncoder.format(ldapConfiguration.getUserSearchFilter(), "test");
    public List<Entry> search(final String filter, final String[] filterArgs, final int maxResultCount) {
    <T> List<T> search(String userSearchFilter, String[] filterArgs, Mapper<T> mapper, int maxResult);
    public <T> List<T> search(String filter, String[] filterArgs, Mapper<T> mapper, int maxResult) {
class ApacheDsLdapClientTest {
    @Test
    void shouldEscapeSearchFilterValues() throws ParseException {
        final LdapConfiguration ldapConfiguration = new LdapConfigurationBuilder()
                .withSearchTimeout(10)
                .withSearchBases("ou=foo,dc=bar")
                .build();

        final LdapConnectionTemplate ldapConnectionTemplate = mock(LdapConnectionTemplate.class);
        final ApacheDsLdapClient ldap = new ApacheDsLdapClient(ldapConfiguration, ldapConnectionTemplate);
        final ArgumentCaptor<SearchRequest> argumentCaptor = ArgumentCaptor.forClass(SearchRequestImpl.class);

        when(ldapConnectionTemplate.search(argumentCaptor.capture(), ArgumentMatchers.<EntryMapper<Entry>>any())).thenReturn(Collections.singletonList(new DefaultEntry()));

        String injectionUserName = "*)(objectclass=*";
        ldap.search("(uid={0})", new String[]{injectionUserName}, 1);

        assertThat(argumentCaptor.getValue().getFilter())
                .isEqualTo(FilterParser.parse("(uid=\\2A\\29\\28objectclass=\\2A)"));
    }