Java: add CSRF query

[jcogs33]

Description

Adds a query for CSRF vulnerabilities caused by using HTTP request types that are not default-protected by a given framework for apparent state-changing actions. Supports Spring and Stapler frameworks for now. State-changing actions are determined heuristically based on whether the request handling method updates a database or whether its name suggests that it might change application state. Due to the heuristic nature of determining state changes, this is a low precision query.

Consideration

• Let me know if you have any suggestions for improving the heuristics used for identifying state changes.
• See inline comments/questions regarding some code structure that should maybe be improved.
• Let me know if you have any suggestions for better ways to word the query name/id/description/alert message/qhelp. I went through a few iterations of different wording for these, and I'm not sure if what I ended up with is clear enough.

Pull Request checklist

All query authors

• A change note is added if necessary. See the documentation in this repository.
• All new queries have appropriate .qhelp. See the documentation in this repository.
• QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
• New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

• Changes are validated at scale (internal access required).
• Adding a new query? Consider also adding the query to autofix.
• Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).

[github-actions]

QHelp previews:

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.qhelp

HTTP request type unprotected from CSRF

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically, this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

Recommendation

Make sure any requests that change application state are protected from CSRF. Some application frameworks provide default CSRF protection for unsafe HTTP request methods (such as POST) which may change the state of the application. Safe HTTP request methods (such as GET) should only perform read-only operations and should not be used for actions that change application state.

This query currently supports the Spring and Stapler web frameworks. Spring provides default CSRF protection for all unsafe HTTP methods whereas Stapler provides default CSRF protection for the POST method.

Example

The following examples show Spring request handlers allowing safe HTTP request methods for state-changing actions. Since safe HTTP request methods do not have default CSRF protection in Spring, they should not be used when modifying application state. Instead, use one of the unsafe HTTP methods which Spring default-protects from CSRF.

import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;

// BAD - a safe HTTP request like GET should not be used for a state-changing action
@RequestMapping(value="/transfer", method=RequestMethod.GET)
public boolean doTransfer(HttpServletRequest request, HttpServletResponse response){
  return transfer(request, response);
}

// BAD - no HTTP request type is specified, so safe HTTP requests are allowed
@RequestMapping(value="/delete")
public boolean doDelete(HttpServletRequest request, HttpServletResponse response){
  return delete(request, response);
}

import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.DeleteMapping;

// GOOD - use an unsafe HTTP request like POST
@RequestMapping(value="/transfer", method=RequestMethod.POST)
public boolean doTransfer(HttpServletRequest request, HttpServletResponse response){
  return transfer(request, response);
}

// GOOD - use an unsafe HTTP request like DELETE
@DeleteMapping(value="/delete")
public boolean doDelete(HttpServletRequest request, HttpServletResponse response){
  return delete(request, response);
}

The following examples show Stapler web methods allowing safe HTTP request methods for state-changing actions. Since safe HTTP request methods do not have default CSRF protection in Stapler, they should not be used when modifying application state. Instead, use the POST method which Stapler default-protects from CSRF.

import org.kohsuke.stapler.verb.GET;

// BAD - a safe HTTP request like GET should not be used for a state-changing action
@GET
public HttpRedirect doTransfer() {
  return transfer();
}

// BAD - no HTTP request type is specified, so safe HTTP requests are allowed
public HttpRedirect doPost() {
  return post();
}

import org.kohsuke.stapler.verb.POST;

// GOOD - use POST
@POST
public HttpRedirect doTransfer() {
  return transfer();
}

// GOOD - use POST
@POST
public HttpRedirect doPost() {
  return post();
}

References

• OWASP: Cross Site Request Forgery (CSRF).
• Spring Security Reference: Cross Site Request Forgery (CSRF).
• Jenkins Developer Documentation: Protecting from CSRF.
• MDN web docs: HTTP request methods.
• Common Weakness Enumeration: CWE-352.

[aschackmull]

QL part LGTM now, but be sure to verify performance with an additional dca run.

[jcogs33]

Performance looks fine in the new DCA run.

[mchammer01]

I'll review this on behalf of Docs.

[mchammer01]

@jcogs33 👋🏻 - Approving this on behalf of Docs in order not to block you ⚡
I have left a few minor comments and suggestions. Hope these help!

[jcogs33]

@mchammer01, Thanks for the review! Regarding the wording in the overview section, I've changed that section completely so that it aligns more closely with the QHelp overviews in some of our other CSRF queries (c.f. Python, Ruby). Would you mind re-reviewing the overview section and confirming that this re-write looks okay? 🙏

For context: I copied the original overview section from the Java SpringCSRFProtection.qhelp since I wanted to align with our other Java CSRF query, but I agree that the wording could be improved. If you think the re-write is okay, then I'll open a follow-up PR to update SpringCSRFProtection.qhelp as well.

[mchammer01]

@jcogs33 - reviewed as requested. Left a couple of minor suggestions and asked a question.
As usual, feel free to ignore anything you don't agree with 😄

[jcogs33]

Thanks for the review @mchammer01! I've addressed your comments.

[egregius313]

LGTM