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
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ private module Frameworks {
private import semmle.code.java.frameworks.spring.SpringValidation
private import semmle.code.java.frameworks.spring.SpringWebClient
private import semmle.code.java.frameworks.spring.SpringBeans
private import semmle.code.java.frameworks.spring.SpringWebMethod
private import semmle.code.java.frameworks.spring.SpringWebMultipart
private import semmle.code.java.frameworks.spring.SpringWebUtil
private import semmle.code.java.security.ResponseSplitting
Expand Down
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",
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

sauyon marked this conversation as resolved.
Show resolved Hide resolved
"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 java/ql/test/library-tests/frameworks/spring/web/MethodTest.java
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
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import org.springframework.web.multipart.MultipartResolver;

// Test case generated by GenerateFlowTestCase.ql
public class Test {
public class MultipartTest {

Object getElement(Iterator container) { return container.next(); }
Object getElement(Collection container) { return container.iterator().next(); }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
import org.springframework.web.util.WebUtils;

// Test case generated by GenerateFlowTestCase.ql
public class Test {
public class UtilTest {

class StubUriTemplateVariables extends HashMap<String, Object> implements UriComponents.UriTemplateVariables {
StubUriTemplateVariables(Map m) { super(m); }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class TaintFlowConf extends TaintTracking::Configuration {
override predicate isSink(DataFlow::Node n) {
n.asExpr().(Argument).getCall().getCallee().hasName("sink")
}

override int fieldFlowBranchLimit() { result = 10 }
}

class HasFlowTest extends InlineExpectationsTest {
Expand Down

This file was deleted.

This file was deleted.

Empty file.
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){}
}
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){}
}
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; }
}
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){}
}
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);
}
Loading