Command Injection

Proof-of-concept (PoC) to illustrate the vulnerability and the fix: Assumptions: - A Kubernetes cluster is available with kubectl configured. - A pod exists (e.g., pod-name in namespace pod-ns) and has a writable path in which we can place a file for testing (or we rely on an existing path). - The vulnerable behavior is triggered when kubectl cp streams a tar from a pod path via a shell that does not escape the path properly. Vulnerable scenario (pre-fix behavior): - If a user supplies a path containing a shell metacharacter, e.g. /tmp/dir'; uname -a, the remote command constructed for tar could become: sh -c tar cf - '/tmp/dir'; uname -a | tail -c+N - The semicolon ends the tar command and the injected command (uname -a) runs inside the container, leaking the system information and demonstrating command execution. Exploit payload example (pre-fix): - Pod path: /tmp/dir'; uname -a - kubectl cp pod-ns/pod-name:/tmp/dir'; uname -a /tmp/local - The remote shell executes tar cf - '/tmp/dir'; uname -a | tail -c+N, thereby printing kernel/host information from the pod. Fixed behavior (post-fix): - The path is escaped and wrapped safely in single quotes, using a pattern that prevents shell interpretation of embedded quotes or metacharacters. - New command (post-fix): sh -c "tar cf - '/tmp/dir'\''; uname -a' | tail -c+N" (example of the escaping technique showing how the path is safely embedded) - The injected commands are not executed because the path is treated as a literal string argument rather than executable code. Minimal reproducible code illustration (pseudo Go-like framing): // vulnerable (pre-fix) construction cmdPre := fmt.Sprintf("sh -c tar cf - %s | tail -c+%d", path, n) // safe (post-fix) construction escaped := strings.ReplaceAll(path, "'", `'\\''`) cmdPost := fmt.Sprintf("sh -c tar cf - '%s' | tail -c+%d", escaped, n) Expected outcome: - Pre-fix: an injected command (e.g., uname -a) can execute in the container, leaking information or enabling further actions. - Post-fix: path is properly escaped and quoted, preventing injection; only the intended tar operation runs.

diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go index f8e2faa097c7a..78d44e41196b0 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go @@ -76,6 +76,7 @@ type CopyOptions struct { ClientConfig *restclient.Config Clientset kubernetes.Interface ExecParentCmdName string + Executor exec.RemoteExecutor args []string @@ -204,6 +205,7 @@ func (o *CopyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []str if cmd.Parent() != nil { o.ExecParentCmdName = cmd.Parent().CommandPath() } + o.Executor = &exec.DefaultRemoteExecutor{} var err error o.Namespace, _, err = f.ToRawKubeConfigLoader().Namespace() @@ -277,7 +279,7 @@ func (o *CopyOptions) checkDestinationIsDir(dest fileSpec) error { }, Command: []string{"test", "-d", dest.File.String()}, - Executor: &exec.DefaultRemoteExecutor{}, + Executor: o.Executor, } ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) @@ -345,7 +347,7 @@ func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) e } options.Command = cmdArr - options.Executor = &exec.DefaultRemoteExecutor{} + options.Executor = o.Executor return o.execute(options) } @@ -391,10 +393,11 @@ func (t *TarPipe) initReadFrom(n uint64) { }, Command: []string{"tar", "cf", "-", t.src.File.String()}, - Executor: &exec.DefaultRemoteExecutor{}, + Executor: t.o.Executor, } if t.o.MaxTries != 0 { - options.Command = []string{"sh", "-c", fmt.Sprintf("tar cf - %s | tail -c+%d", t.src.File, n)} + escapedPath := strings.ReplaceAll(t.src.File.String(), "'", `'\''`) + options.Command = []string{"sh", "-c", fmt.Sprintf("tar cf - '%s' | tail -c+%d", escapedPath, n)} } go func() { diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go index b0038d0f3fc1b..95ba4216aca6e 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go @@ -19,9 +19,11 @@ package cp import ( "archive/tar" "bytes" + "context" "fmt" "io" "net/http" + "net/url" "os" "path/filepath" "reflect" @@ -33,10 +35,13 @@ 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/cli-runtime/pkg/genericiooptions" + restclient "k8s.io/client-go/rest" "k8s.io/client-go/rest/fake" + "k8s.io/client-go/tools/remotecommand" kexec "k8s.io/kubectl/pkg/cmd/exec" cmdtesting "k8s.io/kubectl/pkg/cmd/testing" "k8s.io/kubectl/pkg/scheme" @@ -674,6 +679,113 @@ func TestCopyToPod(t *testing.T) { } } +func TestCopyFromPod(t *testing.T) { + tf := cmdtesting.NewTestFactory().WithNamespace("test") + ns := scheme.Codecs.WithoutConversion() + codec := scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) + + tf.Client = &fake.RESTClient{ + GroupVersion: schema.GroupVersion{Group: "", Version: "v1"}, + NegotiatedSerializer: ns, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + responsePod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "pod-name", Namespace: "pod-ns"}, + Spec: v1.PodSpec{Containers: []v1.Container{{Name: "container"}}}, + } + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: io.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(codec, responsePod))))}, nil + }), + } + + tf.ClientConfigVal = cmdtesting.DefaultClientConfig() + ioStreams, _, _, _ := genericiooptions.NewTestIOStreams() + + cmd := NewCmdCp(tf, ioStreams) + + destDir, err := os.MkdirTemp("", "test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer os.RemoveAll(destDir) + + tests := map[string]struct { + src string + dest string + podName string + retries int + expectedErr string + expectedCommand string + }{ + "copy from pod to empty path": { + src: "pod-ns/pod-name:/tmp/foo", + dest: "", + expectedErr: "filepath can not be empty", + }, + "path without single quotes": { + src: "pod-ns/pod-name:/tmp/foo", + dest: destDir, + podName: "pod-name", + expectedCommand: "tar cf - /tmp/foo", + }, + "path with single quotes": { + src: "pod-ns/pod-name:/tmp/path'with'quotes", + dest: destDir, + podName: "pod-name", + retries: 1, + expectedCommand: `sh -c tar cf - '/tmp/path'\''with'\''quotes' | tail -c+1`, + }, + } + + for name, test := range tests { + opts := NewCopyOptions(ioStreams) + opts.MaxTries = test.retries + if err := opts.Complete(tf, cmd, []string{test.src, test.dest}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + remoteExec := &testingRemoteExecutor{} + opts.Executor = remoteExec + t.Run(name, func(t *testing.T) { + err := opts.Run() + if len(test.expectedErr) > 0 { + if err == nil { + t.Fatalf("expected error but got none") + } + if !strings.Contains(err.Error(), test.expectedErr) { + t.Errorf("expected error to contain %q, got: %v", test.expectedErr, err) + } + } + if len(test.expectedErr) == 0 && err != nil { + t.Errorf("unexpected error: %v", err) + } + if !strings.Contains(remoteExec.capturedPath, test.podName) { + t.Errorf("missing pod name %q in the captured path: %q", test.podName, remoteExec.capturedPath) + } + query, err := url.ParseQuery(remoteExec.capturedQuery) + if err != nil { + t.Errorf("unexpected error parsing captured query: %v", err) + } + actualQuery := strings.Join(query["command"], " ") + if actualQuery != test.expectedCommand { + t.Errorf("unexpected command, got %q, expected: %q", actualQuery, test.expectedCommand) + } + }) + } +} + +type testingRemoteExecutor struct { + capturedPath string + capturedQuery string +} + +func (t *testingRemoteExecutor) Execute(url *url.URL, config *restclient.Config, stdin io.Reader, stdout, stderr io.Writer, tty bool, terminalSizeQueue remotecommand.TerminalSizeQueue) error { + return t.ExecuteWithContext(context.Background(), url, config, stdin, stdout, stderr, tty, terminalSizeQueue) +} + +func (t *testingRemoteExecutor) ExecuteWithContext(ctx context.Context, url *url.URL, config *restclient.Config, stdin io.Reader, stdout, stderr io.Writer, tty bool, terminalSizeQueue remotecommand.TerminalSizeQueue) error { + t.capturedPath = url.Path + t.capturedQuery = url.RawQuery + return nil +} + func TestCopyToPodNoPreserve(t *testing.T) { tf := cmdtesting.NewTestFactory().WithNamespace("test") ns := scheme.Codecs.WithoutConversion()