[Java] Add QL for detecting Spring View Manipulation Vulnerabilities.

[ghost]

An attacker contolling the name of the Spring View, can exploit Expression Language Injection vulnerabilities as described in this blog.

This PR adds a QL query for the same.

[Marcono1234]

Would it also make sense to cover the implicit view name translation? Though that is not really a dataflow / taint issue so would probably need a separate query.

[ghost]

Yup, I was just adding support for that. I will add the tests later today too.

[ghost]

@smowton I made the changes you requested. I have decided to not include the tests with this one as this is experimental. I hope that's not an issue. I am creating a corresponding GHSL issue for bounty tracking now.

[pwntester]

Please include Portlet and Servlet versions of ModelAndView which contains multiple sinks in its constructor and setViewName.

Also, include the implicit view resolution cases

[ghost]

@pwntester I can't include the implicit view resolution classes here as they don't actually require flow analysis. they would form part of a separate query.

I have included the sinks for ModelAndView though.

[pwntester]

Good point, but I think it makes sense to add that separate query part of this PR though

[pwntester]

We observed many False Positives caused by controller methods performing redirects such as:

 return "redirect:" + request.getHeader("Referer");

or

 return redirectToRenderSession(bar, foo);

These are open for a new separate bounty submission for Open Redirect, but not relevant in this context. Note that redirect: prefix can be added directly in the return statement or in a method such as the second example, so the sanitizer should look for string concatenations/interpolations in any node of the dataflow

[ghost]

okay I will extract this and create a new PR for the Open Redirect issue.

[pwntester]

is this necessary given the StringFlowConfig?

[ghost]

yes, codeql won't recognize the local flow otherwise. I tried testing with the dataflow config alone. QL won't recognise the flow from in the case of statements like

return "redirect:" + request.getHeader("Referer");

[pwntester]

thanks for the info, it make sense since both the source and the sink belong to the same expression. ccing @aschackmull to double check

[aschackmull]

Well, the StringFlowConfig likely won't do much, as it only does pure data flow without any taint steps, so it'll only report flow if the literal string "redirect:" (or "ajaxredirect:") reaches the sink unmodified. Adding a suitable string concatenation step as an additional step would address this.
However, checking sanitization through a separate flow config like this likely isn't what you want, since if a sink is reached both by "redirect:" + something and remotely controlled data, without the former path being a subpath of the latter, then you still have a vulnerability, but this sort of pattern will remove those results leading to false negatives.
Instead there's a much simpler and better solution: Get rid of StringFlowConfig and insert the concatenated-with-"redirect:" check as a sanitizer directly in the SpringViewManipulationConfig, i.e. add something like

override predicate isSanitizer(DataFlow::Node node) {
  exists(AddExpr e |
    node.asExpr() = e and
    e.getLeftOperand().(StringLiteral).getRepresentedString() = ["redirect:", "ajaxredirect:"]
  )
}

This will remove all such AddExpr nodes from the data-flow graph in the calculation of flow in SpringViewManipulationConfig.

[pwntester]

Thanks @aschackmull! @porcupineyhairs do not forget to account for Appendable.append (StringBuffer, StringBuilder, StringWriter, ...) and String.format

[ghost]

@aschackmull I considered adding the isSanitizer check earlier. String concatenation steps such as those suggested by @pwntester would make writing a good sanitizer almost impossible. Hence I chose the flow config. I could change it to a taintflow to fix the issue you pointed earlier. WDYT?

[aschackmull]

I don't think it sounds that difficult to incorporate those additional kinds of string concatenation in a sanitizer. String.format should be very similar to an AddExpr, and for things like StringBuilder you can likely just mark any local flow from a StringBuilder that gets a string like "redirect" as being sanitizers, i.e. all occurrences of x in the following:

x.append("redirect:");
x.append(tainted());
return x.toString();

As you can see, this will block flow from tainted() reaching the return when the final x is marked as a sanitizer.

[ghost]

@aschackmull @pwntester I have added the sanitisers and rebased to the latest main.

[ghost]

@pwntester @aschackmull I have also included a second query now to keep track of the implicit view resolution classes.

