[lgtm] Fix LGTM-reported issues. (dotnet#1074)

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

Author: jonpryor

build-tools/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/DownloadUri.cs

@@ -41,7 +41,10 @@ public override bool Execute ()
 			}
 
 			var tasks   = new TTask [SourceUris.Length];
-			using (var client = new HttpClient ()) {
+			var handler = new HttpClientHandler {
+				CheckCertificateRevocationList  = true,
+			};
+			using (var client = new HttpClient (handler)) {
 				client.Timeout = TimeSpan.FromHours (3);
 				for (int i = 0; i < SourceUris.Length; ++i) {
 					tasks [i] = DownloadFile (client, SourceUris [i], DestinationFiles [i].ItemSpec);

src/java-interop/java-interop-util.cc

@@ -7,12 +7,21 @@ char*
 utf16_to_utf8 (const wchar_t *widestr)
 {
 	int required_size = WideCharToMultiByte (CP_UTF8, 0, widestr, -1, NULL, 0, NULL, NULL);
+	if (required_size <= 0) {
+		return nullptr;
+	}
+
 	char *mbstr = static_cast<char*> (calloc (required_size, sizeof (char)));
-	int converted_size = WideCharToMultiByte (CP_UTF8, 0, widestr, -1, mbstr, required_size, NULL, NULL);
+	if (mbstr == nullptr) {
+		return nullptr;
+	}
 
-	// Hush a compiler warning about unused variable in RELEASE
-	(void)converted_size;
+	int converted_size = WideCharToMultiByte (CP_UTF8, 0, widestr, -1, mbstr, required_size, NULL, NULL);
 	assert (converted_size == required_size);
+	if (required_size != converted_size) {
+		free (mbstr);
+		return nullptr;
+	}
 
 	return mbstr;
 }
@@ -21,12 +30,21 @@ wchar_t*
 utf8_to_utf16 (const char *mbstr)
 {
 	int required_chars = MultiByteToWideChar (CP_UTF8, 0, mbstr, -1, NULL, 0);
+	if (required_chars <= 0) {
+		return nullptr;
+	}
+
 	wchar_t *widestr = static_cast<wchar_t*> (calloc (required_chars, sizeof (wchar_t)));
-	int converted_chars = MultiByteToWideChar (CP_UTF8, 0, mbstr, -1, widestr, required_chars);
+	if (widestr == nullptr) {
+		return nullptr;
+	}
 
-	// Hush a compiler warning about unused variable in RELEASE
-	(void)converted_chars;
+	int converted_chars = MultiByteToWideChar (CP_UTF8, 0, mbstr, -1, widestr, required_chars);
 	assert (converted_chars == required_chars);
+	if (required_chars != converted_chars) {
+		free (widestr);
+		return nullptr;
+	}
 
 	return widestr;
 }

tools/java-source-utils/src/main/java/com/microsoft/android/JavaSourceUtilsOptions.java

@@ -167,7 +167,7 @@ private final JavaSourceUtilsOptions parse(Iterator<String> args) throws IOExcep
 					final   String          bootClassPath   = getNextOptionValue(args, arg);
 					final   ArrayList<File> files           = new ArrayList<File>();
 					for (final String cp : bootClassPath.split(File.pathSeparator)) {
-						final   File    file    = new File(cp);
+						final   File    file    = new File(cp); // lgtm [java/path-injection-local]
 						if (!file.exists()) {
 							System.err.println(App.APP_NAME + ": warning: invalid file path for option `-bootclasspath`: " + cp);
 							continue;
@@ -253,7 +253,7 @@ private final JavaSourceUtilsOptions parse(Iterator<String> args) throws IOExcep
 					if (arg.startsWith("@")) {
 						// response file?
 						final   String  responseFileName = arg.substring(1);
-						final   File    responseFile     = new File(responseFileName);
+						final   File    responseFile     = new File(responseFileName);  // lgtm [java/path-injection-local]
 						if (responseFile.exists()) {
 							final   Iterator<String>        lines   =
 								Files.readAllLines(responseFile.toPath())
@@ -267,7 +267,7 @@ private final JavaSourceUtilsOptions parse(Iterator<String> args) throws IOExcep
 							break;
 						}
 					}
-					final   File    file        = new File(arg);
+					final   File    file        = new File(arg);    // lgtm [java/path-injection-local]
 					if (!file.exists()) {
 						System.err.println(App.APP_NAME + ": warning: invalid file path for option `FILES`: " + arg);
 						break;
@@ -319,6 +319,10 @@ private static void extractTo(final File zipFilePath, final File toDir, final Co
 				if (!entry.getName().endsWith(".java"))
 					continue;
 				final   File        target      = new File(toDir, entry.getName());
+				if (!target.toPath().normalize().startsWith(toDir.toPath())) {
+					System.err.println(App.APP_NAME + ": warning: skipping bad zip entry: " + zipFilePath + "!" + entry.getName());
+					continue;
+				}
 				if (verboseOutput) {
 					System.out.println ("# creating file: " + target.getAbsolutePath());
 				}
@@ -343,7 +347,7 @@ static File getNextOptionFile(final Iterator<String> args, final String option)
 			throw new IllegalArgumentException(
 					"Expected required value for option `" + option + "`.");
 		final   String  fileName    = args.next();
-		final   File    file        = new File(fileName);
+		final   File    file        = new File(fileName);   // lgtm [java/path-injection-local]
 		if (!file.exists()) {
 			System.err.println(App.APP_NAME + ": warning: invalid file path for option `" + option + "`: " + fileName);
 			return null;

tools/java-source-utils/src/main/java/com/microsoft/android/JavadocXmlGenerator.java

@@ -39,7 +39,7 @@ public JavadocXmlGenerator(final String output) throws FileNotFoundException, Pa
 		if (output == null)
 			this.output = System.out;
 		else {
-			final File file     = new File(output);
+			final File file     = new File(output);     // lgtm [java/path-injection-local]
 			final File parent   = file.getParentFile();
 			if (parent != null) {
 				parent.mkdirs();