Java: add websocket reads as remote flow source.

[ghost]

Currently, JAX-WS reads are considered as untrusted. However, java.net.http.WebSocket reads are not marked as such. This PR adds support for the same.

[aschackmull]

Overall LGTM, but the qldoc needs some minor fixups, and the test doesn't run (likely needs additional stubs).

However, I wasn't able to find many uses of this in the wild: https://lgtm.com/query/7727712516629826150/
Do you think this is because it isn't used much, or is there some subtle bug in the modeling that prevents us from finding more?

[aschackmull]

Suggested change

	ws.get().sendText​(message, false);
	ws.get().sendText(message, false);

Remove U+200B character.

[ghost]

@aschackmull This is dependent on github/securitylab#142

The WebSocket package was only available starting Java 11, CodeQL extractor by default uses Java 8. Hence, the tests don't compile and run. The test code does not depend on any third party libraries so there's nothing I need to stub here.

As for the positive result on LGTM, I dont know why only that one was detected. Github's fuzzy search returns about 4000+ results for websocket onText So we should be seeing more results on LGTM provided the query runs correctly.

[intrigus-lgtm]

As for the positive result on LGTM, I dont know why only that one was detected. Github's fuzzy search returns about 4000+ results for websocket onText So we should be seeing more results on LGTM provided the query runs correctly.

Websocket are pretty new.
They have been added with Java 11.

Most of Github's fuzzy search results are in forks of the jdk sources.

[aschackmull]

For the test you can add a file called options next to the test containing the following single line:

//semmle-extractor-options: --javac-args -source 11 -target 11

That makes the test run with Java 11 instead of 8.

[ghost]

@aschackmull Unfortunately, that does not work. You may see in the javac-extractor-*.log, the command generated is

sun.java.command=com.semmle.extractor.java.JavaExtractor --strict-javac-errors --javac-args -source 8 -target 8 -source 11 -target 11 -cp <Redacted>/java/ql/test/query-tests/security/CWE-079/semmle/tests/../../../../../stubs/servlet-api-2.4/ -encoding UTF-8 WebsocketXss.java XSS.java

The -source 8 and -target 8 flags are added regardless of whether they are specified explicitly. This looks like a bug in the extractor to me. WDYT?

[aschackmull]

I've done some debugging and found the reason why this qltest fails to compile. For the sake of reproducibility we run all the qltests in a fixed and fairly minimal JDK, and this doesn't include the java.net.http module. I've tried to locally upgrade the qltest JDK to include the java.net.http module, and then the test compiles.

For this PR, I think the best way forward is to commit a completely commented-out version of the WebsocketXss.java test file - then I'll test it locally and work to enable it once the tooling supports the java.net.http module.

Btw., when running the test locally I found that it didn't produce any new results - I guess the XSS sink is also missing. Presumably WebSocket.sendText should be considered an XSS sink? (I'm not familiar with WebSockets, so please correct me if I'm wrong here). Do you want to fix this here, or should we address this in a separate PR?

[ghost]

@aschackmull Yes, the sendText function is a potential sink. However. it can be used to send almost anything even json responses. By marking all sendText calls as sinks, we may significantly increase the number of false postiives in our results.
One more important point to note is that the sent message may be sanitized on the client side too. So we have no way of knowing for sure if a true positive is actually a true positive.

For my corresponding Golang PR, I had included the send calls as sinks but those were rejected by the team and hence had to be removed. See this comment here for the context.

That's the reason I didn't include them in this one. Please let me know if you think otherwise. I would be more than happy to add them here.

I have comment out the .java file and rebase it on the latest master. Sorry for the force push. The commit somehow got messed up during merge.

[aschackmull]

Alright, this looks almost ready to merge, then. Could I just ask you to remove the U+200B character in WebsocketXss.java? It's failing our ascii check. It's an invisible non-breaking space that comes from copy-pasting.

[ghost]

@aschackmull Done 👍