-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
sauyon
wants to merge
6
commits into
github:main
Choose a base branch
from
sauyon:java/spring-web-method
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8068207
Move spring web tests into one directory
fce8f41
Add stubs for Spring web.method tests
7b2815a
Add models for Spring web.method
6682d93
Add tests for Spring web.method
8cf9e9a
fixup Revert changes to MediaType.java
84f24c8
Fix typo in Spring web.method model
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
30 changes: 30 additions & 0 deletions
30
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebMethod.qll
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/** Provides models of taint flow in `org.springframework.web.method` */ | ||
|
||
import java | ||
private import semmle.code.java.dataflow.ExternalFlow | ||
|
||
// currently only models classes in the `support` subpackage | ||
private class FlowSummaries extends SummaryModelCsv { | ||
override predicate row(string row) { | ||
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", | ||
"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 `.getModel().*`. | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;getModel;;;SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];ReturnValue;value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;getDefaultModel;;;SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];ReturnValue;value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;setRedirectModel;;;Argument[0];SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;addAttribute;;;Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;addAttribute;;;Argument[1];MapValue of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;addAllAttributes;;;MapKey of Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;addAllAttributes;;;MapValue of Argument[0];MapValue of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;mergeAttributes;;;MapKey of Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;mergeAttributes;;;MapValue of Argument[0];MapValue of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;setView;;;Argument[0];SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.View] of Argument[-1];value", | ||
"org.springframework.web.method.support;ModelAndViewContainer;false;getView;;;SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.View] of Argument[-1];ReturnValue;value" | ||
] | ||
} | ||
} |
136 changes: 136 additions & 0 deletions
136
java/ql/test/library-tests/frameworks/spring/web/MethodTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
package generatedtest; | ||
|
||
import java.util.Map; | ||
import org.springframework.ui.ModelMap; | ||
import org.springframework.web.context.request.NativeWebRequest; | ||
import org.springframework.web.method.support.HandlerMethodArgumentResolver; | ||
import org.springframework.web.method.support.ModelAndViewContainer; | ||
import org.springframework.web.method.support.UriComponentsContributor; | ||
import org.springframework.web.util.UriComponentsBuilder; | ||
|
||
// Test case generated by GenerateFlowTestCase.ql | ||
public class MethodTest { | ||
|
||
Object getMapKey(Map container) { return container.keySet().iterator().next(); } | ||
Object getMapValue(Map container) { return container.get(null); } | ||
ModelMap getModelAndViewContainer_Model(ModelAndViewContainer container) { return container.getModel(); } | ||
Object getModelAndViewContainer_View(ModelAndViewContainer container) { return container.getView(); } | ||
ModelAndViewContainer newWithModelAndViewContainer_Model(Object element) { ModelAndViewContainer ret = new ModelAndViewContainer(); ret.setRedirectModel((ModelMap)element); return ret; } | ||
ModelAndViewContainer newWithModelAndViewContainer_View(Object element) { ModelAndViewContainer ret = new ModelAndViewContainer(); ret.setView(element); return ret; } | ||
Object source() { return null; } | ||
void sink(Object o) { } | ||
|
||
public void test() throws Exception { | ||
|
||
{ | ||
// "org.springframework.web.method.support;HandlerMethodArgumentResolver;true;resolveArgument;;;Argument[2];ReturnValue;taint" | ||
Object out = null; | ||
NativeWebRequest in = (NativeWebRequest)source(); | ||
HandlerMethodArgumentResolver instance = null; | ||
out = instance.resolveArgument(null, null, in, null); | ||
sink(out); // $ hasTaintFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;addAllAttributes;;;MapKey of Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Map in = Map.of(source(), null); | ||
out.addAllAttributes(in); | ||
sink(getMapKey(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;addAllAttributes;;;MapValue of Argument[0];MapValue of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Map in = Map.of(null, source()); | ||
out.addAllAttributes(in); | ||
sink(getMapValue(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;addAttribute;;;Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
String in = (String)source(); | ||
out.addAttribute(in, null); | ||
sink(getMapKey(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;addAttribute;;;Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Object in = (Object)source(); | ||
out.addAttribute(in); | ||
sink(getMapKey(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;addAttribute;;;Argument[1];MapValue of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Object in = (Object)source(); | ||
out.addAttribute(null, in); | ||
sink(getMapValue(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;getDefaultModel;;;SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];ReturnValue;value" | ||
ModelMap out = null; | ||
ModelAndViewContainer in = (ModelAndViewContainer)newWithModelAndViewContainer_Model(source()); | ||
out = in.getDefaultModel(); | ||
sink(out); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;getModel;;;SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];ReturnValue;value" | ||
ModelMap out = null; | ||
ModelAndViewContainer in = (ModelAndViewContainer)newWithModelAndViewContainer_Model(source()); | ||
out = in.getModel(); | ||
sink(out); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;getView;;;SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.View] of Argument[-1];ReturnValue;value" | ||
Object out = null; | ||
ModelAndViewContainer in = (ModelAndViewContainer)newWithModelAndViewContainer_View(source()); | ||
out = in.getView(); | ||
sink(out); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;mergeAttributes;;;MapKey of Argument[0];MapKey of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Map in = Map.of(source(), null); | ||
out.mergeAttributes(in); | ||
sink(getMapKey(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;mergeAttributes;;;MapValue of Argument[0];MapValue of SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Map in = Map.of(null, source()); | ||
out.mergeAttributes(in); | ||
sink(getMapValue(getModelAndViewContainer_Model(out))); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;setRedirectModel;;;Argument[0];SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.Model] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
ModelMap in = (ModelMap)source(); | ||
out.setRedirectModel(in); | ||
sink(getModelAndViewContainer_Model(out)); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;ModelAndViewContainer;false;setView;;;Argument[0];SyntheticField[org.springframework.web.method.support.ModelAndViewContainer.View] of Argument[-1];value" | ||
ModelAndViewContainer out = null; | ||
Object in = (Object)source(); | ||
out.setView(in); | ||
sink(getModelAndViewContainer_View(out)); // $ hasValueFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;UriComponentsContributor;true;contributeMethodArgument;;;Argument[1];Argument[2];taint" | ||
UriComponentsBuilder out = null; | ||
Object in = (Object)source(); | ||
UriComponentsContributor instance = null; | ||
instance.contributeMethodArgument(null, in, out, null, null); | ||
sink(out); // $ hasTaintFlow | ||
} | ||
{ | ||
// "org.springframework.web.method.support;UriComponentsContributor;true;contributeMethodArgument;;;Argument[1];Argument[3];taint" | ||
Map out = null; | ||
Object in = (Object)source(); | ||
UriComponentsContributor instance = null; | ||
instance.contributeMethodArgument(null, in, null, out, null); | ||
sink(out); // $ hasTaintFlow | ||
} | ||
|
||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 0 additions & 1 deletion
1
java/ql/test/library-tests/frameworks/spring/webmultipart/options
This file was deleted.
Oops, something went wrong.
52 changes: 0 additions & 52 deletions
52
java/ql/test/library-tests/frameworks/spring/webmultipart/test.ql
This file was deleted.
Oops, something went wrong.
Empty file.
18 changes: 18 additions & 0 deletions
18
.../ql/test/stubs/springframework-5.3.8/org/springframework/beans/BeanMetadataAttribute.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Generated automatically from org.springframework.beans.BeanMetadataAttribute for testing purposes | ||
|
||
package org.springframework.beans; | ||
|
||
import org.springframework.beans.BeanMetadataElement; | ||
|
||
public class BeanMetadataAttribute implements BeanMetadataElement | ||
{ | ||
protected BeanMetadataAttribute() {} | ||
public BeanMetadataAttribute(String p0, Object p1){} | ||
public Object getSource(){ return null; } | ||
public Object getValue(){ return null; } | ||
public String getName(){ return null; } | ||
public String toString(){ return null; } | ||
public boolean equals(Object p0){ return false; } | ||
public int hashCode(){ return 0; } | ||
public void setSource(Object p0){} | ||
} |
19 changes: 19 additions & 0 deletions
19
.../stubs/springframework-5.3.8/org/springframework/beans/BeanMetadataAttributeAccessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Generated automatically from org.springframework.beans.BeanMetadataAttributeAccessor for testing purposes | ||
|
||
package org.springframework.beans; | ||
|
||
import org.springframework.beans.BeanMetadataAttribute; | ||
import org.springframework.beans.BeanMetadataElement; | ||
import org.springframework.core.AttributeAccessorSupport; | ||
|
||
public class BeanMetadataAttributeAccessor extends AttributeAccessorSupport implements BeanMetadataElement | ||
{ | ||
public BeanMetadataAttribute getMetadataAttribute(String p0){ return null; } | ||
public BeanMetadataAttributeAccessor(){} | ||
public Object getAttribute(String p0){ return null; } | ||
public Object getSource(){ return null; } | ||
public Object removeAttribute(String p0){ return null; } | ||
public void addMetadataAttribute(BeanMetadataAttribute p0){} | ||
public void setAttribute(String p0, Object p1){} | ||
public void setSource(Object p0){} | ||
} |
9 changes: 9 additions & 0 deletions
9
java/ql/test/stubs/springframework-5.3.8/org/springframework/beans/BeanMetadataElement.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// Generated automatically from org.springframework.beans.BeanMetadataElement for testing purposes | ||
|
||
package org.springframework.beans; | ||
|
||
|
||
public interface BeanMetadataElement | ||
{ | ||
default Object getSource(){ return null; } | ||
} |
12 changes: 12 additions & 0 deletions
12
java/ql/test/stubs/springframework-5.3.8/org/springframework/beans/BeansException.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Generated automatically from org.springframework.beans.BeansException for testing purposes | ||
|
||
package org.springframework.beans; | ||
|
||
import org.springframework.core.NestedRuntimeException; | ||
|
||
abstract public class BeansException extends NestedRuntimeException | ||
{ | ||
protected BeansException() {} | ||
public BeansException(String p0){} | ||
public BeansException(String p0, Throwable p1){} | ||
} |
18 changes: 18 additions & 0 deletions
18
...t/stubs/springframework-5.3.8/org/springframework/beans/ConfigurablePropertyAccessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Generated automatically from org.springframework.beans.ConfigurablePropertyAccessor for testing purposes | ||
|
||
package org.springframework.beans; | ||
|
||
import org.springframework.beans.PropertyAccessor; | ||
import org.springframework.beans.PropertyEditorRegistry; | ||
import org.springframework.beans.TypeConverter; | ||
import org.springframework.core.convert.ConversionService; | ||
|
||
public interface ConfigurablePropertyAccessor extends PropertyAccessor, PropertyEditorRegistry, TypeConverter | ||
{ | ||
ConversionService getConversionService(); | ||
boolean isAutoGrowNestedPaths(); | ||
boolean isExtractOldValueForEditor(); | ||
void setAutoGrowNestedPaths(boolean p0); | ||
void setConversionService(ConversionService p0); | ||
void setExtractOldValueForEditor(boolean p0); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.