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

fix(query): Extend image_pull_policy_of_container_is_not_always k8s rule to cover additional resource kinds #4891

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

Churro
Copy link
Contributor

@Churro Churro commented Feb 27, 2022

The rule intends to address the problem that image tags are mutable and it is not guaranteed that the version of an image that is cached on a node equals the one with the same tag in a registry. This potential security issue can be prevented by any of the following conditions:

  • Explicitly setting imagePullPolicy to "Always" to ensure the kubelet downloads the image anew every time it launches the container
  • By not specifying imagePullPolicy but by providing an image in one the following ways:
    • With tag "latest"
    • By not specifying a tag at all. From the docs:

      If you don't specify a tag, Kubernetes assumes you mean the tag latest

    • By specifying a digest with the image

Except for the last sub-item, Kubernetes will apply imagePullPolicy: Always. When an image is provided together with a digest, caching by a node is legit and does not trigger the potential security risk this rule strives to detect. Also, this is analogous to what is done by the kubelet with policy "Always" (see here. In other words, since the digest associated with an image is immutable, it unambiguously refers to a specific image locally and/or in the registry.

Problem

  • The rule only covers "Pod" and works with the assumption that imagePullPolicy is always defined. Otherwise, the rule aborts early as a statement evalutes to false. This is an issue as the default pull policy is IfNotPresent and not Always
  • The rule does not cover the case that no tag is used or an image is specified with a digest

Proposed Changes

  • Extend the rule to cover additional resource kinds, e.g., Deployment, DaemonSet, etc.
  • Cover the possibilites described above
  • New positive test cases:
    • imagePullPolicy != "Always" in a Deployment
    • imagePullPolicy not specified but a tag

I submit this contribution under the Apache-2.0 license.

@kicsbot
Copy link
Contributor

kicsbot commented Feb 27, 2022

Scan submitted to Checkmarx

@rafaela-soares
Copy link
Contributor

rafaela-soares commented Mar 9, 2022

Hi, @Churro! Thank you so much for your impressive work and contributions to KICS!

I think we can cover the cases where the tag is set to the latest or undefined or the image has digest but the imagePullPolicy is defined and not set to "Always". What do you think?

For example:

apiVersion: v1
kind: Pod
metadata:
  name: private-image-test-always
spec:
  containers:
    - name: uses-private-image
      image: gcr.io/google-samples/cassandra@sha256:123456
      imagePullPolicy: Never

@Churro
Copy link
Contributor Author

Churro commented Mar 9, 2022

Hi @rafaela-soares,

Probably my explanation in the first post was a bit misleading:
In case a digest is specified together with the image, specifying imagePullPolicy: Always may be set to IfNotPresent or overall omitted. This is fine since the digest unambiguously identifies an image and unlike a tag, it is not "mutable".

The likelihood that a locally cached image with digest X, computed from its content, is the same as the one at a remote registry with digest X, computed from its content, is warranted by modern hash functions that are resistant to preimage and collision attacks.

In other words, there is no match for the sample you posted but that is on purpose because the digest refers to an unambiguous image and the situation this rule intends to detect cannot occur.

If you would specify Always instead of IfNotPresent but an image without digest, the kubelet would compute the hash digest of a locally cached image, compare it with the digest of an image at a remote location and in case of digest mismatch, re-pull the image. Hence, it would use the digest (as the tag could be overwritten) to check if the image contents are the same.

@rafaela-soares
Copy link
Contributor

Hi @Churro!

I understood now! Thank you so much for the kind explanation!

Copy link
Contributor

@rafaela-soares rafaela-soares left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thank you so much for another great contribution!

@cx-joao-reigota cx-joao-reigota merged commit ed7879f into Checkmarx:master Mar 14, 2022
@rafaela-soares rafaela-soares added the community Community contribution label Mar 16, 2022
@rafaela-soares rafaela-soares added the query New query feature label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution query New query feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants