CodeQL Action fails blocking code review/merging

[serathius]

Within last 19h all CodeQL actions started failing blocking code review/merging.

I'm not familiar with CodeQL, what this action does? Would we be ok with disabling it or should we look into a fix?

I assume that this is organization/project configuration error, to fix it we would need help from maintainer that has access to the Github configuration.
cc @ptabor @spzala @ahrtr @hexfusion

Dashboard: https://github.com/etcd-io/etcd/actions/workflows/codeql-analysis.yml

Error:

Waiting for processing to finish
  Error: Resource not accessible by integration
  RequestError [HttpError]: Resource not accessible by integration
      at /home/runner/work/_actions/github/codeql-action/v1/node_modules/@octokit/request/dist-node/index.js:66:23
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:93:5)
      at async Job.doExecute (/home/runner/work/_actions/github/codeql-action/v1/node_modules/bottleneck/light.js:405:18) {
    name: 'HttpError',
    status: 403,

[serathius]

Looks like 403 is not the only error, there is a failure on report like https://github.com/etcd-io/etcd/pull/13590/checks?check_run_id=4789367815 that reports errors like:

[serathius]

From https://codeql.github.com/codeql-query-help/csharp/cs-log-forging/#recommendation (note that this csharp documentation as there is no article for Go with recommendations on how to handle it) recommendation is to escape user input. Method of escaping depends on output we are generating, for example if we generated HTML document it should use HTML encoder.

In Etcd case user input is passed to zap logging library. In default configuration zap will write a JSON and properly escape values, so there is not immediate security threat. One case would be with new v3.6 feature where zap can be switched from JSON to traditional console logs. In that case characters like \n and \t might not be escaped, however I don't see any impact here too.

[serathius]

Heh, disabling a query is complicated enough that I will look into fixing the errors.

[serathius]

I didn't found easy way to disable just go/log-injection, I think the best approach would be fix cases where zap is not used and Dismiss all others.

Instruction on dismiss https://docs.github.com/en/github-ae@latest/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/managing-code-scanning-alerts-for-your-repository#dismissing-or-deleting-alerts

Dismissing requires Edit permissions on repo. @ptabor can you take a look?

[ptabor]

I see only 2 pretty specific alerts:

etcd/tools/etcd-dump-logs/main.go

Line 235 in 14c527f

fmt.Printf("%4d\t%10d\tnorm\t%s", entry.Term, entry.Index, rr.String())

etcd/tools/etcd-dump-db/backend.go

Line 125 in 14c527f

fmt.Printf("user=%q, roles=%q, password=%q, option=%v\n", user.Name, user.Roles, string(user.Password), user.Options)

And the remaining 44 are not on the list to dismiss.

[serathius]

Those two errors should be addressed by #13591

[serathius]

Looks like issue is still not resolved :(

[serathius]

This time let's try to get some debug info #13598

[spzala]

Looks like issue is still not resolved :(

Yes, specially the changes in backend.go removed the logging per the scanning tool suggest. I have closed/reported that as a false positive. And you also tried taking care of main.go is taken care by not logging anything problematic. So I think we are fine with those two alerts.

About the 403 error Error: Resource not accessible by integration, I thought it could be a permission issue but this recent one seems passed it without the error, #13582

Investigating further.

[adityasharad]

Greetings. I'm on the team at GitHub that maintains the CodeQL Action, and was directed here by our developer relations team, to whom I believe maintainers of this project reached out. Thanks for letting us know, and apologies for the inconvenience.

The timeline you describe (9 days ago) coincides with our update of v1 to v1.0.27, which includes a change to the default behaviour when uploading results, specifically checking on the status of the results while they're being processed.
The error (Resource not accessible by integration in the Waiting for processing to finish step) suggests that the default GITHUB_TOKEN being used in your Actions workflow doesn't have permission to perform that check.

Could you please try one of the following and let me know if they resolve the problem with the failing workflow?

• Add an explicit permissions block to your Actions workflow updating the default permissions for the GITHUB_TOKEN. We recommend the following here:

  permissions:
    # required for all workflows
    security-events: write
    # only required for workflows in private repositories
    actions: read
    contents: read

  OR
• Turn off the default behaviour of waiting for results processing:

  - name: Perform CodeQL Analysis
    uses: github/codeql-action/analyze@v1
    # workaround: add the two lines below
    with:
      wait-for-processing: false

[serathius]

This solves one of the problems with CodeQL. @adityasharad please refrain from introducing a breaking changes without proper notification.

There is also problem of false positives that we are unable to silence. CodeQL reports problems Log entries created from user input on data passed to zap which escapes the data when writing JSON.

@adityasharad Do you have any recommendation on silencing those reports?

[spzala]

@adityasharad thank you so much for helping! A qq - does enabling the write permission pause any known/possible security risk in any case or anything that we need to take care in our settings? (if there is anything, we can discuss security related discussion offline).
Also, as @serathius mentioned there seems false positive. Besides what he mentioned, we also kept seeing an alert where we removed logging password in one case under this PR, https://github.com/etcd-io/etcd/pull/13591/files (backup_command.go file) Thanks again!

[ptabor]

I see the warnings refreshed now in the security console. I will review them and dismiss when justified.

[adityasharad]

Appreciate your patience and willingness to try the workaround. I realise the workaround I suggested may not be enough to handle all situations, in particular workflows running on pull requests from forked repositories, which received a more limited set of permissions by default, even if you specify write in the permissions block. We have now rolled back the change in the CodeQL Action while we figure out a way to check on results without breaking such workflows. So you can continue to use v1 of the Action in your workflow, and should no longer see the error.

To learn more about the options for Actions token permissions, you may be in interested in https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token and https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token. My understanding is that by keeping the Send write tokens to workflows from pull requests setting disabled in your repo settings, pull requests from forks will get read permission at most for any of the configured scopes (even if the permissions block sets it to write). The first column of the table in the docs describes the permissions for a workflow with no explicit permissions block: most scopes are set to read/write by default, so adding a permissions block to the workflow restricts the scopes further than the default to exactly the scopes you need.

There is also problem of false positives that we are unable to silence. CodeQL reports problems Log entries created from user input on data passed to zap which escapes the data when writing JSON.

Thanks for pointing these out too. Dismissing the alerts in the code scanning UI will ensure they don't reappear. However the CodeQL team is interested in hearing about false positives and library coverage that affect our security-focused analysis. In this case it sounds like we could handle zap APIs as sanitisers for logging-related vulnerabilities.
Could you please create an issue in https://github.com/github/codeql or https://github.com/github/codeql-go, so that the team that works on Go analysis can consider it?

Going forward, please don't hesitate to file questions or issues in https://github.com/github/codeql and we will be very happy to help you out.

[serathius]

Looks like issue has been resolved.