| Dec | JAN | Feb |
| 05 | ||
| 2025 | 2026 | 2027 |
COLLECTED BY
Collection: Save Page Now
To see all available qualifiers, see our documentation.
Sign in /
zbazztian
commented
Sorry, something went wrong.
zbazztian
requested a review
from a team
as a code owner
github-actions
bot
added
the
Java
label
|
@aschackmull I added you as a contributor to the PoC repository so you can view the results. |
Sorry, something went wrong.
|
Click to show differences in coveragejavaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,469,91,,,,19,14,,29
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,473,91,,,,19,14,,29
- Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jooq``, ``org.mvel2``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",44,269,151,,,,14,18,,
+ Others,"``androidx.slice``, ``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jooq``, ``org.mvel2``, ``org.owasp.webgoat.assignments``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",44,272,151,,,,14,18,,
- Totals,,180,5624,431,13,6,10,107,33,1,66
+ Totals,,180,5631,431,13,6,10,107,33,1,66
+ org.owasp.webgoat.assignments,,,3,,,,,,,,,,,,,,,,,,,,,,3,
+ org.springframework.context.support,,,4,,,,,,,,,,,,,,,,,,,,,,4, |
Sorry, something went wrong.
owen-mc
changed the title
| "org.owasp.webgoat.assignments;AttackResult;false;AttackResult;;;Argument[1];Argument[-1];taint", | ||
| "org.owasp.webgoat.assignments;AttackResult;false;AttackResult;;;Argument[2];Argument[-1];taint", | ||
| "org.owasp.webgoat.assignments;AttackResult;false;AttackResult;;;Argument[3];Argument[-1];taint", |
aschackmull
AttackResult it looks like we just need to recognize StringEscapeUtils.escapeJson from org.apache.commons.lang3 as a taint step. Since this is mostly just escaping quotes " then I'd guess that it's a likely candidate for a general taint step, but I'll defer to @atorralba on that.
Sorry, something went wrong.
zbazztian
StringEscapeUtils.escapeJson() and then AttackResult objects will indeed be tainted (partially). However, they will be tainted with an access path (to the tainted field assigned to in the constructor) and apparently this then does not work in the last step, where that object flows into a return statement. I am happy to learn about the way to make that work though.
Sorry, something went wrong.
aschackmull
AttackResult, so whichever rules leads us to model the sink, ought also to allow us to identify the relevant fields in QL. Once we have that, we can specify that these fields are implicitly read at the sink using Configuration::allowImplicitRead(DataFlow::Node node, DataFlow::Content c) (when overriding this predicate it is generally advisable to call super).
See e.g. the default implementation of allowImplicitReadonTaintTracking::Configuration - this specifies that array and collection contents can be implicitly read at sink nodes.
Sorry, something went wrong.
zbazztian
Sorry, something went wrong.
atorralba
StringEscapeUtils.escapeJson as a general taint step. The implicit read shouldn't be needed because a method returning an AttackResult isn't actually a XSS sink — see my comment below.
Sorry, something went wrong.
| exists(RefType returnType | | ||
| returnType = requestMappingMethod.getReturnType() and | ||
| returnType.hasQualifiedName("org.owasp.webgoat.assignments", "AttackResult") | ||
| ) |
aschackmull
AttackResult class instances in a generic way, so we should too. @atorralba could you advise?
Sorry, something went wrong.
zbazztian
Sorry, something went wrong.
atorralba
AttackResult seems to be the POJO that holds the response data of a call to one of the "attack exercise" APIs. As @zbazztian mentioned, this object is JSONified by Spring before being sent over the network to the client. So, I agree this sink shouldn't be in the standard library since it's too specific to have any use in real-world applications.
Regarding the XSS, this isn't actually a server-side issue but rather a client-side one: the server just exposes an API that returns a JSON object when a request is made -- that isn't XSS and it wouldn't make sense for the server to apply any validation or sanitization to the data, since it isn't being added to an HTML page but rather to a JSON object.
The client, on the other hand, is receiving external data from the server (the response to an API call) and directly adding it to the HTML page without sanitization. So I would say this is something that should be detected by the JavaScript queries, not the Java ones.
The LibrarySink class in the JS libraries seems to account for the relevant sink (jQuery's html method). The source we are interested in isn't included by default: importing AdditionalSources.qll into Customizations.qll should do the trick, since it defines RemoteServerResponse, which models jQuery Ajax call's responses.
Of course, the assumption here is that the attacker is able to manipulate the server's response (e.g. because the response reflects one or more of the request's parameters), but I think it's a good rule of thumb to consider any data coming from external systems as untrusted.
Sorry, something went wrong.
zbazztian
Sorry, something went wrong.
Java: Pass taint through Spring's AbstractMessageSource.getMessage() …
f36ee95
…methods.
Java: Pass taint through Apache's StringEscapeUtils.escapeJson() method.
e2a9ced
zbazztian
force-pushed
the
spring-taint-summaries
branch
from
2a20adetoe2a9ced
Compare
|
@aschackmull I rebased and only added those flow summaries that we discussed. I guess I could add some test cases, but would first have to create some stubs for Spring and Apache Commons Lang. Is there some sort of a stub generator that we use for this or is it all manual? |
Sorry, something went wrong.
zbazztian
changed the title
|
Click to show differences in coveragejavaGenerated file changes for java
- `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,423,,,,,,,,
+ `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,424,,,,,,,,
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,469,91,,,,19,14,,29
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,473,91,,,,19,14,,29
- Totals,,180,5624,431,13,6,10,107,33,1,66
+ Totals,,180,5629,431,13,6,10,107,33,1,66
- org.apache.commons.lang3,,,423,,,,,,,,,,,,,,,,,,,,,,292,131
+ org.apache.commons.lang3,,,424,,,,,,,,,,,,,,,,,,,,,,293,131
+ org.springframework.context.support,,,4,,,,,,,,,,,,,,,,,,,,,,4, |
Sorry, something went wrong.
|
@zbazztian you can use |
Sorry, something went wrong.
|
What a nifty tool. Thanks! |
Sorry, something went wrong.
Java: Add test case for StringEscapeUtils.escapeJson() taint step.
39b6678
Java: Add test cases for AbstractMessageSource.getMessage() methods
69f329f
Java: Use the interface instead of the abstract class
a6e4f29
Contributor
Author
|
@aschackmull Added test cases. |
Sorry, something went wrong.
|
Click to show differences in coveragejavaGenerated file changes for java
- `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,423,,,,,,,,
+ `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,424,,,,,,,,
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,469,91,,,,19,14,,29
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,472,91,,,,19,14,,29
- Totals,,180,5642,1276,13,6,10,107,33,1,66
+ Totals,,180,5646,1276,13,6,10,107,33,1,66
- org.apache.commons.lang3,,,423,,,,,,,,,,,,,,,,,,,,,,,292,131
+ org.apache.commons.lang3,,,424,,,,,,,,,,,,,,,,,,,,,,,293,131
+ org.springframework.context,,,3,,,,,,,,,,,,,,,,,,,,,,,3, |
Sorry, something went wrong.
atorralba
left a comment
Sorry, something went wrong.
aschackmull
merged commit 69973da
into
github:main
zbazztian
mentioned this pull request
Successfully merging this pull request may close these issues.