Add models of the jakarta/javax.json package by smowton · Pull Request #6183 · github/codeql

smowton · 2021-06-29T14:21:23Z

Notes:

I've left the stream subpackage alone for now to come back to once we have lambda flow
I've used whole-object taint rather than tracking Element, MapValue etc here on the understanding that the purpose of the package is to build an object then serialise or the inverse, rather than to use these as general-purpose data structures where flows involving access paths are more plausible.
One possible shortcoming of that approach: JsonArray exposes a full immutable List interface; some reads through that interface against a deserialised object may not convey taint as a result.

Marcono1234

Would it make sense to cover jakarta.json.* as well now that the Eclipse Foundation has took over the project? See https://eclipse-ee4j.github.io/jsonp/

I have left some review comments regarding methods which might only exist for Jakarta JSON-P; feel free to ignore them if you don't want to cover JSON-P.
Though note that some of the comments might also apply to Java EE, but not to Java EE 7 documented on https://docs.oracle.com but to the newer versions, see https://javadoc.io/doc/javax.json/javax.json-api/latest/overview-summary.html

Should the class Json be modelled as well? It has multiple factory methods.

Marcono1234 · 2021-06-29T16:06:09Z

+        "javax.json;JsonArray;false;getJsonString;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonArray;false;getString;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonArray;false;getString;;;Argument[1];ReturnValue;value",
+        "javax.json;JsonArray;false;getValuesAs;;;Argument[-1];ReturnValue;taint",


Shouldn't that be value flow since getValueAs(Class) seems to return a view (without modifying the entries in any way)? (or is value flow only when it is the exact same reference?)
Though the overload getValueAs(Function) would only be taint flow then I guess.

It should be the exact same reference indeed

Marcono1234 · 2021-06-29T16:08:30Z

+        "javax.json;JsonReaderFactory;false;createReader;;;Argument[0];ReturnValue;taint",
+        "javax.json;JsonString;false;getChars;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonString;false;getString;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonValue;false;toString;;;Argument[-1];ReturnValue;taint",


Shouldn't this cover subtypes as well? (hopefully my understanding of the CSV model is correct)

Suggested change

"javax.json;JsonValue;false;toString;;;Argument[-1];ReturnValue;taint",

"javax.json;JsonValue;true;toString;;;Argument[-1];ReturnValue;taint",

Marcono1234 · 2021-06-29T16:10:10Z

+        "javax.json;JsonReaderFactory;false;createReader;;;Argument[0];ReturnValue;taint",
+        "javax.json;JsonString;false;getChars;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonString;false;getString;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonValue;false;toString;;;Argument[-1];ReturnValue;taint",


Should that also cover the following?

asJsonArray()

asJsonObject()

Marcono1234 · 2021-06-29T16:12:30Z

+        "javax.json;JsonReaderFactory;false;createReader;;;Argument[0];ReturnValue;taint",
+        "javax.json;JsonString;false;getChars;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonString;false;getString;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonValue;false;toString;;;Argument[-1];ReturnValue;taint",


Should JsonStructure.getValue(String) be covered as well?

Marcono1234 · 2021-06-29T16:14:14Z

+        "javax.json;JsonArray;false;getString;;;Argument[1];ReturnValue;value",
+        "javax.json;JsonArray;false;getValuesAs;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonArrayBuilder;false;add;;;Argument[-1];ReturnValue;value",
+        "javax.json;JsonArrayBuilder;false;add;;;Argument[0];Argument[-1];taint",


This might result in false positives since there are also overloads in the form add(index, ...) (which are in turn also not covered)

Marcono1234 · 2021-06-29T16:16:04Z

+        "javax.json;JsonArray;false;getValuesAs;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonArrayBuilder;false;add;;;Argument[-1];ReturnValue;value",
+        "javax.json;JsonArrayBuilder;false;add;;;Argument[0];Argument[-1];taint",
+        "javax.json;JsonArrayBuilder;false;addNull;;;Argument[-1];ReturnValue;value",


Jakarta JSON-P also has the following methods JsonArrayBuilder methods:

addAll

remove(...)

set(...)

setNull(int)

Marcono1234 · 2021-06-29T16:23:55Z

+        "javax.json;JsonArrayBuilder;false;add;;;Argument[0];Argument[-1];taint",
+        "javax.json;JsonArrayBuilder;false;addNull;;;Argument[-1];ReturnValue;value",
+        "javax.json;JsonArrayBuilder;false;build;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonNumber;false;bigDecimalValue;;;Argument[-1];ReturnValue;taint",


~~Isn't at least bigDecimalValue() value flow since there is (most likely) no precision loss?~~
~~(Or maybe I am misunderstanding the meaning of value)~~

Edit: Nevermind, see #6183 (comment)

Marcono1234 · 2021-06-29T16:24:59Z

+        "javax.json;JsonNumber;false;intValue;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonNumber;false;intValueExact;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonNumber;false;longValue;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonNumber;false;longValueExact;;;Argument[-1];ReturnValue;taint",


There is also numberValue(); that one might be value flow?

Marcono1234 · 2021-06-29T16:28:28Z

+        "javax.json;JsonObjectBuilder;false;add;;;Argument[-1];ReturnValue;value",
+        "javax.json;JsonObjectBuilder;false;add;;;Argument[1];Argument[-1];taint",
+        "javax.json;JsonObjectBuilder;false;addNull;;;Argument[-1];ReturnValue;value",
+        "javax.json;JsonObjectBuilder;false;build;;;Argument[-1];ReturnValue;taint",


There are also the following JsonObjectBuilder methods:

addAll

remove

@smowton I think you missed JsonObjectBuilder.remove

Marcono1234 · 2021-06-29T16:29:52Z

+        "javax.json;JsonObjectBuilder;false;build;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonReader;false;read;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonReader;false;readArray;;;Argument[-1];ReturnValue;taint",
+        "javax.json;JsonReader;false;readObject;;;Argument[-1];ReturnValue;taint",


There is also JsonReader.readValue()

smowton · 2021-06-30T14:06:01Z

Thanks @Marcono1234, I've improved this to cover the jakarta version of the package and to incorporate the missing functions you mention.

I'll edit the top comment with notes on taint strategy here and the uncovered subpackages

Marcono1234

Thanks a lot for the changes. Usage of a set literal expression for supporting the different package names is a pretty neat idea!

Since you have covered some additional classes as well, would it make sense to cover JsonPointer, JsonMergePatch and JsonPatchBuilder from the same package as well?

Marcono1234 · 2021-07-04T23:41:06Z

+          "Json;false;createDiff;;;Argument[0..1];ReturnValue;taint",
+          "Json;false;createMergeDiff;;;Argument[0..1];ReturnValue;taint",


Not completely sure whether argument 0 (source) is actually affecting taint. Did not have a look at the implementation or the RFC, but if I understand it correctly source can only affect whether JSON values are added or removed, but not which values they will have.
E.g.:

source has "a": 1, target does not have it → a is removed

source does not have a, target has "a": 1 → "a": 1 is added

In both cases source does not taint the patch.
But might be good to verify this, also for JSON arrays.

I figured the diff must represent the removal of the key somehow, and <script>alert('Oh no')</script> is a valid key, so we might be interested in this as a taint propagator

Marcono1234 · 2021-07-04T23:48:13Z

+          "Json;false;createObjectBuilder;(Map);;MapValue of Argument[0];ReturnValue;taint",
+          "Json;false;createPatch;;;Argument[0];ReturnValue;taint",
+          "Json;false;createReader;;;Argument[0];ReturnValue;taint",
+          "Json;false;createWriter;;;Argument[0];ReturnValue;taint",


In case you decide to add JsonPatchBuilder it would be good to add Json.createPatchBuilder(JsonArray) as well.

Marcono1234 · 2021-07-04T23:49:44Z

+          "Json;false;createObjectBuilder;(Map);;MapValue of Argument[0];ReturnValue;taint",
+          "Json;false;createPatch;;;Argument[0];ReturnValue;taint",
+          "Json;false;createReader;;;Argument[0];ReturnValue;taint",
+          "Json;false;createWriter;;;Argument[0];ReturnValue;taint",


Would probably be good to cover the createValue(...) methods as well.

Marcono1234 · 2021-07-06T15:59:44Z

@@ -0,0 +1,110 @@
+/**
+ * Provides models for the `javax.json` package.


Maybe also have to adjust this:

Suggested change

* Provides models for the `javax.json` package.

* Provides models for the `javax.json` and `jakarta.json` packages.

github-actions · 2021-07-12T17:11: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:

-    Java extensions,"``javax.*``, ``jakarta.*``",22,294,18,,,,,1,1,2
+    Java extensions,"``javax.*``, ``jakarta.*``",22,536,18,,,,,1,1,2
-    Totals,,84,2013,287,13,6,6,98,33,1,66
+    Totals,,84,2255,287,13,6,6,98,33,1,66

Changes to framework-coverage-java.csv:

+ jakarta.json,,,121,,,,,,,,,,,,,,,98,23
+ javax.json,,,121,,,,,,,,,,,,,,,98,23

smowton · 2021-07-12T17:12:53Z

@Marcono1234 all changes applied

Marcono1234

Thanks a lot for considering the feedback!

Do you think it would be worth considering methods for de- and encoding a JSON pointer string as well?

Marcono1234 · 2021-07-12T19:33:52Z

+          ".json;JsonPatchBuilder;false;replace;;;Argument[-1];ReturnValue;value",
+          ".json;JsonPatchBuilder;false;test;;;Argument[0..1];ReturnValue;taint",
+          ".json;JsonPatchBuilder;false;test;;;Argument[-1];ReturnValue;value",
+          ".json;JsonPointer;false;add;;;Argument[-1];ReturnValue;taint",


Is add really transferring taint from the receiver JsonPointer?
If so should that then in the same way apply to the other methods (getValue, remove, replace)?

add: yes, because the pointer could contain an injection string, which would be incorporated into the result as a new key. getValue, remove, replace: no, they do not incorporate text from the pointer into the result (replace requires that the key is already present)

Thanks for the clarification! Makes sense.

smowton · 2021-07-13T10:10:43Z

Done

github-actions · 2021-07-13T10:13:24Z

⚠️ 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:

-    Java extensions,"``javax.*``, ``jakarta.*``",22,294,18,,,,,1,1,2
+    Java extensions,"``javax.*``, ``jakarta.*``",22,540,18,,,,,1,1,2
-    Totals,,84,2013,287,13,6,6,98,33,1,66
+    Totals,,84,2259,287,13,6,6,98,33,1,66

Changes to framework-coverage-java.csv:

+ jakarta.json,,,123,,,,,,,,,,,,,,,100,23
+ javax.json,,,123,,,,,,,,,,,,,,,100,23

Add models of the javax.json package

ba5dc3c

smowton requested a review from a team as a code owner June 29, 2021 14:21

github-actions Bot added documentation Java labels Jun 29, 2021

Marcono1234 reviewed Jun 29, 2021

View reviewed changes

Also cover jakarta version of javax.json, and some missed methods

753c878

smowton changed the title ~~Add models of the javax.json package~~ Add models of the jakarta/javax.json package Jun 30, 2021

Marcono1234 reviewed Jul 4, 2021

View reviewed changes

Marcono1234 reviewed Jul 6, 2021

View reviewed changes

smowton added 4 commits July 12, 2021 17:13

Add missing model of JsonObjectBuilder.remove

6bf9313

Add models of JsonMergePatch, JsonPatchBuilder and JsonPointer

5398594

Add models of JsonPointer, JsonMergeDiff and JsonPatchBuilder

cc4401b

Improve header comment

2bd58d6

Marcono1234 reviewed Jul 12, 2021

View reviewed changes

Add models for decode/encodePointer methods

78fe0f8

aschackmull approved these changes Jul 19, 2021

View reviewed changes

aschackmull merged commit c32a75a into github:main Jul 19, 2021

	"javax.json;JsonValue;false;toString;;;Argument[-1];ReturnValue;taint",
	"javax.json;JsonValue;true;toString;;;Argument[-1];ReturnValue;taint",

		"Json;false;createDiff;;;Argument[0..1];ReturnValue;taint",
		"Json;false;createMergeDiff;;;Argument[0..1];ReturnValue;taint",

		@@ -0,0 +1,110 @@
		/**
		* Provides models for the `javax.json` package.

	* Provides models for the `javax.json` package.
	* Provides models for the `javax.json` and `jakarta.json` packages.

Conversation

smowton commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Jun 30, 2021

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 12, 2021

java

Generated file changes for java

Uh oh!

smowton commented Jul 12, 2021

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Jul 13, 2021

Uh oh!

github-actions Bot commented Jul 13, 2021

java

Generated file changes for java

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smowton commented Jun 29, 2021 •

edited

Loading

Marcono1234 Jun 29, 2021 •

edited

Loading