Escape terminal special characters in kubectl #112553

dgl · 2022-09-19T04:57:17Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Escapes terminal special characters in kubectl output. These can be used to confuse the user of kubectl, or potentially more serious attacks if their terminal emulator has a bug, as mentioned in the related issue.

Which issue(s) this PR fixes:

Fixes #101695

Special notes for your reviewer:

I've covered:

kubectl get (default table view)
kubectl get events -o custom-columns=message:".message"
kubectl describe
kubectl alpha events

Which I believe covers the original report along with the follow-up mentioned in #107617. Additionally I've confirmed that kubectl get -o json and kubectl get -o yaml perform their own escaping.

In addition to the original report I noticed that \r is allowed in various contexts, which can be used to replace the current line, and therefore still trick a user, so that is also escaped.

There are some cases which will not be covered, e.g. users using -o template=..., this is harder to fix as the user could be expecting to parse these outputs and ad-hoc human readable escaping may not be suitable. I believe these are less of a concern as the main commands are covered (we could consider detecting if the tty is a terminal or not, as done in some places in kubectl already, but then there are still issues, e.g. a user piping to grep).

Does this PR introduce a user-facing change?

kubectl now escapes terminal special characters in output. This fixes CVE-2021-25743.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-09-19T04:57:25Z

Welcome @dgl!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-09-19T04:57:26Z

Hi @dgl. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dgl · 2022-09-19T04:57:58Z

/sig cli
/sig security

pacoxu · 2022-09-21T08:27:26Z

/ok-to-test
/priority important-longterm
/triage accepted

pacoxu · 2022-09-21T08:29:01Z

/assign @soltysh

pacoxu

/lgtm

ardaguclu · 2022-10-13T12:13:00Z

staging/src/k8s.io/cli-runtime/pkg/printers/tableprinter.go

 					if truncated {
 						fmt.Fprint(output, "...")
 					}
 				default:
-					fmt.Fprint(output, val)
+					WriteEscaped(output, fmt.Sprint(val))


Sorry, I'm not fully aware of the context of this change. Why we are fmt.Sprint(val)?

It's to get val into a string (i.e. via Stringer or the rules fmt uses), it made sense to me to make the new WriteEscaped take a string, rather than put the formatting over there, which might not match all caller's expectations.

ardaguclu · 2022-10-13T12:14:27Z

staging/src/k8s.io/kubectl/pkg/cmd/events/event_printer.go

-		e.Reason,
-		e.InvolvedObject.Kind, e.InvolvedObject.Name,
-		strings.TrimSpace(e.Message),
+		printers.EscapeTerminal(e.Type),


Aren't events only generated by kubernetes itself?, how user can generate events needed to be escaped?

See the kubectl create command mentioned in #101695 (comment) -- it's possible for a user (with enough permission against say a namespace) to do this, which could then be viewed by an admin or other user.

It might also be possible to get a \x1B into an error message that an operator or other component ends up putting into an event -- it doesn't make sense that every component should have to consider terminal escaping that is only relevant to kubectl (in the same way HTML escaping is usually done by the component that renders the HTML).

ardaguclu · 2022-10-13T12:16:05Z

staging/src/k8s.io/kubectl/pkg/describe/describe.go

