Only run check-binary-size workflow for PRs against master

[Marcono1234]

The check-binary-size workflow uses the pull_request_target event, which is inherently dangerous, see for example this GitHub Security Lab post.

This pull request tries to make some precautionary changes to improve the safety of this workflow. There are to my knowledge no issues with the workflow which are directly exploitable.

Changes:

• Only run the workflow for pull requests targeting master
  Currently the workflow uses branches: [] which from my testing seems to act like "run for all branches", see also recent workflow runs at https://github.com/rust-lang/backtrace-rs/actions/workflows/check-binary-size.yml.
  The problem with this is that pull_request_target uses the workflow file from the target branch of the pull request (not the default branch). If in the future a vulnerability in the check-binary-size workflow is noticed and fixed, this reduces the likelihood that there is a stale / feature branch which still contains the vulnerable workflow and that a malicious user can create a PR against that branch to still exploit the vulnerable workflow.
• Specify permissions: {} at workflow level
  This disables all permissions for the workflow by default, and requires the individual jobs to explicitly define the permissions they need (see also GitHub documentation for how permissions are calculated). This should prevent that in the future an additional job is added to the workflow and the author forgets to specify permissions for it, causing implicit 'write' (or 'read', depending on repository settings) permissions to be inadvertently granted to the job.

[workingjubilee]

@Marcono1234 What motivated you to notice this and open this PR? I am aware of the security concerns and I am not sure I trust your modifications per se because your PR description contains a lot of text and nothing actually informative or motivating the very specific changes, especially in light of why the previous iteration was incorrect.

[workingjubilee]

@Marcono1234 Please take another pass at the PR description and remove all text that does not pertain to the actual threats presented by the current repository state. Reasoning about the risk of introducing vulnerabilities is hard enough without having a bunch of extraneous detail.

1. In particular, the only version of the workflow file that matters for this workflow is the one on the master branch, rather than a version in the PR, which makes it hard to iterate on and also limits precisely the damage of PRs that you are thinking of.
2. I am aware of the concern about introducing vulnerabilities in the future, be more specific if you think there is something that is likely to happen.
3. The secrets for this repo don't have a ton of permissions to begin with, last I checked, so this repo being subverted should have a relatively limited impact.
4. We don't use the cache for this repo, so please don't discuss the cache.
5. We only have one branch actually on this repo, because work is done via forks, so please leave out the discussion of stale branches.

I try to take security concerns seriously but I need the reports to be accurate and precise in order to be able to understand them and respond appropriately.

[workingjubilee]

What is the motivation for the change to this filter? This sure looks like adding a value to an empty array, which seems to increase the number of possibilities. I'm not entirely sure if I understand what this did anymore and the GitHub Actions docs don't provide enough info for me to understand how Actions interprets an empty array here either, so I am not convinced this change is good.

[workingjubilee]

Ah, the answer is that the filter that matches nothing instead matches everything. Great.

[Marcono1234]

@Marcono1234 You can tell GitHub that if the empty array doesn't mean "empty" then they have a bug.

I don't have any direct connection to GitHub. I can only tell them that the same way you can, through https://github.com/orgs/community/discussions I assume. There is actually an existing discussion about it: https://github.com/orgs/community/discussions/158135 (not sure if other duplicate ones exist as well)

[workingjubilee]

If the jobs specify permissions then do they not strip permissions from the jobs? Because specifying any one permission removes all others.

In either case it seems somewhat irrelevant. It's a false safety if a permission later in the job can be readded, no? Please be more precise in what we are achieving here. What is the permission we are preventing from being misused?

[Marcono1234]

I have extended the PR description to explain this. This requires that every job explicitly needs to list permissions it needs, and prevents that in the future by accident a job without permissions is added which then gets too extensive default permissions.

[workingjubilee]

On second thought, this workflow seems to mostly bug out a lot and attract complaints from people who think I don't know what I am doing with the repo's security and are panickedly informing me that a state which has not arisen yet can hypothetically arise if I act without thinking about the precise thing that I always think about when trying to evaluate workflows.

I think I regretfully must remove it.

[workingjubilee]

ah no, wait, I'm wrong. the other half of the complaints is that I wind up sighing and explaining I can't make something easier to reproduce or iterate on because the change required would make the workflow insecure.

[workingjubilee]

@Marcono1234 You can tell GitHub that if the empty array doesn't mean "empty" then they have a bug.

[Marcono1234]

You are right, sorry. It would have been better to raise the general concerns in a separate issue. I have rewritten the description now to only be about the changes. However, I still kept detailed explanations / justifications so that it is easier to understand the rationale.
For future reference the original PR description is here:

Original PR description (click to expand)

It seems the previous configuration was erroneously running for PRs against any branches, instead of none.

See also the workflow runs here: https://github.com/rust-lang/backtrace-rs/actions/workflows/check-binary-size.yml

Why this is important

The pull_request_target event (which this workflow uses) is inherently dangerous, see also GitHub Security Lab post.

An additional problem with pull_request_target is that the workflow file is chosen based on the target branch of the PR. So if you ever notice that the check-binary-size workflow is vulnerable and fix it, you could risk having a still vulnerable version of it on a stale branch.
Therefore restricting the workflow only to specific branches (in this case master) should prevent that the workflow from being run on stale branches, assuming their stale version at least has the branches: filter.

---

Note that in general what the check-binary-size workflow is doing (= on: pull_request_target and checking out and running untrusted code) is rather dangerous, as mentioned in the above linked GitHub Security Lab post. At least it already tries to restrict the permissions (6a68ad8) but there are still other risks:

• Accidentally introducing vulnerabilities in the workflow or its used actions in the future
• Accidentally accessing workflow secrets in the future, without noticing that untrusted user PRs might be able to access them
• Cache poisoning; even without write permissions it seems cache poisoning is possible, see also https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/
  Possibly not an issue now that the cache can be poisoned because it seems the other workflows are not using (respectively restoring) the cache, but could easily become an issue.

The best solution would be to not use pull_request_target, but I am not sure if that is possible here.

Maybe at least restrictions could be applied for example to only run when a PR has a specific label, but this might also not be completely safe.
Maybe https://github.blog/security/application-security/how-to-secure-your-github-actions-workflows-with-codeql/ is useful for you as well.

---

the only version of the workflow file that matters for this workflow is the one on the master branch, rather than a version in the PR

You are right that it won't use the version from the PR. But on the other hand it won't only use the version from the master (or more generally default) branch, instead it uses the version from the target branch of the PR, assuming this repo here might have more branches in the future.

We don't use the cache for this repo, so please don't discuss the cache

Ok, I just wanted to bring it up that currently it is probably possible to poison the cache of backtrace-rs through the check-binary-size workflow. And I am not sure how easily it could happen in the future that inadvertently a new workflow is added or one of the other existing ones is changed to using caching. Which might then restore that poisoned cache.

so please leave out the discussion of stale branches

Right, this repository here currently only has the default branch. Though I am not sure if this is a requirement or enforced for rust-lang, or just happens to be this way currently for backtrace-rs. For example https://github.com/rust-lang/miri does have stale branches.

---

and attract complaints from people who think I don't know what I am doing with the repo's security and are panickedly informing me that a state which has not arisen yet can hypothetically arise if I act without thinking about the precise thing that I always think about when trying to evaluate workflows

I didn't say that. The changes in this PR here try to introduce precautionary changes to make it less likely that something goes wrong in the future. I hoped they might be useful. If you don't think they are necessary, then fine.

And even if you were not aware of some aspects of this, that wouldn't mean that you not considering security and are not mindful of GitHub workflow modifications.

• GitHub's documentation might be in parts incomplete or not exactly mention the consequences of using pull_request_target, and if / how it can be used safely
• Some of the attack surfaces (such as cache poisoning) only got more attention recently
• Everybody gets it wrong, Microsoft, Azure, GitHub, ...
  (possibly indicating that there is a general issue with some aspects of GitHub workflows)

[workingjubilee]

Probably the case is that after deserializing from YAML, or during, they assume absence of a field is the same as the field having the empty array. Then they assign the default behavior for it, as if the field was unset.

[workingjubilee]

My apologies for my grumpiness yesterday. I certainly agree that this job is a risk.

I just truly have had it with this job causing more headaches for me and other contributors while being essentially purely advisory, which is further a headache because most of our repos do not have purely-advisory jobs.

I hope you can understand my desire to bury the hatchet
...in this workflow.

Removed at #710