Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[discussion] Harden repository checkout #626

Open
laurentsimon opened this issue Jul 25, 2022 · 5 comments
Open

[discussion] Harden repository checkout #626

laurentsimon opened this issue Jul 25, 2022 · 5 comments
Labels
area:hardening Issue related to security hardening type:discussion A point of discussion

Comments

@laurentsimon
Copy link
Collaborator

laurentsimon commented Jul 25, 2022

We use the actions/checkout to fetch the repository source code. Internally, the action uses GH APIs to fetch the tarball, but it never validates that it's a valid git tree and / or that it corresponds to the expected hash GITHUB_SHA.

One possible attack here is a TLS interception: we could be served a different source code. That also got me thinking whether we should propose recording the TLS connection parameters used by builders in the SLSA provenance (TLS version, ciphersuite, certificate). It would bloat the provenance output, and is not necessary if sha validation is performed.

Note: I think it's common practice to fetch source code via API or download it from the a release URL in general. So I think hardening the validation is going to be something that needs to be called out in SLSA guidelines somewhere.

Note: I think none of the TLS client libraries (node, golang, etc) have configuration options to verifier the certificate is in the CT log. So it could be difficult to find evidence that something happened.

@laurentsimon laurentsimon added type:feature New feature or request status:triage Issue that has not been triaged labels Jul 25, 2022
@laurentsimon
Copy link
Collaborator Author

fyi @MarkLodato

@ianlewis ianlewis added type:discussion A point of discussion and removed type:feature New feature or request status:triage Issue that has not been triaged labels Jul 29, 2022
@ianlewis ianlewis changed the title [feature] Harden repository checkout [discussion] Harden repository checkout Jul 29, 2022
@laurentsimon
Copy link
Collaborator Author

laurentsimon commented Sep 6, 2022

I tested a different attack. Say, there's a new tag pushed by maintainers. The checkout Action seems to fetch and do git checkout --progress --force refs/tags/<tag> - as per the logs. So one idea is to force push a different tag before the checkout happens, to trick the builder into fetching different code for the same tag. I was not able to make it work. It seems that the tag is "locked", it the old tag is still accessible until the workflow finishes. Still, I'd be more comfortable making the check explicit in our codebase:

if [[ "$(git log -1 --format='%H')" != "$GITHUB_SHA" ]]; then
    echo "bad"
    exit 1
fi

I'll send a PR for this.

@ianlewis
Copy link
Member

ianlewis commented Oct 7, 2022

We use the actions/checkout to fetch the repository source code. Internally, the action uses GH APIs to fetch the tarball, but it never validates that it's a valid git tree and / or that it corresponds to the expected hash GITHUB_SHA.

I think one important thing to point out is that GITHUB_SHA refers to the commit that triggered the workflow and not necessarily the repo being checked out.

@ianlewis
Copy link
Member

ianlewis commented Oct 7, 2022

Also, I think we can improve the checks we have currently that disallow using 'actions/checkout' in our internal actions and extend to workflows as well.

We can have an internal action for checking out code without a repository or ref input that always uses GITHUB_REPOSITORY and GITHUB_SHA (or github.event.pull_request.head.sha for pull_request events) and prevent internal actions and reusable workflows from using anything but that action to check out code. That way we can prevent actions and workflows from checking out the wrong version of the code.

@laurentsimon
Copy link
Collaborator Author

Let's also address:

  • dirty tree
  • tree verification

@ianlewis ianlewis removed this from the 2022 Stability improvements milestone Jan 12, 2023
@ianlewis ianlewis added the area:hardening Issue related to security hardening label Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:hardening Issue related to security hardening type:discussion A point of discussion
Projects
None yet
Development

No branches or pull requests

2 participants