Privilege Escalation / Authorization bypass in reconciliation logic due to ownership checks

Prerequisites: - A Kubernetes cluster with RBAC allowing creation of Jobs in a namespace where a CronJob exists. - A CronJob named 'priv-esc-cron' in namespace 'default'. - A user/serviceaccount with permission to create Jobs in that namespace. Before the fix (v1.36.0-beta.0 and earlier): a malicious or misconfigured Job with an OwnerReference pointing to the CronJob by name could be included in the CronJob's reconciliation set even if the UID did not match the CronJob's UID. This could allow an attacker to influence the CronJob reconciliation behavior (e.g., cause deletion or modification of a non-owned Job) because the code validated ownership by CronJob name only, not by UID. Proof-of-concept (conceptual, for demonstration only): 1) Create a CronJob named poc-cron with UID A in namespace default. 2) Create a Job named poc-job-not-owned in the same namespace with an OwnerReference referencing CronJob poc-cron by name but with a mismatched UID (UID: B). - Example Job manifest (OwnerReference UID shown as A for real ownership; for demonstration assume you set a mismatched UID B): apiVersion: batch/v1 kind: Job metadata: name: poc-job-not-owned namespace: default ownerReferences: - apiVersion: batch/v1 kind: CronJob name: poc-cron uid: "B" # intentionally mismatched UID spec: template: spec: containers: - name: busybox image: busybox command: ["sh", "-c", "sleep 3600"] restartPolicy: Never 3) Observe in the cluster: the old CronJob controller logic (before the fix) would include this job in the reconciliation loop because the OwnerReference.Name matches the CronJob name, potentially leading to unintended actions on a non-owned Job (e.g., deletions during cleanup). 4) After applying the fix (the commit), the CronJob controller uses a UID-based index (jobControllerUIDIndex) and only reconciles Jobs whose OwnerReference UID matches the CronJob UID. The poc-job-not-owned would fail to be indexed under the CronJob UID and would not be reconciled, preventing the unintended interaction. Note: This PoC is illustrative; exact behavior depends on the cluster RBAC, admission controllers, and the specifics of OwnerReference validation in a given Kubernetes version.

diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index 89ebd0b098998..12670cae57b93 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -29,7 +29,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -51,6 +50,10 @@ import ( "k8s.io/utils/ptr" ) +const ( + jobControllerUIDIndex = "jobControllerUID" +) + var ( // controllerKind contains the schema.GroupVersionKind for this controller type. controllerKind = batchv1.SchemeGroupVersion.WithKind("CronJob") @@ -70,9 +73,11 @@ type ControllerV2 struct { jobControl jobControlInterface cronJobControl cjControlInterface - jobLister batchv1listers.JobLister cronJobLister batchv1listers.CronJobLister + // jobIndexer allows looking up jobs by ControllerRef UID + jobIndexer cache.Indexer + jobListerSynced cache.InformerSynced cronJobListerSynced cache.InformerSynced @@ -99,12 +104,14 @@ func NewControllerV2(ctx context.Context, jobInformer batchv1informers.JobInform jobControl: realJobControl{KubeClient: kubeClient}, cronJobControl: &realCJControl{KubeClient: kubeClient}, - jobLister: jobInformer.Lister(), cronJobLister: cronJobsInformer.Lister(), + jobIndexer: jobInformer.Informer().GetIndexer(), + jobListerSynced: jobInformer.Informer().HasSynced, cronJobListerSynced: cronJobsInformer.Informer().HasSynced, - now: time.Now, + + now: time.Now, } jobInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -125,6 +132,22 @@ func NewControllerV2(ctx context.Context, jobInformer batchv1informers.JobInform }, }) + err := jobInformer.Informer().AddIndexers(cache.Indexers{ + jobControllerUIDIndex: func(obj interface{}) ([]string, error) { + job, ok := obj.(*batchv1.Job) + if !ok { + return nil, nil + } + if controllerRef := metav1.GetControllerOf(job); controllerRef != nil { + return []string{string(controllerRef.UID)}, nil + } + return nil, nil + }, + }) + if err != nil { + return nil, fmt.Errorf("adding job controller UID indexer: %w", err) + } + metrics.Register() return jm, nil @@ -202,7 +225,7 @@ func (jm *ControllerV2) sync(ctx context.Context, cronJobKey string) (*time.Dura return nil, err } - jobsToBeReconciled, err := jm.getJobsToBeReconciled(cronJob) + jobsToBeReconciled, err := jm.getCronJobJobsByIndexer(cronJob) if err != nil { return nil, err } @@ -234,6 +257,23 @@ func (jm *ControllerV2) sync(ctx context.Context, cronJobKey string) (*time.Dura return nil, syncErr } +func (jm *ControllerV2) getCronJobJobsByIndexer(cronJob *batchv1.CronJob) ([]*batchv1.Job, error) { + var jobsForCronJob []*batchv1.Job + jobs, err := jm.jobIndexer.ByIndex(jobControllerUIDIndex, string(cronJob.UID)) + if err != nil { + return nil, err + } + for _, obj := range jobs { + job, ok := obj.(*batchv1.Job) + if !ok { + utilruntime.HandleError(fmt.Errorf("unexpected object type in job indexer: %v", obj)) + continue + } + jobsForCronJob = append(jobsForCronJob, job) + } + return jobsForCronJob, nil +} + // resolveControllerRef returns the controller referenced by a ControllerRef, // or nil if the ControllerRef could not be resolved to a matching controller // of the correct Kind. @@ -255,26 +295,6 @@ func (jm *ControllerV2) resolveControllerRef(namespace string, controllerRef *me return cronJob } -func (jm *ControllerV2) getJobsToBeReconciled(cronJob *batchv1.CronJob) ([]*batchv1.Job, error) { - // list all jobs: there may be jobs with labels that don't match the template anymore, - // but that still have a ControllerRef to the given cronjob - jobList, err := jm.jobLister.Jobs(cronJob.Namespace).List(labels.Everything()) - if err != nil { - return nil, err - } - - jobsToBeReconciled := []*batchv1.Job{} - - for _, job := range jobList { - // If it has a ControllerRef, that's all that matters. - if controllerRef := metav1.GetControllerOf(job); controllerRef != nil && controllerRef.Name == cronJob.Name { - // this job is needs to be reconciled - jobsToBeReconciled = append(jobsToBeReconciled, job) - } - } - return jobsToBeReconciled, nil -} - // When a job is created, enqueue the controller that manages it and update it's expectations. func (jm *ControllerV2) addJob(obj interface{}) { job := obj.(*batchv1.Job) diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go index c61b17b47f568..1472b27d14510 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2_test.go +++ b/pkg/controller/cronjob/cronjob_controllerv2_test.go @@ -29,7 +29,6 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/informers" @@ -1668,107 +1667,6 @@ func TestControllerV2UpdateCronJob(t *testing.T) { } } -func TestControllerV2GetJobsToBeReconciled(t *testing.T) { - trueRef := true - tests := []struct { - name string - cronJob *batchv1.CronJob - jobs []runtime.Object - expected []*batchv1.Job - }{ - { - name: "test getting jobs in namespace without controller reference", - cronJob: &batchv1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}}, - jobs: []runtime.Object{ - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "foo-ns"}}, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo1", Namespace: "foo-ns"}}, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo2", Namespace: "foo-ns"}}, - }, - expected: []*batchv1.Job{}, - }, - { - name: "test getting jobs in namespace with a controller reference", - cronJob: &batchv1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}}, - jobs: []runtime.Object{ - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "foo-ns"}}, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo1", Namespace: "foo-ns", - OwnerReferences: []metav1.OwnerReference{{Name: "fooer", Controller: &trueRef}}}}, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo2", Namespace: "foo-ns"}}, - }, - expected: []*batchv1.Job{ - {ObjectMeta: metav1.ObjectMeta{Name: "foo1", Namespace: "foo-ns", - OwnerReferences: []metav1.OwnerReference{{Name: "fooer", Controller: &trueRef}}}}, - }, - }, - { - name: "test getting jobs in other namespaces", - cronJob: &batchv1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}}, - jobs: []runtime.Object{ - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar-ns"}}, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo1", Namespace: "bar-ns"}}, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: "foo2", Namespace: "bar-ns"}}, - }, - expected: []*batchv1.Job{}, - }, - { - name: "test getting jobs whose labels do not match job template", - cronJob: &batchv1.CronJob{ - ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}, - Spec: batchv1.CronJobSpec{JobTemplate: batchv1.JobTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "value"}}, - }}, - }, - jobs: []runtime.Object{ - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{ - Namespace: "foo-ns", - Name: "foo-fooer-owner-ref", - Labels: map[string]string{"key": "different-value"}, - OwnerReferences: []metav1.OwnerReference{{Name: "fooer", Controller: &trueRef}}}, - }, - &batchv1.Job{ObjectMeta: metav1.ObjectMeta{ - Namespace: "foo-ns", - Name: "foo-other-owner-ref", - Labels: map[string]string{"key": "different-value"}, - OwnerReferences: []metav1.OwnerReference{{Name: "another-cronjob", Controller: &trueRef}}}, - }, - }, - expected: []*batchv1.Job{{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "foo-ns", - Name: "foo-fooer-owner-ref", - Labels: map[string]string{"key": "different-value"}, - OwnerReferences: []metav1.OwnerReference{{Name: "fooer", Controller: &trueRef}}}, - }}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, ctx := ktesting.NewTestContext(t) - ctx, cancel := context.WithCancel(ctx) - defer cancel() - kubeClient := fake.NewSimpleClientset() - sharedInformers := informers.NewSharedInformerFactory(kubeClient, controller.NoResyncPeriodFunc()) - for _, job := range tt.jobs { - sharedInformers.Batch().V1().Jobs().Informer().GetIndexer().Add(job) - } - jm, err := NewControllerV2(ctx, sharedInformers.Batch().V1().Jobs(), sharedInformers.Batch().V1().CronJobs(), kubeClient) - if err != nil { - t.Errorf("unexpected error %v", err) - return - } - - actual, err := jm.getJobsToBeReconciled(tt.cronJob) - if err != nil { - t.Errorf("unexpected error %v", err) - return - } - if !reflect.DeepEqual(actual, tt.expected) { - t.Errorf("\nExpected %#v,\nbut got %#v", tt.expected, actual) - } - }) - } -} - func TestControllerV2CleanupFinishedJobs(t *testing.T) { tests := []struct { name string