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

encryption.CheckAuthorization not working for multi-arch images #69

Closed
dimitar-dimitrow opened this issue Mar 15, 2022 · 10 comments
Closed

Comments

@dimitar-dimitrow
Copy link

When a multi-arch index descriptor is provided to the imgcrypt's CheckAuthorization func (e.g. via image.Target()), the library iterates over the manifests it refers to with the cryptoOpUnwrapOnly option set to true to perform a check only. That causes the cycle to stop on the first manifest in the collection as the condition here will always be evaluated to true error-regardless. Additionally, if reading any of the referred manifest's children returns an errdefs.IsNotFound(err), the cycle will exit with a nil error, thus, the authorization check passes incorrectly.
Let's take for example the case where the cycle checks the first manifest in the collection (e.g. for amd64) on an arm/arm64 machine, the children of this manifest are not found since this is not the target platform and they are not pulled -> the authorization check passes incorrectly. This issue is rarely reproducible on an amd64 machine as usually, this is the first manifest in the index descriptor.

@stefanberger
Copy link
Contributor

Do you have examples of command lines that you used and ran into this issue?

@dimitar-dimitrow
Copy link
Author

dimitar-dimitrow commented Mar 17, 2022

# ctr-enc -n issue i pull docker.io/library/bash:latest
# ctr-enc -n issue i encrypt --recipient jwe:/home/pi/develop/mypubkey.pem docker.io/library/bash:latest
# ctr-enc -n issue run -t --key /home/pi/develop/mykey.pem docker.io/library/bash:latest with_key
bash-5.1# exit
exit
# ctr-enc -n issue run -t docker.io/library/bash:latest without_key
bash-5.1# exit
exit

Result: The without_key container runs without providing the private key.
Expected: Authorization check fails for without_key, the container does not run.

@stefanberger
Copy link
Contributor

And you are trying this on something other than amd64?

@dimitar-dimitrow
Copy link
Author

Yes, on raspberry(armv6l). This issue is rarely reproducible on an amd64 machine as usually, this is the first manifest in the index descriptor. In the case of docker.io/library/bash:latest the first manifest is indeed the amd64 one, so you won't be able to reproduce it on amd64 machine.

@stefanberger
Copy link
Contributor

I am running this now on a ppc64 machine. There's a test case in script/tests/test_encryption.sh covering exactly this case:

MSG=$(sudo bash -c "$CTR run \
--rm \
${ALPINE_ENC} testcontainer1 echo 'Hello world'" 2>&1)
if [ $? -eq 0 ]; then
MSG=$($CTR snapshot rm testcontainer1 2>&1)
failExit 1 "Should not have been able to run a container from encrypted image without passing keys"
fi
MSG=$($CTR snapshot rm testcontainer1 2>&1)
MSG=$(sudo bash -c "$CTR run \
--gpg-homedir ${GPGHOMEDIR} \
--gpg-version 2 \
--key <(echo "${GPGTESTKEY1}" | base64 -d) \
--rm \
${ALPINE_ENC} testcontainer1 echo 'Hello world'" 2>&1)
failExit $? "Should have been able to run a container from encrypted image when passing keys\n${MSG}"

Unfortunately it's passing as expected, meaning it refuses to run the encrypted container image without key and runs it when the key is provided.

@stefanberger
Copy link
Contributor

So the problem with the test case is that for this image --all-platforms were pulled:

$CTR images pull ${IMAGE_PULL_CREDS:+--user ${IMAGE_PULL_CREDS}} --all-platforms ${ALPINE} &>/dev/null

If one doesn't pull --all-platforms then this leads to the problem you are seeing.

stefanberger added a commit that referenced this issue Mar 17, 2022
Create a reproducing test case for issue #69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
To be able to properly perform an authorization check on an image we need
to know the platform to perform check when in cryptManifestList(). Extend
the logic for cryptoOp == cryptoOpUnwrapOnly to skip over manifests that
do not correspond to the local platform and return an error if no manifest
was found that matches the local platform.

Resolves: containerd#69
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 17, 2022
To be able to properly perform an authorization check on an image we need
to know the platform to perform check when in cryptManifestList(). Extend
the logic for cryptoOp == cryptoOpUnwrapOnly to skip over manifests that
do not correspond to the local platform and return an error if no manifest
was found that matches the local platform.

Resolves: containerd#69
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
@stefanberger
Copy link
Contributor

I now have a pending PR. @dimitar-dimitrow , maybe you can give it a try.

https://github.com/stefanberger/imgcrypt/tree/fix_issue_69

stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 18, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 18, 2022
To be able to properly perform an authorization check on an image we need
to know the platform to perform check when in cryptManifestList(). Extend
the logic for cryptoOp == cryptoOpUnwrapOnly to skip over manifests that
do not correspond to the local platform and return an error if no manifest
was found that matches the local platform.

Resolves: containerd#69
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 18, 2022
To be able to properly perform an authorization check on an image we need
to know the platform to perform check when in cryptManifestList(). Extend
the logic for cryptoOp == cryptoOpUnwrapOnly to skip over manifests that
do not correspond to the local platform and return an error if no manifest
was found that matches the local platform.

The following projects seem NOT to be affect due to the change in the code
path of CheckAuthorization() since they are not using it:

- cri-o
- nerdctl
- skopeo
- buildah

The impact on imgcrypt via ctr-enc is not so clear either since
CheckAuthorization() is not called on the server side but by the ctr-enc
client, thus can be modified easily.

Resolves: containerd#69
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 18, 2022
Create a reproducing test case for issue containerd#69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
stefanberger added a commit to stefanberger/imgcrypt that referenced this issue Mar 18, 2022
To be able to properly perform an authorization check on an image we need
to know the platform to perform check when in cryptManifestList(). Extend
the logic for cryptoOp == cryptoOpUnwrapOnly to skip over manifests that
do not correspond to the local platform and return an error if no manifest
was found that matches the local platform.

The following projects seem NOT to be affect due to the change in the code
path of CheckAuthorization() since they are not using it:

- cri-o
- nerdctl
- skopeo
- buildah
- podman

The impact on imgcrypt via ctr-enc is not so clear either since
CheckAuthorization() is not called on the server side but by the ctr-enc
client, thus can be modified easily.

Resolves: containerd#69
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
lumjjb pushed a commit that referenced this issue Mar 21, 2022
Create a reproducing test case for issue #69 by adding a test case
with a bash image that is only pulled for the local platform, so
without --all-platforms. The test case will likey work on amd64 but
does fail locally on a ppc64 host.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
@lumjjb lumjjb closed this as completed in 6fdd981 Mar 21, 2022
@stefanberger
Copy link
Contributor

@dimitar-dimitrow Even though the code has been merged already, can you give it a try?

@dimitar-dimitrow
Copy link
Author

@stefanberger Sorry, for the late response, the fix works as expected. Thanks!
Would there be a new version release with this fix soon or should I keep to the snapshot version for now?

@stefanberger
Copy link
Contributor

@dimitar-dimitrow I am going to create v1.1.4 once a few more things are merged. I will create a CVE referencing your report. Thanks.

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

No branches or pull requests

2 participants