Differential D11859

Copy&paste exploits
AbandonedPublic
Actions

Authored by hindenburg on Apr 1 2018, 8:05 AM.

Details

Reviewers
michalhumpula
Group Reviewers
Konsole
Summary

It's quite a common practice today to copy&paste pieces of code/commands from web to editor/terminal. As shown here [1], it's not always apparent what the copied content would be. Konsole implements the bracketed paste mode, which places special escape sequences on the start and end of the pasted text. The shell/editor can later decide how to interpret them.

Current konsole implementation limits only the first variant. The second variant evades the escapes sequences, by terminating the first one. The easiest solution is to remove the problematic sequences from the pasted text, which prevents the snippet from escaping the bracketed mode.

Questions:

  1. is there a better way, how to protect the shell from harm?
  2. can figure out the scenario, where it would be valid to paste such escape sequences and expect them to be honored, but maybe there is?
  3. should it be configurable? If the answer to 2. is there is no such scenario, then it doesn't seem practical to let user disable the paste cleanup.

[1]: http://thejh.net/misc/website-terminal-copy-paste

Diff Detail

Repository
R319 Konsole
Lint
Lint Skipped
Unit
Unit Tests Skipped
michalhumpula created this revision.Apr 1 2018, 8:05 AM
Restricted Application added a project: Konsole. · View Herald TranscriptApr 1 2018, 8:05 AM
michalhumpula requested review of this revision.Apr 1 2018, 8:05 AM
hindenburg added a subscriber: hindenburg.Apr 2 2018, 3:35 PM

I don't see any downsides - any reason to not remove 200 as well?

mglb added a subscriber: mglb.EditedApr 2 2018, 5:26 PM

How does this handle pasting of something like this: "\033\033[201~[201~" (I can't check right now)?
This can be defeated with \033\033[201~[201~.
Why not remove all control characters (except \n, \t, etc.) like xterm does?
Also, please link this with the bug: https://bugs.kde.org/show_bug.cgi?id=392554

sandsmark added a subscriber: sandsmark.Apr 5 2018, 12:49 PM

this was the patch I was about to upload before I saw this, I think this makes more sense:

diff --git src/TerminalDisplay.cpp src/TerminalDisplay.cpp
index 0caa3997..98ba493a 100644
--- src/TerminalDisplay.cpp
+++ src/TerminalDisplay.cpp
@@ -3187,6 +3187,7 @@ void TerminalDisplay::doPaste(QString text, bool appendReturn)
if (!text.isEmpty()) {
text.replace(QLatin1Char('\n'), QLatin1Char('\r'));
if (bracketedPasteMode()) {
+            text.remove(QLatin1String("\033"));
text.prepend(QLatin1String("\033[200~"));
text.append(QLatin1String("\033[201~"));
}
hindenburg added a comment.Apr 5 2018, 10:35 PM
In D11859#238646, @mglb wrote:

Why not remove all control characters (except \n, \t, etc.) like xterm does?
Also, please link this with the bug: https://bugs.kde.org/show_bug.cgi?id=392554

I would agree that this seems to be the way to go in the future

hindenburg added a comment.Apr 6 2018, 2:21 PM

Do we agree this is the way to go for now?

+ text.remove(QLatin1String("\033"));

mglb added a comment.Apr 6 2018, 9:23 PM

I agree. Most minimal way to fully solve the problem.

hindenburg commandeered this revision.Apr 7 2018, 3:43 PM
hindenburg added a reviewer: michalhumpula.

OK committed - thanks for the patches and reports - https://commits.kde.org/konsole/0b482990279d6684089a404df7473f0354c284c3

hindenburg abandoned this revision.Apr 7 2018, 3:43 PM
ngraham added a subscriber: ngraham.Apr 7 2018, 11:47 PM

Could this be backported to 18.04? It's always good to get security fixed into users' hands faster.

Revision Contents
Changeset List

PathPackages
Msrc/TerminalDisplay.cpp (1 line)

Diff 31086

View Options

src/TerminalDisplay.cpp

Loading...