-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Java: model LDAP bind-DN sinks for java/ldap-injection #22002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Added LDAP bind-DN sinks to the `java/ldap-injection` query: the `String name` argument of `javax.naming.Context` / `javax.naming.directory.DirContext` `bind`, `rebind`, `lookup`, `lookupLink`, and `createSubcontext`; the `java.naming.security.principal` JNDI environment value; and the `principal` argument of Apache Shiro `LdapContextFactory.getLdapContext`. The query now detects LDAP distinguished-name injection (CWE-90) into a bind DN, not just into a search filter or search base. `new javax.naming.ldap.LdapName(String)` is deliberately not modelled as a sink, as it commonly parses an existing certificate or principal DN rather than constructing one for a bind. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,17 @@ extensions: | |
| - ["javax.naming", "Context", True, "lookupLink", "", "", "Argument[0]", "jndi-injection", "manual"] | ||
| - ["javax.naming", "Context", True, "rename", "", "", "Argument[0]", "jndi-injection", "manual"] | ||
| - ["javax.naming", "InitialContext", True, "doLookup", "", "", "Argument[0]", "jndi-injection", "manual"] | ||
| # The `String name` argument of these methods is interpreted as a (distinguished) | ||
| # name; an unescaped, attacker-controlled value lets the caller manipulate the | ||
| # bind DN (LDAP DN injection, CWE-90). `bind`/`rebind`/`createSubcontext` create | ||
| # an entry at the given DN; `lookup`/`lookupLink` resolve it (e.g. to authenticate | ||
| # a bind DN). Only the `(String,...)` overloads matter -- the `(Name,...)` | ||
| # overloads take a structured, already-parsed name. | ||
| - ["javax.naming", "Context", True, "bind", "(String,Object)", "", "Argument[0]", "ldap-injection", "manual"] | ||
| - ["javax.naming", "Context", True, "rebind", "(String,Object)", "", "Argument[0]", "ldap-injection", "manual"] | ||
| - ["javax.naming", "Context", True, "createSubcontext", "(String)", "", "Argument[0]", "ldap-injection", "manual"] | ||
| - ["javax.naming", "Context", True, "lookup", "(String)", "", "Argument[0]", "ldap-injection", "manual"] | ||
| - ["javax.naming", "Context", True, "lookupLink", "(String)", "", "Argument[0]", "ldap-injection", "manual"] | ||
|
Comment on lines
+18
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These don't seem LDAP-specific enough. An option which will work in some cases is to make them models on |
||
|
|
||
| - addsTo: | ||
| pack: codeql/java-all | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| extensions: | ||
| - addsTo: | ||
| pack: codeql/java-all | ||
| extensible: sinkModel | ||
| data: | ||
| # `LdapContextFactory.getLdapContext(principal, credentials)` binds to the | ||
| # directory using `principal` as the bind DN. An unescaped, attacker-controlled | ||
| # principal lets the caller manipulate the DN structure (LDAP DN injection, | ||
| # CWE-90). This is the sink in Apache Shiro CVE-2026-49268, where | ||
| # `DefaultLdapRealm.getUserDn` / `ActiveDirectoryRealm.getUsernameWithSuffix` | ||
| # concatenated the login username into the bind DN with no `Rdn.escapeValue`. | ||
| - ["org.apache.shiro.realm.ldap", "LdapContextFactory", True, "getLdapContext", "(Object,Object)", "", "Argument[0]", "ldap-injection", "manual"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already propagate taint from the argument of a constructor of
javax.naming.ldap.LdapNameto the constructed object. (This is done in QL, rather than yml.) You can see this in the testjava/ql/test/query-tests/security/CWE-090/LdapInjection.java:58(if you look atLdapInjection.expectedyou'll see that line has two alerts, one for theLdapName).Would it make sense to include the
(Name, ...)overloads, since they will only produce an alert for a Name constructed with a user-controlled argument?