library_mono.js triggers apparent false-positive report from GitHub's codeql scan

[SteveSandersonMS]

This was originally reported at dotnet/aspnetcore#36529

It seems that if you add GitHub's CodeQL analysis (like this), then you will get a report of a vulnerability like this: https://github.com/damienbod/AspNetCore6Experiments/pull/7/checks?check_run_id=3607015850. Just in case that gets deleted, here's a screenshot:

Notice that these are all from dotnet.js (which gets copied and renamed during the build but its contents aren't changed).

On investigation, this seems to map to CodeQL's rule for CWE-134. It gives an example of "bad" code in which user-supplied data becomes part of a format string:

  let user = req.query.user;
  let ip = req.connection.remoteAddress;
  console.log("Unauthorized access attempt by " + user, ip);

I agree that this code is potentially buggy, since if user contained %s, then the ip value would be inserted into the middle of the console output instead of being appended to the end of it. Which would be strange.

I think the closest match to that pattern is here:

runtime/src/mono/wasm/runtime/library_mono.js

Line 1302 in 55622ab

console.log ("Attempting to fetch '" + attemptUrl + "' for", asset.name);

There might be others, but this is the closest one I can find. It's possible that codeql is going to raise an alert for any use of console.log that involves string concatenation instead of %s-style tokens.

Of course, it doesn't seem like this would actually be a vulnerability in a Blazor application, because:

1. Blazor never runs JavaScript code on the server, and I don't really think user-supplied input could get into this line of code anyway as the list of assets to load (and their URLs) is determined by compile-time code only.
2. Even if user-supplied input could get here, the fallout seems to be limited to potential "garbled output" in the console, which end users aren't looking at anyway

But it would be good to avoid confusing people and avoid this particular code pattern if it makes CodeQL stop raising an alert. It could probably be fixed by using the format string pattern they cite in the "good" example:

console.log("Unauthorized access attempt by %s", user, ip);

In my opinion this looks like a false alert and I wouldn't suggest it's urgent to work around, but cc @blowdart for info.

[ghost]

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This was originally reported at dotnet/aspnetcore#36529

It seems that if you add GitHub's CodeQL analysis (like this), then you will get a report of a vulnerability like this: https://github.com/damienbod/AspNetCore6Experiments/pull/7/checks?check_run_id=3607015850. Just in case that gets deleted, here's a screenshot:

Notice that these are all from dotnet.js (which gets copied and renamed during the build but its contents aren't changed).

On investigation, this seems to map to CodeQL's rule for CWE-134. It gives an example of "bad" code in which user-supplied data becomes part of a format string:

  let user = req.query.user;
  let ip = req.connection.remoteAddress;
  console.log("Unauthorized access attempt by " + user, ip);

I think the closest match to that pattern is here:

runtime/src/mono/wasm/runtime/library_mono.js

Line 1302 in 55622ab

console.log ("Attempting to fetch '" + attemptUrl + "' for", asset.name);

There might be others, but this is the closest one I can find.

Of course, it doesn't seem like this would actually be a vulnerability in a Blazor application, because Blazor never runs JavaScript code on the server, and I don't really think user-supplied input could get into this line of code anyway as the list of assets to load (and their URLs) is determined by compile-time code only. But it would be good to avoid confusing people and avoid this particular code pattern if it makes CodeQL stop raising an alert. It could probably be fixed by using the format string pattern they cite in the "good" example:

console.log("Unauthorized access attempt by %s", user, ip);

In my opinion this looks like a false alert and I wouldn't suggest it's urgent to work around, but cc @blowdart for info.

Author:	SteveSandersonMS
Assignees:	-
Labels:	arch-wasm
Milestone:	-

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[blowdart]

Not ship blocking, but should be addressed before release

[ghost]

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This was originally reported at dotnet/aspnetcore#36529

It seems that if you add GitHub's CodeQL analysis (like this), then you will get a report of a vulnerability like this: https://github.com/damienbod/AspNetCore6Experiments/pull/7/checks?check_run_id=3607015850. Just in case that gets deleted, here's a screenshot:

Notice that these are all from dotnet.js (which gets copied and renamed during the build but its contents aren't changed).

On investigation, this seems to map to CodeQL's rule for CWE-134. It gives an example of "bad" code in which user-supplied data becomes part of a format string:

  let user = req.query.user;
  let ip = req.connection.remoteAddress;
  console.log("Unauthorized access attempt by " + user, ip);

I agree that this code is potentially buggy, since if user contained %s, then the ip value would be inserted into the middle of the console output instead of being appended to the end of it. Which would be strange.

I think the closest match to that pattern is here:

runtime/src/mono/wasm/runtime/library_mono.js

Line 1302 in 55622ab

console.log ("Attempting to fetch '" + attemptUrl + "' for", asset.name);

There might be others, but this is the closest one I can find. It's possible that codeql is going to raise an alert for any use of console.log that involves string concatenation instead of %s-style tokens.

Of course, it doesn't seem like this would actually be a vulnerability in a Blazor application, because:

1. Blazor never runs JavaScript code on the server, and I don't really think user-supplied input could get into this line of code anyway as the list of assets to load (and their URLs) is determined by compile-time code only.
2. Even if user-supplied input could get here, the fallout seems to be limited to potential "garbled output" in the console, which end users aren't looking at anyway

But it would be good to avoid confusing people and avoid this particular code pattern if it makes CodeQL stop raising an alert. It could probably be fixed by using the format string pattern they cite in the "good" example:

console.log("Unauthorized access attempt by %s", user, ip);

In my opinion this looks like a false alert and I wouldn't suggest it's urgent to work around, but cc @blowdart for info.

Author:	SteveSandersonMS
Assignees:	lewing
Labels:	arch-wasm, area-Infrastructure-mono
Milestone:	6.0.0

[ViktorHofer]

ping @lewing in case this needs to flow into the product.

[lewing]

@SteveSandersonMS I found two cases which appears to match the number of failures per file. I've opened #59839 for net6.

[lewing]

@pavelsavara we should resolve this in main but I can wait till #59655 lands