Skip to content

Conversation

githubnemo
Copy link
Collaborator

Zizmor detected a potential cache poisoning attack via setup-docker-buildx. There is an argument to this (an attacker with a valid github token could modify the cache, change the buildx binary and tamper with Docker build releases) but there is also an argument against it: the buildx cache would prevent general information leaks when a new buildx release is tampered with. Since there is no obvious benefit from either side, we ignore this hint and deem it uncritical.

We also change the trigger of zizmor runs to pushes on main, regardless of whether workflow files are changed or not to catch new audits from more recent zizmor versions.

Zizmor detected a potential cache poisoning attack via `setup-docker-buildx`.
There is an argument to this (an attacker with a valid github token could
modify the cache, change the buildx binary and tamper with Docker build releases)
but there is also an argument against it: the buildx cache would prevent
general information leaks when a new buildx release is tampered with. Since
there is no obvious benefit from either side, we ignore this hint and deem it
uncritical.

We also change the trigger of zizmor runs to pushes on main, regardless of
whether workflow files are changed or not to catch new audits from more
recent zizmor versions.
@githubnemo githubnemo marked this pull request as ready for review January 16, 2025 12:01
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@BenjaminBossan
Copy link
Member

I want to understand the implications a bit better here before continuing. The zizmor errors are a bit obscure to me. Are they suggesting that we shouldn't be using any cache for docker at all? Or what would be the alternative to fix the error instead of ignoring it?

@githubnemo
Copy link
Collaborator Author

Are they suggesting that we shouldn't be using any cache for docker at all? Or what would be the alternative to fix the error instead of ignoring it?

They are arguing that you shouldn't use any caching when building release artifacts to avoid tempered releases due to cache poisoning. You can read about their take on it here.

The fix would look like this:

diff --git a/.github/workflows/build_docker_images.yml b/.github/workflows/build_docker_images.yml
index 5b93f80..3a01fea 100644
--- a/.github/workflows/build_docker_images.yml
+++ b/.github/workflows/build_docker_images.yml
@@ -21,6 +21,8 @@ jobs:
     steps:
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v1
+        with:
+          cache-binary: false
       - name: Check out code
         uses: actions/checkout@v3
         with:
@@ -54,6 +56,8 @@ jobs:
     steps:
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v1
+        with:
+          cache-binary: false
       - name: Check out code
         uses: actions/checkout@v3
         with:
@@ -87,6 +91,8 @@ jobs:
     steps:
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v1
+        with:
+          cache-binary: false
       - name: Check out code
         uses: actions/checkout@v3
         with:
@@ -120,6 +126,8 @@ jobs:
     steps:
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v1
+        with:
+          cache-binary: false
       - name: Check out code
         uses: actions/checkout@v3
         with:
@@ -153,6 +161,8 @@ jobs:
     steps:
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@v1
+        with:
+          cache-binary: false
       - name: Check out code
         uses: actions/checkout@v3
         with:

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so the consequence would be that the docker build would be severely slowed down. The images are not intended for public use, so the damage would be contained, although of course attackers could pull off some shenanigans if they had access to our GPU workers.

Of course, if an attacker had access to the GitHub token of PEFT, they could do quite some damage (no releases though), so one could argue that cache poisoning would not be our biggest worry in that case. It is also not still unclear to me how the cache poisoning would be performed concretely, would the steps be auditable?

Anyway, I trust your judgment here as I'm not so knowledgeable in this area.

@githubnemo
Copy link
Collaborator Author

githubnemo commented Jan 16, 2025

Okay, so the consequence would be that the docker build would be severely slowed down.

Not severely, the cache is only for the buildx docker plugin binary. The docker/build-push-action does not perform caching by default.

@githubnemo githubnemo merged commit 9481d2e into huggingface:main Jan 16, 2025
14 of 15 checks passed
Guy-Bilitski pushed a commit to Guy-Bilitski/peft that referenced this pull request May 13, 2025
Zizmor detected a potential cache poisoning attack via `setup-docker-buildx`.
There is an argument to this (an attacker with a valid github token could
modify the cache, change the buildx binary and tamper with Docker build releases)
but there is also an argument against it: the buildx cache would prevent
general information leaks when a new buildx release is tampered with. Since
there is no obvious benefit from either side, we ignore this hint and deem it
uncritical.

We also change the trigger of zizmor runs to pushes on main, regardless of
whether workflow files are changed or not to catch new audits from more
recent zizmor versions.
cyyever pushed a commit to cyyever/peft that referenced this pull request Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants