Strip control characters from pasted text

dnkl commented

2021-01-20 18:23:53 +00:00

Owner

We used to replace \r\n with \n. This PR changes that to instead use \r. That is, we replace \r\n and \n with \r.

This PR also strips \x1b from pasted text, to prevent e.g. bracketed paste from being disabled by including the "bracketed paste" end sequence in the text being pasted.

Closes #305
Closes #306

We used to replace `\r\n` with `\n`. This PR changes that to instead use `\r`. That is, we replace `\r\n` **and** `\n` with `\r`. This PR also strips `\x1b` from pasted text, to prevent e.g. bracketed paste from being disabled by including the "bracketed paste" end sequence in the text being pasted. Closes #305 Closes #306

dnkl added the

bug

label 2021-01-20 18:23:53 +00:00

dnkl referenced this pull request

2021-01-20 18:24:37 +00:00

Bracketed Paste #305

craigbarnes commented

2021-01-20 19:26:44 +00:00

Collaborator

Tested working in bash.

This is perhaps a separate issue, but I don't think this PR completely addresses the problem described by http://thejh.net/misc/website-terminal-copy-paste, unless bracketed paste is enabled. For example, if you start foot dash and paste the offending text, it still immediately runs the commands.

xfce4-terminal seems to "solve" this problem by prompting the user (see attached image) if the text contains newlines. Pasting text without newlines seems to work without any warning, since in that case it at least gives the user a chance to see the pasted text and choose whether or not to press Enter (or Ctrl+C).

Tested working in `bash`. This is perhaps a separate issue, but I don't think this PR completely addresses the problem described by http://thejh.net/misc/website-terminal-copy-paste, unless bracketed paste is enabled. For example, if you start `foot dash` and paste the offending text, it still immediately runs the commands. `xfce4-terminal` seems to "solve" this problem by prompting the user (see attached image) if the text contains newlines. Pasting text without newlines seems to work without any warning, since in that case it at least gives the user a chance to see the pasted text and choose whether or not to press <kbd>Enter</kbd> (or <kbd>Ctrl</kbd>+<kbd>C</kbd>).

1611170571.png

21 KiB

JorwLNKwpH commented

2021-01-20 22:55:19 +00:00

Tested and it fixes the second example on http://thejh.net/misc/website-terminal-copy-paste. Although @craigbarnes points out the problem is not completely fixed.

However, the first issue is not yet fixed. I think foot needs to handle more control sequences, like: https://security.stackexchange.com/questions/39118/how-can-i-protect-myself-from-this-kind-of-clipboard-abuse/52655#52655

And it also needs to send a BRACKETED_PASTE terminal mode to notify the application (this is most likely the issue I experienced; the multi-line issue was a red-herring since the bug specifically triggered for multi-line input).

Tested and it fixes the second example on http://thejh.net/misc/website-terminal-copy-paste. Although @craigbarnes points out the problem is not completely fixed. However, the first issue is not yet fixed. I think foot needs to handle more control sequences, like: https://security.stackexchange.com/questions/39118/how-can-i-protect-myself-from-this-kind-of-clipboard-abuse/52655#52655 And it also needs to send a BRACKETED_PASTE terminal mode to notify the application (this is most likely the issue I experienced; the multi-line issue was a red-herring since the bug specifically triggered for multi-line input).

JorwLNKwpH commented

2021-01-21 06:44:49 +00:00

@dnkl Do you think this will make it to a minor point release since it seems
security relevant? Specifically, the part about filtering control sequences.

Also, it might be useful to double-check xterm's current implementation in case anything else is missing for bracketed-paste.

@dnkl Do you think this will make it to a minor point release since it seems security relevant? Specifically, the part about filtering control sequences. Also, it might be useful to double-check xterm's current implementation in case anything else is missing for bracketed-paste.

dnkl commented

2021-01-21 10:20:16 +00:00

Poster

Owner

Although @craigbarnes points out the problem is not completely fixed.

The "problem" in this case is detecting malicious paste attacks. What he means is that filtering out \E does not prevent all the ways paste can be abused in a terminal.

I think foot needs to handle more control sequences, like: https://security.stackexchange.com/questions/39118/how-can-i-protect-myself-from-this-kind-of-clipboard-abuse/52655#52655

That would indeed take things further and prevent more attack vectors.

However, the first issue is not yet fixed
...
And it also needs to send a BRACKETED_PASTE terminal mode to notify the application (this is most likely the issue I experienced; the multi-line issue was a red-herring since the bug specifically triggered for multi-line input).

See #305 (comment). TL;DR this doesn't have anything to do with bracketed paste mode.

There was a short discussion on IRC about detecting multiline pastes in non bracketed paste mode, and asking the user for confirmation before allowing the paste:

21:04:04 cbb  | specifically; having some kind of warning for non-bracketed pastes that contain newlines
21:05:42 dnkl | Yeah, that would be simple, and would catch most paste hacks I think
21:06:35 dnkl | And also wouldn't get in your way since all modern editors, and shells intended for interactive work, enable bracketed paste
21:07:55 dnkl | Of course, foot not using a toolkit means showing a popup/dialog involves a bit of work...
21:08:14 cbb  | yeah, that was what I was leading to

https://freenode.logbot.info/foot-terminal/20210120#c6599846

Do you think this will make it to a minor point release since it seems security relevant? Specifically, the part about filtering control sequences.

Yes, I think we should look into filtering control sequences, and yes, that can likely be backported to a 1.6.x point release.

Detecting and asking for confirmation before pasting multiline text is out of scope for this PR, and maybe out of scope for a point release. But it is something that should eventually be fixed.

> Although @craigbarnes points out the problem is not completely fixed. The "problem" in this case is detecting malicious paste attacks. What he means is that filtering out `\E` does not prevent **all** the ways paste can be abused in a terminal. > I think foot needs to handle more control sequences, like: https://security.stackexchange.com/questions/39118/how-can-i-protect-myself-from-this-kind-of-clipboard-abuse/52655#52655 _That_ would indeed take things further and prevent more attack vectors. > However, the first issue is not yet fixed > ... > And it also needs to send a BRACKETED_PASTE terminal mode to notify the application (this is most likely the issue I experienced; the multi-line issue was a red-herring since the bug specifically triggered for multi-line input). See https://codeberg.org/dnkl/foot/issues/305#issuecomment-170644. TL;DR this doesn't have anything to do with bracketed paste mode. There was a short discussion on IRC about detecting multiline pastes in **non** bracketed paste mode, and asking the user for confirmation before allowing the paste: ``` 21:04:04 cbb | specifically; having some kind of warning for non-bracketed pastes that contain newlines 21:05:42 dnkl | Yeah, that would be simple, and would catch most paste hacks I think 21:06:35 dnkl | And also wouldn't get in your way since all modern editors, and shells intended for interactive work, enable bracketed paste 21:07:55 dnkl | Of course, foot not using a toolkit means showing a popup/dialog involves a bit of work... 21:08:14 cbb | yeah, that was what I was leading to ``` https://freenode.logbot.info/foot-terminal/20210120#c6599846 > Do you think this will make it to a minor point release since it seems security relevant? Specifically, the part about filtering control sequences. Yes, I think we should look into filtering control sequences, and yes, that can likely be backported to a 1.6.x point release. Detecting and asking for confirmation before pasting multiline text is out of scope for this PR, and maybe out of scope for a point release. But it is something that should eventually be fixed.

dnkl commented

2021-01-21 14:49:48 +00:00

Poster

Owner

From what I can tell, XTerm strips all non-formatting C0 characters, and all C1 characters by default.

This list can be extended by setting disallowedPasteControls:

The allowPasteControls resource is normally used to prevent pasting C1 controls, as well as non-formatting C0 controls such as the ASCII escape character. Those characters are simply ignored. This resource further extends the set of control characters which cannot be pasted, converting each into a space.

The resource value is a comma-separated list of names. Xterm ignores capitalization. The default value is BS,DEL,ENT,EOT,ESC,NUL

But:

$  grep DEF_DISALLOWED_PASTE *
main.h:#ifndef DEF_DISALLOWED_PASTE_CONTROLS
main.h:#define DEF_DISALLOWED_PASTE_CONTROLS    "BS,HT,DEL"

Also not sure what ENT is?

Since foot doesn't support C1 controls, and since they may be part of UTF-8 sequences, we should leave those be.

As for the other, filtering non-formatting C0, BS, DEL, HT, EOT, ESC and NUL should be ok, I think?

From what I can tell, XTerm strips all non-formatting C0 characters, and all C1 characters by default. This list can be extended by setting `disallowedPasteControls`: > The allowPasteControls resource is normally used to prevent pasting C1 controls, as well as non-formatting C0 controls such as the ASCII escape character. Those characters are simply ignored. This resource further extends the set of control characters which cannot be pasted, converting each into a space. > > The resource value is a comma-separated list of names. Xterm ignores capitalization. The default value is BS,DEL,ENT,EOT,ESC,NUL But: ``` $ grep DEF_DISALLOWED_PASTE * main.h:#ifndef DEF_DISALLOWED_PASTE_CONTROLS main.h:#define DEF_DISALLOWED_PASTE_CONTROLS "BS,HT,DEL" ``` Also not sure what `ENT` is? Since foot doesn't support C1 controls, and since they may be part of UTF-8 sequences, we should leave those be. As for the other, filtering non-formatting C0, `BS`, `DEL`, `HT`, `EOT`, `ESC` and `NUL` should be ok, I think?

dnkl force-pushed selection-multiline-paste-fix from 68333948d7 to 5318d27212

2021-01-23 09:46:03 +00:00

Compare

dnkl added 1 commit 2021-01-23 10:22:48 +00:00

fb9a225eee

selection: strip non-formatting C0, BS, HT and DEL from pasted text

dnkl commented

2021-01-23 10:27:49 +00:00

Poster

Owner

Updated to strip non-formatting C0, BS, HT and DEL.

One difference between foot and XTerm is that XTerm strips non-formatting C0 (like foot), but replaces the remaining characters with a space. Furthermore, which additional characters (i.e. formatting C0 and "other" - BS, HT and DEL in our case) that should be replaced is configurable in XTerm.

I'd rather not add configuration options unless there's an actual use case. But if there's a need for it, we can of course make it configurable in foot too.

The remaining question is whether to strip or replace with space? As mentioned above, XTerm strips non-formatting characters, but replaces the remaining ones. I guess the idea is that non-formatting characters can be stripped because they don't affect the resulting text? While formatting characters are replaced to indicate something was there?

Updated to strip non-formatting C0, `BS`, `HT` and `DEL`. One difference between foot and XTerm is that XTerm **strips** non-formatting C0 (like foot), but **replaces** the remaining characters with a space. Furthermore, _which_ additional characters (i.e. formatting C0 and "other" - `BS`, `HT` and `DEL` in our case) that should be replaced is configurable in XTerm. I'd rather not add configuration options unless there's an actual use case. But if there's a need for it, we can of course make it configurable in foot too. The remaining question is whether to strip or replace with space? As mentioned above, XTerm strips non-formatting characters, but replaces the remaining ones. I guess the idea is that non-formatting characters can be stripped because they don't affect the resulting text? While formatting characters are replaced to indicate something was there?

dnkl added 2 commits 2021-01-23 10:29:43 +00:00

2472c3b8c3

selection: codespell: stript -> strip

81bcf6e8e0

selection: DEL is 0x7f, not 0x1f

dnkl added 1 commit 2021-01-23 10:30:48 +00:00

3db57968f2

selection: remove duplicate ‘ESC’ in switch case

dnkl changed title from ~~Replace \r\n and \n with \r, and strip \e from pasted text~~ to Strip control characters from pasted text

2021-01-23 10:31:27 +00:00

dnkl closed this pull request

2021-01-23 10:31:52 +00:00