containerd/imgcrypt

encryption.CheckAuthorization not working for multi-arch images

dimitar-dimitrow opened this issue · 10 comments

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.

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

# 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.

And you are trying this on something other than amd64?

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.

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.

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.

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

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

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

@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?

@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.