Java: Add models for the Spring `web.util` package #5953

sauyon · 2021-05-25T19:16:01Z

No description provided.

github-actions · 2021-07-15T16:19:07Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

aschackmull · 2021-07-16T10:11:06Z

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

+        "java.util;HashMap;false;HashMap;;;MapKey of Argument[0];MapKey of Argument[-1];value",
+        "java.util;HashMap;false;HashMap;;;MapValue of Argument[0];MapValue of Argument[-1];value",


I think this ought to be restricted with a signature - no need to match all the constructors.

aschackmull · 2021-07-19T10:50:48Z

java/ql/test/library-tests/frameworks/spring/webutil/test.ql

+class SummaryModelTest extends SummaryModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        //"package;type;overrides;name;signature;ext;inputspec;outputspec;kind",
+        "generatedtest;Test;false;getMapKey;;;MapKey of Argument[0];ReturnValue;value",
+        "generatedtest;Test;false;getMapValue;;;MapValue of Argument[0];ReturnValue;value"
+      ]
+  }
+}


You should be able to rebase and remove this now.

github-actions · 2021-07-22T14:24:01Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

github-actions · 2021-07-22T15:36:44Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

sauyon · 2021-07-22T15:36:52Z

This PR should be good to go now that my constructor PR is merged.

aschackmull · 2021-07-26T09:10:23Z

Looks like there's a bad row somewhere:

 invalidModelRow
+| Wrong number of columns in summary model row, expected 9, got 10. |

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

sauyon · 2021-07-26T14:35:33Z

Oops, forgot to actually push the fixes. I missed the test generator error for that one, have now double checked.

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

sauyon · 2021-08-04T09:38:58Z

I've now actually added a test for the sanitizer and confirmed that at least the webutil test passes. This review cycle definitely shouldn't have happened, sorry.

aschackmull · 2021-08-05T13:20:02Z

java/ql/src/semmle/code/java/security/XSS.qll

@@ -94,6 +94,8 @@ private class DefaultXssSink extends XssSink {
 private class DefaultXSSSanitizer extends XssSanitizer {
  DefaultXSSSanitizer() {
    this.getType() instanceof NumericType or this.getType() instanceof BooleanType
+    or
+    this.asExpr().(MethodAccess).getMethod().getName().regexpMatch("(?i)html_?escape.*")


Suggested change

this.asExpr().(MethodAccess).getMethod().getName().regexpMatch("(?i)html_?escape.*")

// Match `org.springframework.web.util.HtmlUtils.htmlEscape` and possibly other methods like it.

this.asExpr().(MethodAccess).getMethod().getName().regexpMatch("(?i)html_?escape.*")

aschackmull · 2021-08-05T13:23:44Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;UriBuilder;true;queryParamIfPresent;;;Argument[0];Argument[-1];taint",
+        "org.springframework.web.util;UriBuilder;true;queryParamIfPresent;;;Element of Argument[1];Argument[-1];taint",
+        "org.springframework.web.util;UriBuilder;true;queryParams;;;Argument[-1];ReturnValue;value",
+        "org.springframework.web.util;UriBuilder;true;queryParams;;;MapKey of Argument[0];SyntheticField[uri.Query] ofArgument[-1];taint",


Typo. This should be caught by our tests. If it isn't: find out why.

github-actions · 2021-08-08T21:48:29Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Spring <https://spring.io/>`_,``org.springframework.*``,29,306,91,,,,19,14,,29
+    `Spring <https://spring.io/>`_,``org.springframework.*``,29,467,91,,,,19,14,,29
-    Totals,,84,2705,398,13,6,6,107,33,1,66
+    Totals,,84,2866,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

+ org.springframework.web.util,,,161,,,,,,,,,,,,,,,,,,,136,25

sauyon · 2021-08-08T21:48:33Z

Fixed. The reason the tests didn't catch it is threefold:

The test generator generates tests for UriBuilder using UriComponentsBuilder,
I'd added redundant models for methods of UriComponentBuilder, and
the models for UriComponentBuilder applied to calls on UriBuilder, presumably because there was only one implementation of UriBuilder.

I also added tests for the HTML utility functions that I'd added models for.

Co-Authored-By: Anders Schack-Mulligen <[email protected]>

github-actions · 2021-08-12T18:23:19Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Spring <https://spring.io/>`_,``org.springframework.*``,29,306,91,,,,19,14,,29
+    `Spring <https://spring.io/>`_,``org.springframework.*``,29,467,91,,,,19,14,,29
-    Totals,,84,2711,398,13,6,6,107,33,1,66
+    Totals,,84,2872,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

+ org.springframework.web.util,,,161,,,,,,,,,,,,,,,,,,,136,25

smowton · 2021-08-16T17:25:18Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;UriTemplateHandler;true;expand;;;Argument[-1..0];ReturnValue;taint",
+        "org.springframework.web.util;UriTemplateHandler;true;expand;(String,Map);;MapValue of Argument[1];ReturnValue;taint",
+        "org.springframework.web.util;UriTemplateHandler;true;expand;(String,Object[]);;ArrayElement of Argument[1];ReturnValue;taint",


These are defined on the abstract superclass too

I assume you meant the DefaultUriBuilderFactory models below; I've removed them.

I meant they're specified by https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/util/AbstractUriTemplateHandler.html like getBaseUrl et al below, so should be modelled against it for consistency

It seems to me like AbstractUriTemplateHandler is an implementation of UriTemplateHandler?

Doh yes sorry, I read the class hierarchy backwards

github-actions · 2021-08-17T09:19:54Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Spring <https://spring.io/>`_,``org.springframework.*``,29,306,91,,,,19,14,,29
+    `Spring <https://spring.io/>`_,``org.springframework.*``,29,464,91,,,,19,14,,29
-    Totals,,84,2711,398,13,6,6,107,33,1,66
+    Totals,,84,2869,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

+ org.springframework.web.util,,,158,,,,,,,,,,,,,,,,,,,133,25

smowton · 2021-08-17T09:31:18Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;UriTemplateHandler;true;expand;;;Argument[-1..0];ReturnValue;taint",
+        "org.springframework.web.util;UriTemplateHandler;true;expand;(String,Map);;MapValue of Argument[1];ReturnValue;taint",
+        "org.springframework.web.util;UriTemplateHandler;true;expand;(String,Object[]);;ArrayElement of Argument[1];ReturnValue;taint",


I meant they're specified by https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/util/AbstractUriTemplateHandler.html like getBaseUrl et al below, so should be modelled against it for consistency

smowton · 2021-08-17T09:45:15Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;ContentCachingRequestWrapper;false;ContentCachingRequestWrapper;;;Argument[0];Argument[-1];taint",
+        "org.springframework.web.util;ContentCachingRequestWrapper;false;getContentAsByteArray;;;Argument[-1];ReturnValue;taint",
+        "org.springframework.web.util;ContentCachingResponseWrapper;false;ContentCachingResponseWrapper;;;Argument[0];Argument[-1];taint",
+        "org.springframework.web.util;ContentCachingResponseWrapper;false;getContentAsByteArray;;;Argument[-1];ReturnValue;taint",
+        "org.springframework.web.util;ContentCachingResponseWrapper;false;getContentInputStream;;;Argument[-1];ReturnValue;taint",


Worth noting for a future task: we don't currently model the superclass javax.servlet.[Http]ServletResponseWrapper

smowton · 2021-08-17T10:02:16Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;UriComponents;false;getScheme;;;Argument[-1];ReturnValue;taint",
+        "org.springframework.web.util;UriComponents;false;getSchemeSpecificPart;;;Argument[-1];ReturnValue;taint",
+        "org.springframework.web.util;UriComponents;false;getUserInfo;;;Argument[-1];ReturnValue;taint",
+        "org.springframework.web.util;UriComponents;false;toUri;;;Argument[-1];ReturnValue;taint",


I think normalize and toString should both be taint propagating here

smowton · 2021-08-17T10:07:58Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;UriTemplate;false;expand;(Map);;MapValue of Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;UriTemplate;false;expand;(Object[]);;ArrayElement of Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;UriTemplate;false;getVariableNames;;;Argument[-1];Element of ReturnValue;taint",
+        "org.springframework.web.util;UriTemplate;false;match;;;Argument[0];MapValue of ReturnValue;taint",


to check: probably has a taint-propagating toString?

toString returns the template, so I would assume yes.

smowton · 2021-08-17T10:14:09Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;UrlPathHelper;false;getOriginatingQueryString;;;Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;UrlPathHelper;false;getOriginatingRequestUri;;;Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;UrlPathHelper;false;getOriginatingServletPath;;;Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;UrlPathHelper;false;getRequestUri;;;Argument[0];ReturnValue;taint",


I think getPathWithinServletMapping etc are taint propagating (sounds like if you servlet is mounted at a and the user requested a/b then that would yield /b?) I'd appreciate a once-over from someone who knows their servlets though regarding which of these methods in practice yields developer/admin-specified config information and which yield user/remote-specified data.

This sounds right to me.

I've removed getOriginatingServletPath since that seems like it would be admin-specified.

smowton · 2021-08-17T10:15:43Z

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll

+        "org.springframework.web.util;WebUtils;false;getRequiredSessionAttribute;;;Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;WebUtils;false;getSessionAttribute;;;Argument[0];ReturnValue;taint",
+        "org.springframework.web.util;WebUtils;false;parseMatrixVariables;;;Argument[0];MapKey of ReturnValue;taint",
+        "org.springframework.web.util;WebUtils;false;parseMatrixVariables;;;Argument[0];MapValue of ReturnValue;taint"


Model setters?

I'm unsure why I didn't model the session variable setter, but I excluded the system property setter because it doesn't seem easily modelable.

github-actions · 2021-08-17T19:48:18Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Spring <https://spring.io/>`_,``org.springframework.*``,29,306,91,,,,19,14,,29
+    `Spring <https://spring.io/>`_,``org.springframework.*``,29,469,91,,,,19,14,,29
-    Totals,,84,2711,398,13,6,6,107,33,1,66
+    Totals,,84,2874,398,13,6,6,107,33,1,66

Changes to framework-coverage-java.csv:

+ org.springframework.web.util,,,163,,,,,,,,,,,,,,,,,,,138,25

sauyon requested a review from a team as a code owner May 25, 2021 19:16

sauyon marked this pull request as draft May 25, 2021 19:16

github-actions bot added the Java label May 25, 2021

sauyon force-pushed the sauyon/java/spring-webutil branch from ae2f6ea to 0264aff Compare May 29, 2021 19:15

sauyon force-pushed the sauyon/java/spring-webutil branch from 0264aff to 1d6726a Compare July 14, 2021 12:24

sauyon marked this pull request as ready for review July 14, 2021 12:26

github-actions bot added the documentation label Jul 14, 2021

sauyon force-pushed the sauyon/java/spring-webutil branch from 55ffef8 to 6b53162 Compare July 15, 2021 14:49

github deleted a comment from github-actions bot Jul 15, 2021

sauyon force-pushed the sauyon/java/spring-webutil branch 2 times, most recently from 34a36c4 to 3d9808b Compare July 15, 2021 16:16

github deleted a comment from github-actions bot Jul 15, 2021

aschackmull reviewed Jul 16, 2021

View reviewed changes

aschackmull mentioned this pull request Jul 19, 2021

Java: Fix a bug in call-context-sensitve dispatch to SummarizedCallable. #6313

Merged

aschackmull reviewed Jul 19, 2021

View reviewed changes

sauyon force-pushed the sauyon/java/spring-webutil branch 2 times, most recently from 467148d to d34602f Compare July 22, 2021 14:23

sauyon force-pushed the sauyon/java/spring-webutil branch from d34602f to d468388 Compare July 22, 2021 15:34

aschackmull reviewed Jul 26, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll Outdated Show resolved Hide resolved

github deleted a comment from github-actions bot Jul 27, 2021

sauyon force-pushed the sauyon/java/spring-webutil branch from 12ddb73 to ebb87a0 Compare July 27, 2021 04:55

aschackmull reviewed Jul 27, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 27, 2021

View reviewed changes

java/ql/src/semmle/code/java/frameworks/spring/SpringWebUtil.qll Outdated Show resolved Hide resolved

sauyon force-pushed the sauyon/java/spring-webutil branch 2 times, most recently from 0ccfad7 to 9c35332 Compare August 4, 2021 09:38

aschackmull reviewed Aug 5, 2021

View reviewed changes

sauyon force-pushed the sauyon/java/spring-webutil branch from 9c35332 to 627dbc0 Compare August 8, 2021 21:46

github deleted a comment from github-actions bot Aug 8, 2021

Sauyon Lee and others added 8 commits August 12, 2021 11:20

Java: Add models for the Spring web.util package

25649a6

Add stubs for Spring web.util tests

fd0ea15

Add tests for Spring web.util

d86dffb

Add change note

eb980e2

Java: Add HTML escapes as XSS sanitizers

9a5c0f6

Co-Authored-By: Anders Schack-Mulligen <[email protected]>

Java: Add test for XSS sanitizer

9c1d5a7

Java: Remove redundant models from Spring web.util and fix typo

ed1d855

Add tests for html escape functions

814004e

sauyon force-pushed the sauyon/java/spring-webutil branch from 627dbc0 to 814004e Compare August 12, 2021 18:20

Autoformat

ff3f85b

smowton reviewed Aug 16, 2021

View reviewed changes

Remove more redundant models

390e48f

smowton reviewed Aug 17, 2021

View reviewed changes

Address review comments

17cef3f

smowton approved these changes Aug 18, 2021

View reviewed changes

smowton merged commit cc4fe73 into main Aug 18, 2021

smowton deleted the sauyon/java/spring-webutil branch August 18, 2021 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add models for the Spring `web.util` package #5953

Java: Add models for the Spring `web.util` package #5953

sauyon commented May 25, 2021

github-actions bot commented Jul 15, 2021

aschackmull Jul 16, 2021

aschackmull Jul 19, 2021

github-actions bot commented Jul 22, 2021

github-actions bot commented Jul 22, 2021

sauyon commented Jul 22, 2021

aschackmull commented Jul 26, 2021

sauyon commented Jul 26, 2021

sauyon commented Aug 4, 2021

aschackmull Aug 5, 2021

aschackmull Aug 5, 2021

github-actions bot commented Aug 8, 2021

sauyon commented Aug 8, 2021

github-actions bot commented Aug 12, 2021

smowton Aug 16, 2021

sauyon Aug 17, 2021

smowton Aug 17, 2021

sauyon Aug 17, 2021

smowton Aug 17, 2021

github-actions bot commented Aug 17, 2021

smowton Aug 17, 2021

smowton Aug 17, 2021

smowton Aug 17, 2021

smowton Aug 17, 2021

sauyon Aug 17, 2021

smowton Aug 17, 2021

sauyon Aug 17, 2021

sauyon Aug 17, 2021

smowton Aug 17, 2021

sauyon Aug 17, 2021

github-actions bot commented Aug 17, 2021

		"java.util;HashMap;false;HashMap;;;MapKey of Argument[0];MapKey of Argument[-1];value",
		"java.util;HashMap;false;HashMap;;;MapValue of Argument[0];MapValue of Argument[-1];value",

	this.asExpr().(MethodAccess).getMethod().getName().regexpMatch("(?i)html_?escape.*")
	// Match `org.springframework.web.util.HtmlUtils.htmlEscape` and possibly other methods like it.
	this.asExpr().(MethodAccess).getMethod().getName().regexpMatch("(?i)html_?escape.*")

Java: Add models for the Spring web.util package #5953

Java: Add models for the Spring web.util package #5953

Conversation

sauyon commented May 25, 2021

github-actions bot commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2021

github-actions bot commented Jul 22, 2021

sauyon commented Jul 22, 2021

aschackmull commented Jul 26, 2021

sauyon commented Jul 26, 2021

sauyon commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 8, 2021

java

Generated file changes for java

sauyon commented Aug 8, 2021

github-actions bot commented Aug 12, 2021

java

Generated file changes for java

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2021

java

Generated file changes for java

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2021

java

Generated file changes for java

Java: Add models for the Spring `web.util` package #5953

Java: Add models for the Spring `web.util` package #5953