Sanitize paste content by filtering out control chars

[skyline75489]

Summary of the Pull Request

Address the security issued mentioned in #395 (comment)

References

Supersedes #7508

PR Checklist

• Closes #xxx
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated.
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

[zadjii-msft]

@msftbot make sure @DHowett signs off on this

[ghost]

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I'll only merge this pull request if it's approved by @DHowett

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[zadjii-msft]

This seems reasonable to me, but I want Dustin to take a look too, since he's the expert in that particular area.

[DHowett]

(making my NAK an actual "request changes")

[skyline75489]

I should probably mention that thanks to #8744, the potentially malicious content from the clipboard is being displayed in a dazzling dialog. This should prevent users from pasting it in most conditions.

[skyline75489]

This PR will still help prevent attacks described in https://security.stackexchange.com/q/39118. Currently the dialog isn't showing invisible control characters.

[skyline75489]

Inlining this in std::remove_if results in very weird indention using clang-format. Nesting this inside sanitize crashes clang-format(!). So I had to put it here.

[skyline75489]

Removing CR and LF seems meaningless from a practical point of view. But generally I think this function should remove CR and LF had it not been in this particular context.

[skyline75489]

(copied from #7508)

The best resource I can find is here https://security.stackexchange.com/a/52655. It suggest removing every control characters except:

• CR(0x0d) \r
• LF(0x0a) \n
• HT(0x09) \t,
• DEL(0x7F)

Then there's some other opinions in https://bugzilla.gnome.org/show_bug.cgi?id=753197, regarding removing:

• 0x80-0x9F

And here https://bugzilla.gnome.org/show_bug.cgi?id=794653, regarding removing:

• BS(0x08)
• DEL(0x7F)

[jantari]

I just want to say I do intentionally paste text with VT control sequences into the terminal, usually just for colors or other effects. So I'm not sure how I feel about 0x1b aka ESC being stripped

[DHowett]

I suspect you are in the minority here. Many other terminal emulators filter out control characters during bracketed paste, and an application shouldn't be expecting to receive raw SGRs in its input stream anyway.

[jantari]

Yea I understand that, but what about an option - either in the JSON config, or directly in the new paste dialog:

When it detects non-printable or control-characters in the text, it could present a third, non-focused button: "Paste with special characters"

[DHowett]

It's just that... every setting we add is another combinatorial factor in our testing, right? There's already sendInput for sending raw controlled input. I just don't see the use case for pasting control characters today..?

[jantari]

Good point, possible usecases were testing string snippets for shell prompts (bash $PS1 or powershells' prompt function) or other snippets of fancy text. It's not a big issue though, by now I've more or less switched entirely to shell-sequences like \033 in bash or the ugly $([char]27) in powershell to get ESC rather than a raw ESC character

Since you said SendInput is not affected by this behavior/change - I assume the same holds true for WriteConsoleInput?

[DHowett]

The code change in this file, and the policy we're instating here, implicates only paste.

We're not breaking Win32 APIs willy-nilly. 😄

Also: PowerShell Core supports "`e" for escaping escape.

[skyline75489]

I am no expert on security issue like this. I think this PR needs to be thoroughly reviewed. Feel free to tag this to 1.7 or future release. No need to rush this one.

[skyline75489]

I think I should also add this to conhost. But I don't know conhost that much. I'll file a issue for conhost if this is merged.

[DHowett]

I appreciate this, but I do think that it needs some changes.

I'm not a huge fan of the lambda filtering implementation; I think that it would be much better for us to have one loop that checks for \n and all the other control sequences at the same time.

As it is implemented in this PR, we scan the string to determine that it doesn't contain \n, and then we re-scan the portion between 0 and the first \n, then we re-scan the portion between that \n and the next one, and then ... (etc) to detect control sequences.

Same concern about the bracketed paste function. Why have a lambda?

Lastly- I wonder if this should be inside TerminalCore so that the WPF control can use it. Needing the correct filtering for \r\n and for control sequences is not something unique to the TerminalControl!

[skyline75489]

The main reason I used lambda is to minimize the modification to the existing code. Yea it does introduced extra scan loop. I can’t blame you for wanting this to run faster. I’ll try to refactor this later.

[skyline75489]

Closed in favor of #9034