hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-25 08:45:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Java: initial update of barrier and test cases to remove FN

Browse Source
This commit is contained in:
Jami Cogswell
2024-03-05 11:08:24 -05:00
parent c8ec301793
commit 911a61df22
4 changed files with 240 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
java/ql/lib/semmle/code/java/security/PathSanitizer.qll
View File

@@ -64,7 +64,8 @@ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
)
}
private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
// TODO: switch back to private if possible
class ExactPathMatchSanitizer extends PathInjectionSanitizer {
ExactPathMatchSanitizer() {
this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()
or
@@ -151,7 +152,8 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer {
}
}
private class BlockListGuard extends PathGuard instanceof MethodCall {
// TODO: switch back to private if possible
class BlockListGuard extends PathGuard instanceof MethodCall {
BlockListGuard() {
(isStringPartialMatch(this) or isPathPrefixMatch(this)) and
isDisallowedWord(super.getAnArgument())
@@ -228,6 +230,7 @@ private predicate isStringPartialMatch(MethodCall ma) {
exists(RefType t | t = ma.getMethod().getDeclaringType() |
t instanceof TypeString or t instanceof StringsKt
) and
// TODO ! Why not use `StringPartialMatchMethod` for the below?
getSourceMethod(ma.getMethod())
.hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])
}

110
java/ql/lib/semmle/code/java/security/UrlForward.qll
View File

@@ -4,6 +4,9 @@ import java
private import semmle.code.java.dataflow.ExternalFlow
private import semmle.code.java.dataflow.FlowSources
private import semmle.code.java.dataflow.StringPrefixes
private import semmle.code.java.security.PathSanitizer
private import semmle.code.java.dataflow.DataFlow
private import semmle.code.java.controlflow.Guards
/** A URL forward sink. */
abstract class UrlForwardSink extends DataFlow::Node { }
@@ -44,16 +47,121 @@ private class PrimitiveBarrier extends UrlForwardBarrier {
}
}
// TODO: should this also take URL encoding/decoding into account?
// TODO: and PathSanitization in general?
private class FollowsBarrierPrefix extends UrlForwardBarrier {
FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() }
}
private class BarrierPrefix extends InterestingPrefix {
BarrierPrefix() {
// TODO: why not META-INF here as well? (and are `/` correct?)
not this.getStringValue().matches("/WEB-INF/%") and
not this.getStringValue() = "forward:"
}
override int getOffset() { result = 0 }
}
private class UrlPathBarrier extends UrlForwardBarrier {
UrlPathBarrier() {
this instanceof PathInjectionSanitizer and
(
this instanceof ExactPathMatchSanitizer //TODO: still need a better solution for this edge case...
or
// TODO: these don't enforce order of checks and PathSanitization... make bypass test cases.
this instanceof NoEncodingBarrier
or
this instanceof FullyDecodesBarrier
)
}
}
abstract class UrlDecodeCall extends MethodCall { }
private class DefaultUrlDecodeCall extends UrlDecodeCall {
DefaultUrlDecodeCall() {
this.getMethod().hasQualifiedName("java.net", "URLDecoder", "decode") or // TODO: reuse existing class? Or make this a class?
this.getMethod().hasQualifiedName("org.eclipse.jetty.util.URIUtil", "URIUtil", "decodePath")
}
}
// TODO: this can probably be named/designed better...
abstract class RepeatedStmt extends Stmt { }
private class DefaultRepeatedStmt extends RepeatedStmt {
DefaultRepeatedStmt() { this instanceof LoopStmt }
}
abstract class CheckEncodingCall extends MethodCall { }
private class DefaultCheckEncodingCall extends CheckEncodingCall {
DefaultCheckEncodingCall() {
// TODO: indexOf?, etc.
this.getMethod().hasQualifiedName("java.lang", "String", "contains") and // TODO: reuse existing class? Or make this a class?
this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "%"
}
}
// TODO: better naming?
// TODO: check if any URL decoding implementations _fully_ decode... or if all need to be called in a loop?
// TODO: make this extendable instead of `RepeatedStmt`?
private class RepeatedUrlDecodeCall extends MethodCall {
RepeatedUrlDecodeCall() {
this instanceof UrlDecodeCall and
this.getAnEnclosingStmt() instanceof RepeatedStmt
}
}
private class CheckEncodingGuard extends Guard instanceof MethodCall {
CheckEncodingGuard() { this instanceof CheckEncodingCall }
Expr getCheckedExpr() { result = this.(MethodCall).getQualifier() }
}
private predicate noEncodingGuard(Guard g, Expr e, boolean branch) {
g instanceof CheckEncodingGuard and
e = g.(CheckEncodingGuard).getCheckedExpr() and
branch = false
or
// branch = false and
// g instanceof AssignExpr and // AssignExpr
// exists(CheckEncodingCall call | g.(AssignExpr).getSource() = call | e = call.getQualifier())
branch = false and
g.(Expr).getType() instanceof BooleanType and // AssignExpr
(
exists(CheckEncodingCall call, AssignExpr ae |
ae.getSource() = call and
e = call.getQualifier() and
g = ae.getDest()
)
or
exists(CheckEncodingCall call, LocalVariableDeclExpr vde |
vde.getInitOrPatternSource() = call and
e = call.getQualifier() and
g = vde.getAnAccess() //and
//vde.getVariable() = g
)
)
}
// TODO: check edge case of !contains(%), make sure that behaves as expected at least.
private class NoEncodingBarrier extends DataFlow::Node {
NoEncodingBarrier() { this = DataFlow::BarrierGuard<noEncodingGuard/3>::getABarrierNode() }
}
private predicate fullyDecodesGuard(Expr e) {
exists(CheckEncodingGuard g, RepeatedUrlDecodeCall decodeCall |
e = g.getCheckedExpr() and
g.controls(decodeCall.getBasicBlock(), true)
)
}
private class FullyDecodesBarrier extends DataFlow::Node {
FullyDecodesBarrier() {
exists(Variable v, Expr e | this.asExpr() = v.getAnAccess() |
fullyDecodesGuard(e) and
e = v.getAnAccess() and
e.getBasicBlock().bbDominates(this.asExpr().getBasicBlock())
)
}
}

