Acknowledged; will look at this on January 4th or later

Thanks @smowton. This is also my last planned PR before the holidays. Merry Christmas and Happy New Year!

@luchua-bc

Looks good, waiting on security-lab to look at the results now.

Hi @luchua-bc,
It looks like it is still possible to use an encrypted connection by setting Context.SECURITY_PROTOCOL to ssl even if the schema is ldap://. Could you please improve the query to take it into account?

Thanks @JarLob for reviewing this PR. I thought about this one when submitting the first version but finally decided not to include it since it's not that a common configuration. There is no GitHub repository found with this configuration during a quick search when I drafted the query.

However, it's nice to have it so that the query can be more comprehensive and can help to reduce possible false positives. I've made requested changes. Please review.

Cheers,
@luchua-bc

The results are much better now! But it bugs me that the sink is .put(Context.PROVIDER_URL instead of the InitialDirContext constructor call. Could you please remove the side-conditions on the sink and additional taint step regarding SECURITY_PROTOCOL and SECURITY_AUTHENTICATION; instead, use a separate Configuration for each of them, and look for a sink which IS also a sink for SECURITY_AUTHENTICATION = simple AND PROVIDER_URL = ldap:// AND NOT SECURITY_PROTOCOL = ssl. It wouldn't warn in a case like:

Hashtable<String, String> env = new Hashtable<>();
setSSL(env);
env.put(Context.PROVIDER_URL, "ldap://blabla");
setBasicAuth(env);
userContext = new InitialLdapContext(env, null);

Sorry I don't quite get what you mean. Do you want to make the Hashtable env the sink? How can three configurations be made with the same sink? And what will be their source? Please provide more details. Thanks.

The new InitialLdapContext(env, ...) call would be the sink, and env.put calls with an appropriate key would be an additional taint step. Look for "ldap://..." flowing to the sink, AND basic-authentication flowing to the sink, AND NOT SSL flowing to the sink. That means three configurations, differing by the env.put key they're sensitive to.

Thanks @smowton for the detailed explanation.

As the approach is quite different from my original thoughts, it took me some time to revamp the query. Although I'm still not entirely sure whether it's what we want, the new query does render desired results.

Please review. Thanks.

I don't have any further comments; pinged a Java code-owner to take a look.