[pwntester]

Hi @porcupineyhairs, after reviewing some initial results we still get low SNR. Most FPs are caused for the following reasons (you can use the projects to verify future enhacements):

Projects returning a redirect: prefixed view:

• everwatchsolutions/bullpen: FPs. "redirect:/candidate/" + id
• common-workflow-language/cwlviewer: FPs. return new ModelAndView("redirect:/?url=" + gitDetails.getUrl());
• irods-contrib/metalnx-web: FPs. new ModelAndView(sb.toString());
• JoseZZ/playbook: FPs. return "redirect:".concat(ultimaUrl);

ModelAndView is only a sink for the view part, not for the model one, so addObject or addAllObjects are not sinks:

• Ricardolv/Brewer: FPs. addObject is not a sink. also new ModelAndView("redirect:/sales/" + sale.getCode())
• leo-dias/brewer: addObject is not a sink. Also return new ModelAndView("redirect:/vendas/" + venda.getCodigo());
• stormpath/stormpath-sdk-java: addAllObjects is not a sink

[ghost]

@pwntester

I have made a few changes to my code. it can now avoid some of the FP's you mention above.

project	resolution
stormpath/stormpath-sdk-java	has 4 valid detections now
leo-dias/brewer	removed the FP
Ricardolv/Brewer	removed the FP
common-workflow-language/cwlviewer	removed the FP
everwatchsolutions/bullpen	removed the FP
ZHENFENG13/spring-boot-projects	13 detections now. requires Admin to exploit though
JoseZZ/playbook	Looks like a CodeQL bug (more below)
irods-contrib/metalnx-web	Codeql won't detect Appendable.append (more below)

As for JoseZZ/playbook and irods-contrib/metalnx-web, my query already includes sanitizers for these FP's. However, for some reason, CodeQL-Java in this case fails to recognize methods declared inside builtin Java classes/interfaces such as java.lang.String or java.lang.Appendable. Hence, none of these sanitizers can work.

@aschackmull can you please help me here?

[ghost]

@aschackmull Thanks to @smowton I have fixed the sanitizers now. With this all of the FP's from the path query, the ones which @pwntester lists above would now be fixed.
I have also modified the implicit manipulation query based on the FP I could see in the LGTM run. There should be significantly lower FP's with that query now.

[ghost]

@pwntester @smowton I went through the results of the implicit view query. There was a FP in anavenianil/watererp_latest. I have added a small sanitizer for that now. With the last FP gone, all of the results detected with the implicit view manipulation query are true positive results.

[intrigus-lgtm]

@porcupineyhairs it would be great if you could unresolve this conversation:
https://github.com/github/codeql/pull/4214/files/22fbf90a4e5557b7186fac9159b9c7d331c297be#r567487724

[ghost]

@pwntester @smowton I have made further changes to my PR. All of the FP's across both the PR's are now fixed. PTAL.

[ghost]

@smowton Changes done!

[ghost]

@smowton Changes done

[smowton]

Address comment or expand to note why it is not done yet

[ghost]

removed it now. A taint obtained from that field is unlikely to be actually exploitable.

[aschackmull]

Change does not appear to be pushed?

[aschackmull]

It would be nice to have some .qhelp for these queries.

[aschackmull]

[ERROR] Input file ql/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll is not correctly formatted

[ghost]

@aschackmull @smowton I have added the qhelp now and rebased the code to the current main.

[smowton]

Looks good to me; over to @aschackmull

[ghost]

I have rebased it to the latest main while this is pending merge.

[smowton]

@porcupineyhairs this is a little tricky because of the experimental query + adding the new Portlet source in the same PR. Is the Portlet source (a) important to this query, (b) generally interesting, or (c) both?

If (a), then you should make it a query-specific source and not add it to the general RemoteFlowSource
If (b), then you should drop it from this PR (making this one wholly experimental) and submit a followup PR to add it to RemoteFlowSource
If (c), then you should do as (a) and then make a followup PR that moves the source out of experimental.

[ghost]

@smowton Taking option (c).
Let's merge this first then I will move the source to FlowSources.qll