5
java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll
View File

@@ -25,10 +25,7 @@ module UrlForwardFlowConfig implements DataFlow::ConfigSig {
predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink }
predicate isBarrier(DataFlow::Node node) {
node instanceof UrlForwardBarrier or
node instanceof PathInjectionSanitizer
}
predicate isBarrier(DataFlow::Node node) { node instanceof UrlForwardBarrier }
DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
}

141
java/ql/test/query-tests/security/CWE-552/UrlForwardTest.java
View File

@@ -112,8 +112,9 @@ public class UrlForwardTest extends HttpServlet implements Filter {
throws IOException, ServletException {
String path = ((HttpServletRequest) request).getServletPath();
// actually BAD since could potentially bypass with ".." encoded as "%2e%2e"?
if (path.startsWith(BASE_PATH) && !path.contains("..")) {
request.getRequestDispatcher(path).forward(request, response);
request.getRequestDispatcher(path).forward(request, response); // $ hasUrlForward
} else {
chain.doFilter(request, response);
}
@@ -124,6 +125,7 @@ public class UrlForwardTest extends HttpServlet implements Filter {
throws IOException, ServletException {
String path = ((HttpServletRequest) request).getServletPath();
// this is still good, should not flag here..., url-decoding first doesn't matter if looking for exact match... :(
if (path.equals("/comaction")) {
request.getRequestDispatcher(path).forward(request, response);
} else {
@@ -199,8 +201,9 @@ public class UrlForwardTest extends HttpServlet implements Filter {
throws ServletException, IOException {
String path = request.getParameter("path");
// actually BAD since could potentially bypass with ".." encoded as "%2e%2e"?
if (path.startsWith(BASE_PATH) && !path.contains("..")) {
request.getServletContext().getRequestDispatcher(path).include(request, response);
request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
}
}
@@ -212,34 +215,43 @@ public class UrlForwardTest extends HttpServlet implements Filter {
// /pages/welcome.jsp/../../WEB-INF/web.xml becomes /WEB-INF/web.xml
// /pages/welcome.jsp/../../%57EB-INF/web.xml becomes /%57EB-INF/web.xml
// actually BAD since could potentially bypass with ".." encoded as "%2e%2e": "/pages/welcome.jsp/%2e%2e/%2e%2e/WEB-INF/web.xml" becomes /pages/welcome.jsp/%2e%2e/%2e%2e/WEB-INF/web.xml, which will pass this check and potentially be problematic if decoded later?
if (requestedPath.startsWith(BASE_PATH)) {
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ hasUrlForward
}
}
// FN: Request dispatcher with negation check and path normalization, but without URL decoding
// When promoting this query, consider using FlowStates to make `getRequestDispatcher` a sink
// only if a URL-decoding step has NOT been crossed (i.e. make URLDecoder.decode change the
// state to a different value than the one required at the sink).
// TODO: but does this need to take into account URLDecoder.decode in a loop...?
// BAD (original FN): Request dispatcher with negation check and path normalization, but without URL decoding
protected void doHead5(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path");
Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ MISSING: hasUrlForward
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); // $ hasUrlForward
}
}
// GOOD: Request dispatcher with path traversal check and URL decoding
protected void doHead6(HttpServletRequest request, HttpServletResponse response)
// BAD (I added to test decode with no loop): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
protected void doHead7(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path");
boolean hasEncoding = path.contains("%");
while (hasEncoding) {
path = URLDecoder.decode(path, "UTF-8");
hasEncoding = path.contains("%");
path = URLDecoder.decode(path, "UTF-8");
if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
}
}
// GOOD: Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
protected void doHead6(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path"); // v
if (path.contains("%")){ // v.getAnAccess()
while (path.contains("%")) {
path = URLDecoder.decode(path, "UTF-8");
}
}
if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
@@ -247,10 +259,107 @@ public class UrlForwardTest extends HttpServlet implements Filter {
}
}
// New Tests (i.e. Added by me)
// GOOD: Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
protected void doHead8(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path"); // v
while (path.contains("%")) {
path = URLDecoder.decode(path, "UTF-8");
}
if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
request.getServletContext().getRequestDispatcher(path).include(request, response);
}
}
// FP now....
// GOOD: Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
protected void doHead9(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path"); // v
boolean hasEncoding = path.contains("%");
while (hasEncoding) {
path = URLDecoder.decode(path, "UTF-8");
hasEncoding = path.contains("%");
}
if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
request.getServletContext().getRequestDispatcher(path).include(request, response); // $ SPURIOUS: hasUrlForward
}
}
// New Tests
public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object obj) throws IOException, ServletException {
String url = req.getParameter("target");
rsp.forward(obj, url, req); // $ hasUrlForward
}
// Other Tests for edge cases:
// // GOOD (I added): Request dispatcher with path traversal check and URL decoding in a loop to avoid double-encoding bypass
// // testing `if` stmt requirement for BB controlling
// protected void doHead12(HttpServletRequest request, HttpServletResponse response)
// throws ServletException, IOException {
// String path = request.getParameter("path");
// if (path.contains("%")) {
// while (path.contains("%")) {
// path = URLDecoder.decode(path, "UTF-8");
// }
// }
// if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
// request.getServletContext().getRequestDispatcher(path).include(request, response);
// }
// }
// // BAD (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
// // having previously been checked against a block-list of forbidden values."
// protected void doHead8(HttpServletRequest request, HttpServletResponse response)
// throws ServletException, IOException {
// String path = request.getParameter("path");
// if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
// boolean hasEncoding = path.contains("%"); // BAD: doesn't do anything with the check...
// request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
// }
// }
// // BAD (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
// // having previously been checked against a block-list of forbidden values."
// protected void doHead9(HttpServletRequest request, HttpServletResponse response)
// throws ServletException, IOException {
// String path = request.getParameter("path");
// boolean hasEncoding = path.contains("%"); // BAD: doesn't do anything with the check... and check comes BEFORE blocklist so guard should not trigger
// if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
// request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
// }
// }
// // BAD (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
// // having previously been checked against a block-list of forbidden values."
// protected void doHead10(HttpServletRequest request, HttpServletResponse response)
// throws ServletException, IOException {
// String path = request.getParameter("path");
// if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
// if (path.contains("%")){ // BAD: wrong check
// request.getServletContext().getRequestDispatcher(path).include(request, response); // $ hasUrlForward
// }
// }
// }
// // "GOOD" (I added): Request dispatcher with path traversal check and single URL decoding; may be vulnerable to double-encoding
// // Tests urlEncoding BarrierGuard "a guard that considers a string safe because it is checked for URL encoding sequences,
// // having previously been checked against a block-list of forbidden values."
// protected void doHead11(HttpServletRequest request, HttpServletResponse response)
// throws ServletException, IOException {
// String path = request.getParameter("path");
// if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
// if (!path.contains("%")){ // GOOD: right check
// request.getServletContext().getRequestDispatcher(path).include(request, response);
// }
// }
// }
}