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

[src] Fix LGTM-reported issues. #1074

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

jonpryor
Copy link
Member

Remember CodeQL (5a0097b)? CodeQL basically runs GitHub LGTM on source code, looking for possible security issues.

Now that CodeQL is running, we can begin addressing reported issues.

Problems found include:

  • Result of call that may return NULL dereferenced unconditionally
  • HttpClient created with CheckCertificateRevocationList disabled
  • Arbitrary file write during archive extraction ("Zip Slip")
  • Local-user-controlled data in path expression

~~ Result of call that may return NULL dereferenced unconditionally ~~

If calloc(3) returns nullptr, we shouldn't pass it on to MultiByteToWideChar() or WideCharToMultiByte() without validation.

~~ HttpClient created with CheckCertificateRevocationList disabled ~~

Apparently the HttpClient default constructor is "bad"; we should instead use the HttpClient(HttpMessageHandler) constructor, provide our own HttpClientHandler, and ensure that HttpClientHandler.CheckCertificateRevocationList is True.

~~ Arbitrary file write during archive extraction ("Zip Slip") ~~

tools/java-source-utils (69e1b80) extracts the contents of .jar files to look for .java files to use for type resolution, as I couldn't find an easier way to get com.github.javaparser to use Java source code for type resolution purposes unless the Java source code was on-disk. Unfortunately, the .jar extraction code was susceptible to "Zip Slip", wherein an entry in the .jar may overwrite unexpected data if it has an entry name of e.g. ../../this/is/really/bad. Fix this by verifying that the target filename stays within the target directory structure.

~~ Local-user-controlled data in path expression ~~

LGTM is complaining that tools/java-source-utils (69e1b80) accepts user-controlled data. These warnings will be ignored because the app is unusable without "user-controlled data"; consider these java-source-utils --help fragments:

Java type resolution options:
      --bootclasspath CLASSPATH
                             ':'-separated list of .jar files to use
                               for type resolution.
  -a, --aar FILE             .aar file to use for type resolution.
  -j, --jar FILE             .jar file to use for type resolution.
  -s, --source DIR           Directory containing .java files for type
                               resolution purposes.  DOES NOT parse all files.

These are all user-controlled, and they are necessary to allow java-source-utils to work.

Similarly:

Output file options:
  -P, --output-params FILE   Write method parameter names to FILE.
  -D, --output-javadoc FILE  Write Javadoc within XML container to FILE.

LGTM complains that --output-javadoc FILE accepts a user-controlled path which may control directory separator chars, and this is intentional; using it would be annoying if that weren't true!

These uses can be ignored by appending the comment
// lgtm [java/path-injection-local].

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Jan 12, 2023
Remember CodeQL (5a0097b)?  CodeQL basically runs [GitHub LGTM][0]
on source code, looking for possible security issues.

Now that CodeQL is running, we can begin addressing reported issues.

Problems found include:

  * Result of call that may return NULL dereferenced unconditionally
  * HttpClient created with CheckCertificateRevocationList disabled
  * Arbitrary file write during archive extraction ("Zip Slip")
  * Local-user-controlled data in path expression

~~ Result of call that may return NULL dereferenced unconditionally ~~

If **calloc**(3) returns `nullptr`, we shouldn't pass it on to
`MultiByteToWideChar()` or `WideCharToMultiByte()` without validation.

~~ HttpClient created with CheckCertificateRevocationList disabled ~~

Apparently the `HttpClient` default constructor is "bad"; we should
instead use the [`HttpClient(HttpMessageHandler)` constructor][1],
provide our own `HttpClientHandler`, and ensure that
[`HttpClientHandler.CheckCertificateRevocationList`][2] is True.

~~ Arbitrary file write during archive extraction ("Zip Slip") ~~

`tools/java-source-utils` (69e1b80) extracts the `.java` files
within `.jar`/`.aar`/.etc files to use for type resolution, as I
couldn't find an easier way to get `com.github.javaparser` to
use Java source code for type resolution purposes unless the Java
source code was on-disk.  Unfortunately, the `.jar` extraction code
was susceptible to "Zip Slip", wherein an entry in the `.jar` may
overwrite unexpected files if it has an entry name of e.g.
`../../this/is/really/bad.java`.  Fix this by verifying that the
target filename stays within the target directory structure, and
skip the entry when the name is invalid.

~~ Local-user-controlled data in path expression ~~

LGTM is complaining that `tools/java-source-utils` (69e1b80) accepts
user-controlled data.  These warnings will be *ignored* because the
app is *unusable* without "user-controlled data"; consider
these `java-source-utils --help` fragments:

	Java type resolution options:
	      --bootclasspath CLASSPATH
	                             ':'-separated list of .jar files to use
	                               for type resolution.
	  -a, --aar FILE             .aar file to use for type resolution.
	  -j, --jar FILE             .jar file to use for type resolution.
	  -s, --source DIR           Directory containing .java files for type
	                               resolution purposes.  DOES NOT parse all files.

These are all user-controlled, and they are necessary to allow
`java-source-utils` to *work*.

Similarly:

	Output file options:
	  -P, --output-params FILE   Write method parameter names to FILE.
	  -D, --output-javadoc FILE  Write Javadoc within XML container to FILE.

LGTM complains that `--output-javadoc FILE` accepts a user-controlled
path which may control directory separator chars, and
*this is intentional*; using it would be annoying if that weren't true!

These uses can be ignored by appending the comment
`// lgtm [java/path-injection-local]`.

[0]: https://github.com/marketplace/lgtm
[1]: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.-ctor?view=netstandard-2.0#system-net-http-httpclient-ctor(system-net-http-httpmessagehandler)
[2]: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler.checkcertificaterevocationlist?view=net-7.0
@jonpryor jonpryor force-pushed the jonp-lgtm-fixes-20230111 branch from 97658c6 to cc96072 Compare January 12, 2023 16:36
@jonpryor jonpryor merged commit e11d024 into dotnet:main Jan 12, 2023
jonpryor pushed a commit to dotnet/android that referenced this pull request Jan 24, 2023
Changes: dotnet/java-interop@cf80deb...1366d99

  * dotnet/java-interop@1366d998: [Java.Interop.Tools.JavaCallableWrappers] use less System.Linq for CAs (dotnet/java-interop#1072)
  * dotnet/java-interop@bde306d5: [Java.Interop.Tools.JavaCallableWrappers] JavaTypeScanner.GetJavaTypes (dotnet/java-interop#1076)
  * dotnet/java-interop@f03088e7: [Java.Interop.Tools.JavaCallableWrappers] IMetadataResolver redux (dotnet/java-interop#1075)
  * dotnet/java-interop@e11d0242: [lgtm] Fix LGTM-reported issues. (dotnet/java-interop#1074)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant