[Java] CWE-552: Unsafe url forward #6240

haby0 · 2021-07-08T11:44:08Z

Constructing a server-side redirect path with user input could allow an attacker to download application binaries
(including application classes or jar files) or view arbitrary files within protected directories.

smowton

Only taken a quick look so do please check in more detail, but I think there are already queries for the Spring aspects of this. getRequestDispatcher(...) does appear to be new.

smowton · 2021-07-13T08:26:03Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+}
+
+/** A Unsafe url forward sink from spring controller method. */
+private class SpringUrlForwardSink extends UnsafeUrlForwardSink {


Overlap with CWE-601/SpringUrlRedirect.ql?

No overlap, no url jump will be triggered here, files will be read.

Hello @pwntester, do you have any suggestions for this?

smowton · 2021-07-13T08:27:04Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("setViewName") and


Overlap with CWE-094/SpringViewManipulation.ql?

CWE-094/SpringViewManipulation must use the Thymeleaf template engine.

pwntester · 2021-07-12T16:11:44Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

+    exists(AddExpr ae |
+      ae.getRightOperand() = node.asExpr() and
+      (
+        not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue().matches("/WEB-INF/%")
+        and
+        not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
+      )
+    )


Needs to account for string building patterns. This has been implemented in the past (#4530)

I made changes, please review again, thank you!

pwntester · 2021-09-23T16:14:03Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

+    not exists(MethodAccess ma |
+      ma.getMethod().getName() in ["getRequestURI", "getRequestURL", "getPathInfo"] and
+      ma.getMethod()
+          .getDeclaringType()
+          .getASupertype*()
+          .hasQualifiedName("javax.servlet.http", "HttpServletRequest")
+    )
+  }


You forgot to connect ma and source. Probably you want to add source.asExpr() = ma

atorralba

I added some inline comments.

Also, I agree there's some overlap with SpringUrlRedirect, since that query also looks for forward: URLs, but I guess it makes sense because that would be both an open redirect and a local file include. So I think there's no need to change it in this PR, but it's something to take into account when we eventually promote all of these (altogether with the copy-pasted code from SpringUrlRedirect, SpringViewManipulation and RequestForgery).

atorralba · 2021-09-27T13:24:32Z

java/ql/lib/semmle/code/java/frameworks/Servlets.qll

+    getNumberOfParameters() = 1 and
+    getParameter(0).getType() instanceof TypeString


Are there overloads of this method with different parameters? If not, these two conditions aren't needed.

atorralba · 2021-09-27T13:53:41Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+/**
+ * A concatenate expression using the string `forward:` on the left.
+ *
+ * E.g: `"forward:" + url`


Avoid contractions in QLDoc: https://github.com/github/codeql/blob/main/docs/qldoc-style-guide.md#language-requirements

atorralba · 2021-09-27T13:54:00Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+/**
+ * A call to `StringBuilder.append` or `StringBuffer.append` method, and the parameter value is `"forward:"`.
+ *
+ * E.g: `StringBuilder.append("forward:")`


Avoid contractions in QLDoc: https://github.com/github/codeql/blob/main/docs/qldoc-style-guide.md#language-requirements

atorralba · 2021-09-27T14:36:25Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

+      ma.getMethod()
+          .getDeclaringType()
+          .getASupertype*()
+          .hasQualifiedName("javax.servlet.http", "HttpServletRequest") and


you can use the HttpServletRequest class from Servlets.qll.

atorralba · 2021-09-27T14:37:17Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+    )
+    or
+    exists(ClassInstanceExpr cie |
+      cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and


You can use the ModelAndView class from SpringWeb.qll.

atorralba · 2021-09-27T14:37:25Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+      ma.getMethod().hasName("setViewName") and
+      ma.getMethod()
+          .getDeclaringType()
+          .hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and


You can use the ModelAndView class from SpringWeb.qll.

smowton · 2021-10-06T16:47:19Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

+In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and 
+<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward


Suggested change

In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and

<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward

The <code>bad</code> methods show an HTTP request parameter being used directly in a URL forward

Also consider should the user manual example have so many cases? Are they all illustrating sufficiently different things, or would one or two do, with the rest kept in the tests but not the documentation?

smowton · 2021-10-06T16:51:54Z

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

+private Expr getQualifier(Expr e) { result = e.(MethodAccess).getQualifier() }
+
+/**
+ * An extension of `StringBuilderVar` that also accounts for strings appended in StringBuilder/Buffer's constructor


This being copy-pasted from RequestForgery.qll, let's pull it out into a library rather than duplicate

smowton · 2021-10-19T13:53:18Z

Rewritten to use the shared library; will merge on green

haby0 requested a review from a team as a code owner July 8, 2021 11:44

github-actions bot added documentation Java labels Jul 8, 2021

haby0 changed the title ~~[Java] CWE-348: Unsafe url forward~~ [Java] CWE-552: Unsafe url forward Jul 8, 2021

This was referenced Jul 12, 2021

Java: CWE-552 Query to detect configuration file/source code exposure from unsafe request dispatcher #6251

Closed

[Java] CWE-552: Unsafe url forward github/securitylab#399

Closed

smowton reviewed Jul 13, 2021

View reviewed changes

haby0 dismissed a stale review via df93cd0 September 16, 2021 12:14

pwntester suggested changes Sep 23, 2021

View reviewed changes

smowton requested a review from atorralba September 24, 2021 17:51

atorralba reviewed Sep 27, 2021

View reviewed changes

haby0 requested a review from atorralba September 28, 2021 01:45

smowton reviewed Oct 6, 2021

View reviewed changes

haby0 requested a review from smowton October 8, 2021 04:13

haby0 and others added 7 commits October 19, 2021 12:49

Add CWE-552-UnsafeUrlForward

d0eec1e

Eliminate FP

952b34a

Modify Sanitizer

679652e

Modify the model

283376e

Fix

9d9a7ab

Copyedit

8a4fa0a

Rewrite query to use shared StringPrefixes library

057d0fb

smowton approved these changes Oct 19, 2021

View reviewed changes

smowton merged commit 233a334 into github:main Oct 19, 2021

haby0 deleted the java/UnsafeUrlForward branch October 26, 2021 06:21

jcogs33 mentioned this pull request Mar 11, 2024

Java: Promote Unsafe URL Forward query from experimental #14854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java] CWE-552: Unsafe url forward #6240

[Java] CWE-552: Unsafe url forward #6240

haby0 commented Jul 8, 2021 •

edited

Loading

smowton left a comment

smowton Jul 13, 2021

haby0 Jul 13, 2021

haby0 Jul 13, 2021

smowton Jul 13, 2021

haby0 Jul 13, 2021

pwntester Jul 12, 2021

haby0 Sep 24, 2021

pwntester Sep 23, 2021

atorralba left a comment

atorralba Sep 27, 2021

haby0 Sep 28, 2021

atorralba Sep 27, 2021

haby0 Sep 28, 2021

atorralba Sep 27, 2021

haby0 Sep 28, 2021

atorralba Sep 27, 2021

haby0 Sep 28, 2021

atorralba Sep 27, 2021

haby0 Sep 28, 2021

atorralba Sep 27, 2021

haby0 Sep 28, 2021

smowton Oct 6, 2021

smowton Oct 6, 2021

haby0 Oct 8, 2021

smowton Oct 6, 2021

smowton commented Oct 19, 2021

		getNumberOfParameters() = 1 and
		getParameter(0).getType() instanceof TypeString

		In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and
		<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward

	In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and
	<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward
	The <code>bad</code> methods show an HTTP request parameter being used directly in a URL forward

[Java] CWE-552: Unsafe url forward #6240

[Java] CWE-552: Unsafe url forward #6240

Conversation

haby0 commented Jul 8, 2021 • edited Loading

smowton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atorralba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smowton commented Oct 19, 2021

haby0 commented Jul 8, 2021 •

edited

Loading