Conversation
| "org.springframework.util;LinkedMultiValueMap;false;deepCopy;;;MapKey of Argument[-1];MapValue of ReturnValue;value", | ||
| "org.springframework.util;LinkedMultiValueMap;false;deepCopy;;;MapValue of Argument[-1];MapValue of ReturnValue;value", | ||
| "org.springframework.util;MultiValueMap;true;add;;;Argument[0];MapKey of Argument[-1];value", | ||
| "org.springframework.util;MultiValueMap;true;add;;;Argument[1];MapValue of Argument[-1];value", |
There was a problem hiding this comment.
Would it make sense to use Element of MapValue throughout? Since MultiValueMap<K,V> extends Map<K,List<V>>, it could be used as one.
There was a problem hiding this comment.
That's a good observation. Indeed that means that we should use Element of MapValue as you suggest.
sauyon
force-pushed
the
sauyon/java/spring-util
branch
from
c7d849a to
affe83b
Compare
| "org.springframework.util;AutoPopulatingList;false;AutoPopulatingList;(java.util.List<E>, org.springframework.util.AutoPopulatingList$ElementFactory<E>);;Element of Argument[0];Element of ReturnValue;value", | ||
| "org.springframework.util;AutoPopulatingList;false;AutoPopulatingList;(java.util.List<E>, Class<? extends E>);;Element of Argument[0];Element of ReturnValue;value", |
There was a problem hiding this comment.
Signatures won't match. Spaces aren't allowed, and you should use the erased types.
| "org.springframework.util;Base64Utils;false;encodeToString;;;Argument[0];ReturnValue;taint", | ||
| "org.springframework.util;Base64Utils;false;encodeToUrlSafeString;;;Argument[0];ReturnValue;taint", | ||
| "org.springframework.util;Base64Utils;false;encodeUrlSafe;;;Argument[0];ReturnValue;taint", | ||
| "org.springframework.util;CollectionUtils;false;arrayToList;;;Element of Argument[0];Element of ReturnValue;value", |
There was a problem hiding this comment.
Looks wrong: ArrayElement of ?
| "org.springframework.util;CollectionUtils;false;findValueOfType;;;Element of Argument[0];Element of ReturnValue;value", | ||
| "org.springframework.util;CollectionUtils;false;firstElement;;;Element of Argument[0];Element of ReturnValue;value", | ||
| "org.springframework.util;CollectionUtils;false;lastElement;;;Element of Argument[0];Element of ReturnValue;value", | ||
| "org.springframework.util;CollectionUtils;false;mergeArrayIntoCollection;;;Element of Argument[0];Element of Argument[1];value", |
| "org.springframework.util;CollectionUtils;false;toArray;;;Element of Argument[0];Element of ReturnValue;value", | ||
| "org.springframework.util;CollectionUtils;false;toArray;;;Element of Argument[0];Element of Argument[1];value", |
| "org.springframework.util;LinkedMultiValueMap;false;LinkedMultiValueMap;(Map<K,List<V>>);;MapKey of Argument[0];MapKey of ReturnValue;value", | ||
| "org.springframework.util;LinkedMultiValueMap;false;LinkedMultiValueMap;(Map<K,List<V>>);;Element of MapValue of Argument[0];Element of MapValue of ReturnValue;value", |
There was a problem hiding this comment.
Signatures won't match. Use erased types.
| "org.springframework.util;MultiValueMap;true;addAll;(K, java.util.List<? extends V>);;Element of Argument[1];Element of MapValue of Argument[-1];value", | ||
| "org.springframework.util;MultiValueMap;true;addAll;(MultiValueMap<K,V>);;MapValue of Argument[0];Element of MapValue of Argument[-1];value", |
sauyon
force-pushed
the
sauyon/java/spring-util
branch
2 times, most recently
from
346d092 to
a365abb
Compare
|
I believe I've addressed all the review comments, as well as fixing a few errors discovered while generating tests. I've also modeled The tests for constructors don't appear to be working and I'm currently not sure why. |
|
Stubs are currently in the test directory because technically they're stubs of Spring 5.3.8 because one of the classes modeled ( |
|
Constructor tests: I believe it's because we're using |
|
Stubs: suggest just renaming the existing stub dir to 5.3.8 and allowing people to incrementally bring the stubs forwards as and when particular use cases dictate |
sauyon
force-pushed
the
sauyon/java/spring-util
branch
from
a365abb to
222a6a6
Compare
|
Ok, this should hopefully be ready to go now. |
sauyon
force-pushed
the
sauyon/java/spring-util
branch
from
222a6a6 to
b061932
Compare
| import semmle.code.java.dataflow.ExternalFlow | ||
| import semmle.code.java.dataflow.TaintTracking | ||
| import TestUtilities.InlineExpectationsTest | ||
| import semmle.code.java.frameworks.spring.SpringUtil |
There was a problem hiding this comment.
Remove. ExternalFlow needs bidirectional import, so SpringUtil should be imported there instead.
There was a problem hiding this comment.
At the moment it seems like the Spring models are not imported by default, which is why I didn't add the import anywhere---I guess the taint flow steps should always be included?
There was a problem hiding this comment.
Yes. Otherwise they don't actually accomplish anything - as you'll see in this test if you remove this line.
There was a problem hiding this comment.
Right, but that's true of all the Spring models, as far as I can tell? I have included this in Spring.qll.
| "generatedtest;Test;false;newWithArrayElement;;;Argument[0];ArrayElement of ReturnValue;value", | ||
| "generatedtest;Test;false;getArrayElement;;;ArrayElement of Argument[0];ReturnValue;value", | ||
| "generatedtest;Test;false;getElement;;;Element of Argument[0];ReturnValue;value", | ||
| "generatedtest;Test;false;newWithElement;;;Argument[0];Element of ReturnValue;value", | ||
| "generatedtest;Test;false;getMapKey;;;MapKey of Argument[0];ReturnValue;value", | ||
| "generatedtest;Test;false;newWithMapKey;;;Argument[0];MapKey of ReturnValue;value", | ||
| "generatedtest;Test;false;newWithMapValue;;;Argument[0];MapValue of ReturnValue;value", | ||
| "generatedtest;Test;false;getMapValue;;;MapValue of Argument[0];ReturnValue;value" |
There was a problem hiding this comment.
Remove. Replace with proper implementations. See #6130
| @@ -0,0 +1,144 @@ | |||
| org.springframework.util;AntPathMatcher;false;combine;;;Argument[0..1];ReturnValue;taint | |||
There was a problem hiding this comment.
What is this file? Something committed by mistake?
There was a problem hiding this comment.
It's the file used to generate the original form of the tests.
There was a problem hiding this comment.
I've removed it since I didn't use it to regenerate test cases for the changes I made; not sure whether you wanted it in the first place.
| /** | ||
| * Provides models for the `org.springframework.util` package. | ||
| */ | ||
| module SpringUtil { |
There was a problem hiding this comment.
No need for this module.
| "org.springframework.util;AntPathMatcher;false;doMatch;;;Argument[0];MapValue of Argument[2];taint", | ||
| "org.springframework.util;AntPathMatcher;false;extractPathWithinPattern;;;Argument[1];ReturnValue;taint", | ||
| "org.springframework.util;AntPathMatcher;false;extractUriTemplateVariables;;;Argument[1];MapValue of ReturnValue;taint", | ||
| // Return values here are arrays, but there's no value in adding `ArrayValue of` for taint, right? |
There was a problem hiding this comment.
Yes, I'd say there is.
| @@ -0,0 +1,53 @@ | |||
| import java | |||
| import semmle.code.java.dataflow.DataFlow | |||
| import semmle.code.java.dataflow.ExternalFlow | |||
There was a problem hiding this comment.
This import looks unused.
| import semmle.code.java.dataflow.ExternalFlow |
| // Test case generated by GenerateFlowTestCase.ql | ||
| public class Test { | ||
|
|
||
| Object getArrayElement(Object container) { return ((Object[])container)[0]; } |
There was a problem hiding this comment.
| Object getArrayElement(Object container) { return ((Object[])container)[0]; } | |
| Object getArrayElement(Object[] container) { return container[0]; } |
| Object getMapKey(Map container) { return container.keySet().iterator().next(); } | ||
| Object getMapValue(Map container) { return container.get(null); } | ||
| Object[] newWithArrayElement(Object element) { return new Object[] {element}; } | ||
| // Object newWithMapKey(Object element) { return null; } |
There was a problem hiding this comment.
| // Object newWithMapKey(Object element) { return null; } |
| Object getMapValue(Map container) { return container.get(null); } | ||
| Object[] newWithArrayElement(Object element) { return new Object[] {element}; } | ||
| // Object newWithMapKey(Object element) { return null; } | ||
| Properties newPropertiesWithMapKey(Object element) { Properties p = new Properties(); p.put(element, null); return p; } |
There was a problem hiding this comment.
Some indentation gone wrong here.
| Object[] newWithArrayElement(Object element) { return new Object[] {element}; } | ||
| // Object newWithMapKey(Object element) { return null; } | ||
| Properties newPropertiesWithMapKey(Object element) { Properties p = new Properties(); p.put(element, null); return p; } | ||
| // Object newWithMapValue(Object element) { return null; } |
There was a problem hiding this comment.
| // Object newWithMapValue(Object element) { return null; } |
| sink(out.get("someKey")); // $hasTaintFlow | ||
| } | ||
| { | ||
| // "org.springframework.util;AntPathMatcher;false;tokenizePath;;;Argument[0];ArrayValue of ReturnValue;taint", |
There was a problem hiding this comment.
More indentation gone wrong. Is this file mixing tabs and spaces by any chance?
| { | ||
| // "org.springframework.util;CollectionUtils;false;arrayToList;;;ArrayElement of Argument[0];Element of ReturnValue;value" | ||
| List out = null; | ||
| Object in = (Object)newWithArrayElement(source()); |
There was a problem hiding this comment.
| Object in = (Object)newWithArrayElement(source()); | |
| Object[] in = newWithArrayElement(source()); |
| { | ||
| // "org.springframework.util;CollectionUtils;false;mergeArrayIntoCollection;;;ArrayElement of Argument[0];Element of Argument[1];value" | ||
| Collection out = null; | ||
| Object in = (Object)newWithArrayElement(source()); |
There was a problem hiding this comment.
| Object in = (Object)newWithArrayElement(source()); | |
| Object[] in = newWithArrayElement(source()); |
Remove unused import; drop unused util methods and needless casts.
| MultiValueMap out = null; | ||
| Map in = Map.of(null, List.of(source())); | ||
| out = CollectionUtils.toMultiValueMap(in); | ||
| sink(getElement((List)getMapValue(out))); // $hasValueFlow |
There was a problem hiding this comment.
If you improve the type of getMapValue then this cast can be removed.
<K,V> V getMapValue(Map<K,V> container)
| // "org.springframework.util;CollectionUtils;false;unmodifiableMultiValueMap;;;MapValue of Argument[0];MapValue of ReturnValue;value" | ||
| MultiValueMap out = null; | ||
| MultiValueMap in = new LinkedMultiValueMap(); | ||
| in.put(null, source()); |
| // "org.springframework.util;CompositeIterator;false;next;;;Element of Argument[-1];ReturnValue;value" | ||
| Object out = null; | ||
| CompositeIterator in = new CompositeIterator(); | ||
| in.add(List.of(source()).iterator()); |
| LinkedMultiValueMap out = null; | ||
| Map in = Map.of(null, List.of(source())); | ||
| out = new LinkedMultiValueMap(in); | ||
| sink(getElement((List)getMapValue(out))); // $hasValueFlow |
| { | ||
| // "org.springframework.util;LinkedMultiValueMap;false;deepCopy;;;MapKey of Argument[-1];MapValue of ReturnValue;value" | ||
| LinkedMultiValueMap out = null; | ||
| LinkedMultiValueMap in = (LinkedMultiValueMap)Map.of(source(), null); |
There was a problem hiding this comment.
This is slightly cheating, but type pruning won't be able to catch this at the moment.
| { | ||
| // "org.springframework.util;MultiValueMap;true;add;;;Argument[0];MapKey of Argument[-1];value" | ||
| MultiValueMap out = null; | ||
| Object in = (Object)source(); |
There was a problem hiding this comment.
Casting to Object is superfluous.
| Object in = (Object)source(); | |
| Object in = source(); |
There was a problem hiding this comment.
(More instances of this below)
| MultiValueMap out = null; | ||
| Object in = (Object)source(); | ||
| out.add((Object)null, in); | ||
| sink(getElement((List)getMapValue(out))); // $hasValueFlow |
There was a problem hiding this comment.
All of these casts to List can be removed with the above sketched improvement to getMapValue.
| { | ||
| // "org.springframework.util;MultiValueMap;true;set;;;Argument[0];MapKey of Argument[-1];value" | ||
| MultiValueMapAdapter out = null; | ||
| Object in = (Object)source(); | ||
| out.set(in, null); | ||
| sink(getMapKey(out)); // $hasValueFlow | ||
| } | ||
| { | ||
| // "org.springframework.util;MultiValueMap;true;set;;;Argument[0];MapKey of Argument[-1];value" | ||
| MultiValueMap out = null; | ||
| Object in = (Object)source(); | ||
| out.set(in, null); | ||
| sink(getMapKey(out)); // $hasValueFlow | ||
| } | ||
| { | ||
| // "org.springframework.util;MultiValueMap;true;set;;;Argument[0];MapKey of Argument[-1];value" | ||
| MultiValueMap out = null; | ||
| Object in = (Object)source(); | ||
| out.set(in, (Object)null); | ||
| sink(getMapKey(out)); // $hasValueFlow |
There was a problem hiding this comment.
These three look like duplicates of each other?
There was a problem hiding this comment.
@smowton Is this something that can be traced back to a bug in the test generation?
There was a problem hiding this comment.
Two differ (the top one is checking inheritance by MultiValueMapAdapter); dropped the last one. The duplicates may be a bug or may be manually introduced thereafter; will check offline.
|
@aschackmull all fixed; some of my commits overlap with your reviews in time; first new commit is "Import SpringUtil from ExternalFlow.qll" |
| import semmle.code.java.frameworks.spring.SpringRef | ||
| import semmle.code.java.frameworks.spring.SpringReplacedMethod | ||
| import semmle.code.java.frameworks.spring.SpringSet | ||
| import semmle.code.java.frameworks.spring.SpringUtil |
There was a problem hiding this comment.
Should this import be removed?
There was a problem hiding this comment.
I think it makes sense to keep. Spring.qll just aims to bulk-import all of the spring-related libraries and even though SpringUtil currently doesn't contain any public declarations, meaning that this import doesn't do anything, then any future public additions in SpringUtil ought to be imported by Spring.qll.
4 participants
No description provided.