@@ -148,11 +149,13 @@ func (pw *prefixWriter) Write(level int, format string, a ...interface{}) {
 	for i := 0; i < level; i++ {
 		prefix += levelSpace
 	}
-	fmt.Fprintf(pw.out, prefix+format, a...)
+	output := fmt.Sprintf(prefix+format, a...)


fmt.Sprintf automatically escapes special characters?

This formats it into a string, where WriteEscaped on the next line does that.

soltysh

/lgtm
/approve

we still have annotations to cover as mentioned in the original issue, I doesn't look like this PR covers that bit, but since it's improving the current situation it's definitely worthy merge as is, and work on annotations in a follow

k8s-ci-robot · 2022-10-30T11:47:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgl, pacoxu, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/cli-runtime/OWNERS~~ [soltysh]
~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2022-10-30T11:47:13Z

/labe tide/merge-method-squash

soltysh · 2022-10-30T11:48:09Z

/label tide/merge-method-squash

pacoxu · 2022-10-30T13:27:10Z

/retest
for a flake Test_Run_OneVolumeDetachFailNodeWithReadWriteOnce

buckaroogeek · 2022-11-28T16:42:07Z

Will this fix be available in 1.25 and 1.24? I tried to find relevant guidance in the Kubernetes security documentation but did not see anything. thanks

* Escape terminal special characters in kubectl * Add escaping for kubectl alpha events

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 19, 2022

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 19, 2022

k8s-ci-robot added the sig/security Categorizes an issue or PR as relevant to SIG Security. label Sep 19, 2022

k8s-ci-robot requested review from dougsland and seans3 September 19, 2022 04:58

dgl force-pushed the fix-101695 branch from 8a0e8db to 9fcce08 Compare September 19, 2022 04:59

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 19, 2022

k8s-ci-robot assigned soltysh Sep 21, 2022

pacoxu mentioned this pull request Sep 21, 2022

ANSI escape characters in kubectl output are not being filtered #101695

Closed

dgl added 2 commits September 21, 2022 11:58

Escape terminal special characters in kubectl

f3ebe9f

Add escaping for kubectl alpha events

53e1268

dgl force-pushed the fix-101695 branch from 0b9ed81 to 53e1268 Compare September 21, 2022 11:59

pacoxu approved these changes Oct 9, 2022

View reviewed changes

k8s-ci-robot assigned pacoxu Oct 9, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2022

dgl mentioned this pull request Oct 10, 2022

[GHSA-f9jg-8p32-2f55] ANSI escape characters not filtered github/advisory-database#736

Merged

ardaguclu reviewed Oct 13, 2022

View reviewed changes

soltysh approved these changes Oct 30, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2022

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 30, 2022

k8s-ci-robot merged commit dad0e93 into kubernetes:master Oct 30, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 30, 2022

jaehnri pushed a commit to jaehnri/kubernetes that referenced this pull request Jan 3, 2023

Escape terminal special characters in kubectl (kubernetes#112553)

46d0514

* Escape terminal special characters in kubectl * Add escaping for kubectl alpha events

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape terminal special characters in kubectl #112553

Escape terminal special characters in kubectl #112553

dgl commented Sep 19, 2022 •

edited

k8s-ci-robot commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

dgl commented Sep 19, 2022

pacoxu commented Sep 21, 2022

pacoxu commented Sep 21, 2022

pacoxu left a comment

ardaguclu Oct 13, 2022

dgl Oct 14, 2022

ardaguclu Oct 13, 2022

dgl Oct 14, 2022

ardaguclu Oct 13, 2022

dgl Oct 14, 2022

soltysh left a comment

k8s-ci-robot commented Oct 30, 2022

soltysh commented Oct 30, 2022

soltysh commented Oct 30, 2022

pacoxu commented Oct 30, 2022

buckaroogeek commented Nov 28, 2022

Escape terminal special characters in kubectl #112553

Escape terminal special characters in kubectl #112553

Conversation

dgl commented Sep 19, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

dgl commented Sep 19, 2022

pacoxu commented Sep 21, 2022

pacoxu commented Sep 21, 2022

pacoxu left a comment

Choose a reason for hiding this comment

ardaguclu Oct 13, 2022

Choose a reason for hiding this comment

dgl Oct 14, 2022

Choose a reason for hiding this comment

ardaguclu Oct 13, 2022

Choose a reason for hiding this comment

dgl Oct 14, 2022

Choose a reason for hiding this comment

ardaguclu Oct 13, 2022

Choose a reason for hiding this comment

dgl Oct 14, 2022

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 30, 2022

soltysh commented Oct 30, 2022

soltysh commented Oct 30, 2022

pacoxu commented Oct 30, 2022

buckaroogeek commented Nov 28, 2022

dgl commented Sep 19, 2022 •

edited