CWE-117 False positive

[theAntiYeti]

CWE-117: Improper Output Neutralization for Logs
CWE-117 is being reported by CodeQL in the following code:

func makeErrorForHTTPResponse(resp *http.Response) error {
	bodyBytes, err := ioutil.ReadAll(resp.Body)
	if err != nil {
		return err
	}
	url := resp.Request.URL.String()
	safeURL := strings.Replace(url, "\n", "", -1)
	safeURL = strings.Replace(safeURL, "\r", "", -1)
	return fmt.Errorf("%s %s returned HTTP %s; \n\n %#q", resp.Request.Method, safeURL, resp.Status, bodyBytes)
}

Despite this code being near identical to the provided "good" example

func handlerGood(req *http.Request) {
	username := req.URL.Query()["username"][0]
	escapedUsername := strings.Replace(username, "\n", "", -1)
	escapedUsername = strings.Replace(escapedUsername, "\r", "", -1)
	log.Printf("user %s logged in.\n", escapedUsername)
}

here.

Here is a screen shot of the output logs for further clarification

It appears that CodeQL completely ignores the above two functions performing the string replacement.

[hmakholm]

Thanks for the report; I agree this looks weird. If this is happening on a public repository, can you share a link to the repo and the analysis result?

[theAntiYeti]

Hi @hmakholm, I have been trying some things in the meantime but I have now reset to the commit I submitted this false positive under.
The branch.
CodeQL analysis result.

[trdyer]

I have also been seeing this, but in a private repository.

In my case it appears that the lines I have fixed are being added as a "fixed" item, but an equivalent item is being created.

[smowton]

Thanks for the heads-up; fixed in #637

[llhuii]

Can we handle the case below?

nodeName := req.Header.Get("Node-Name")

err := validateNodeName(nodeName)
if err != nil {
           return err
}
log.Printf("%s has been connected", nodeName)

here: https://github.com/llhuii/sedna/security/code-scanning/10?query=ref%3Arefs%2Fheads%2Fmain

[smowton]

looks like it's actually

	rawNodeName := req.Header.Get("Node-Name")

	nodeName, err := validateNodeName(rawNodeName)

So yes that ought to be noticed by the sanitizer written in the PR above.

If it was a sanitizer guard as you've written it in your comment we'd need to do a bit more.

[smowton]

Fix committed; will be reflected on code scanning at the next distribution upgrade, which will likely go live mid-January

[llhuii]

If it was a sanitizer guard as you've written it in your comment we'd need to do a bit more.

I would prefer this sanitizer guard rather over reassignment nodeName, err := validateNodeName(rawNodeName).

Reassignment would be weird since it has already been checked.

[smowton]

So replacing and then comparing is a weird sanitizer guard, since you've done the work and may as well just use the replaced string. I'd be happy to make read-only string-searching operations sanitizer guards though.

[olix0r]

We continue to see false-positive alerts for CWE-117

https://github.com/linkerd/linkerd2/blob/9875a0e850b010b6a3dca811c2634a9c5dd97f34/viz/metrics-api/http_server.go#L32-L36

[owen-mc]

@olix0r When I look at the codescanning results for that PR it's saying that 7 alerts were fixed, including the one from your screenshot. I'm not sure if things got sorted out after a delay or if the place where you took the screenshot just didn't make it very clear that it was showing a fixed alert.

[olix0r]

@owen-mc Thanks, it's possible I was confused by the UI--I thought that was indicating that some issues were fixed and others were still active. As I recall, the workflow did not succeed until all of those issues were acknowledged.

In any case, we're no longer impacted by this :)