Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: model Spring web.method.support #6595

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sauyon
Copy link
Contributor

@sauyon sauyon commented Sep 3, 2021

No description provided.

@sauyon sauyon requested a review from a team as a code owner September 3, 2021 04:29
@github-actions github-actions bot added the Java label Sep 3, 2021
@sauyon sauyon force-pushed the java/spring-web-method branch from acfcd5c to 6682d93 Compare September 3, 2021 05:14
row =
[
// for review: arguably this shouldn't be modeled as the implementations of resolveArgument that I've seen are effectively sanitized
"org.springframework.web.method.support;HandlerMethodArgumentResolver;true;resolveArgument;;;Argument[2];ReturnValue;taint",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example what sanitisation you've seen? If the user provides a form value or whatever is going to be associated with the given method parameter, will it be rendered safe for pasting into HTML output? Would it still work as SQL injection for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the two cases I saw in Spring data, the user-input strings were parsed into a Sort object, and page size and page number, neither of which I think can carry meaningful taint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sure, but presumably this would propagate taint when the argument type isn't restrictive like that? If so I'd say we should regard it as a taint propagator in general and sanitize depending on the particular usage (e.g. integers are already blanket sanitized by the XSS query)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my thinking. I'll leave this in then.

"org.springframework.web.method.support;UriComponentsContributor;true;contributeMethodArgument;;;Argument[1];Argument[2];taint",
"org.springframework.web.method.support;UriComponentsContributor;true;contributeMethodArgument;;;Argument[1];Argument[3];taint",
// InvocableHandlerMethod is not modeled as it is difficult to model method-like classes with CSV
// This is a very broad definition of data flow; there is a method `setRedirectModelScenario(boolean)` which is used to determine which of the `Default` and `Redirect` models are returned by `getModel`, and the methods that deal with attributes below are convenience methods for `.getMethod().*`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.getModel().*?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. addAttribute is a shortcut for getModel().addAttribute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants