[Java] CWE-552: Unsafe url forward by haby0 · Pull Request #6240 · github/codeql · GitHub

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.

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.

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?

Overlap with CWE-094/SpringViewManipulation.ql?

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

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

I made changes, please review again, thank you!

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

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).

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

Updated.

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

Updated.

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

Updated.

you can use the HttpServletRequest class from Servlets.qll.

Updated.

You can use the ModelAndView class from SpringWeb.qll.

Updated.

You can use the ModelAndView class from SpringWeb.qll.

Updated.

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?

Updated.

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

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

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

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

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

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

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

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

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

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

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

		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

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

[Java] CWE-552: Unsafe url forward by haby0 · Pull Request #6240 · github/codeql · GitHub

Uh oh!

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

[Java] CWE-552: Unsafe url forward#6240smowton merged 7 commits intogithub:maingithub/codeql:mainfrom haby0:java/UnsafeUrlForwardCopy head branch name to clipboard

Conversation

haby0 commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Java] CWE-552: Unsafe url forward#6240
smowton merged 7 commits into
github:mainfrom
haby0:java/UnsafeUrlForward

haby0 commented Jul 8, 2021 •
edited
Loading