-
Notifications
You must be signed in to change notification settings - Fork 8.9k
Don't trim bracketed pastes #19067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't trim bracketed pastes #19067
Conversation
For the record, this seems wrong to me (assuming I've understood the change correctly). Because trimming carriage returns from the end of a paste is in some ways a security feature, and this is disabling that protection in the one situation where it could be most helpful. That said, this is already a problem with multiline pastes (see #13014), so it's not any worse than it was before. It just feels like in both of these cases that we're misleading users. We have options to trim trailing whitespace, and warn on multiline paste, and they're labelled as Not that I'm expecting you to solve that now. I just wanted to note my concern. |
But this PR will only skip trimming if the paste is bracketed (the |
First off, neither the cmd shell nor powershell are aware of bracketed paste. And even in shells that do support bracketed paste, it can be bypassed if the attacker has access to your input (i.e. you've connected to a remote system or application, rather than just viewing a malicious file). But as I said, this is already an issue, so this PR doesn't make it any worse. It's just another vector that would need to be addressed if we ever did want to fix the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
void TermControl::_CopyCommandHandler(const IInspectable& /*sender*/, | ||
const IInspectable& /*args*/) | ||
{ | ||
// formats = nullptr -> copy all formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14/15 - just the big guy left, sorry for the delay
That's fine in this case, right? Since neither CMD nor PowerShell request bracketed paste, they will get content that has been filtered for control characters and/or the user will get a prompt about it being multi-line text. IMO, an attacker with access to your input doesn't need to wait for a paste to do nefarious things. I do not know if this opinion is upheld in reality though! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I don't looooove this kind of "behind the scenes" collaboration.
It appears sound, but if Edit only works 100% properly in our terminal emulator then we've done something wrong... right?
What happens if the same content is put back on the clipboard (by clipboard history, for example) before we receive it? How do we ensure that a user sees the same behavior when they paste the first time versus the next?
I'd love to know if this behavior is observable anywhere else; how do other terminal emulators that support bracketed paste and OSC 52 handle it.
I'd rather lean towards "bracketed paste never mangles the paste except to remove ESC
" than "bracketed paste acts different if it is pasting content just created by the self-same terminal window that produced the content"
(also this means if you put something on the clipboard with edit in one tab, then paste it into zsh in another tab, you get different behavior than e.g. zsh in another window)
But bracketed paste can be turned on with an escape sequence, so that protection can be bypassed just by viewing a text file in the terminal. Create a file that displays a nice little ASCII animation, ending with a message saying something like "Press Ctrl-V to view again" or "Ctrl-V to exit", and you also hide the prompt so it looks like the demo is still "running". Upload the file to a bunch of forums encouraging people to view it in their terminal. What percentage of users do you imagine would fall for that? If it's greater than zero, that's a successful attack.
Just making a telnet connection is all that's needed to give an attacker access to your input. Imagine something along the lines of the old towel.blinkenlights.nl Star Wars animation. Would you consider that a risky thing to connect to? Other than writing to your clipboard, what other nefarious things could a remote host do to your system? |
I don't believe so. It's the same issue with any other OSC52 application after all. Edit doesn't rely on bracketed paste per-se either. I have unbound Ctrl+V for instance, and so I personally don't have this issue at all.
You mean, whether this logic still takes effect? No, it wouldn't. It uses the clipboard counter and so the counter from another application (e.g. clipboard history) would be different.
Continuing from above, it's us who chose to bind Ctrl+V and so it's similarly up to us now to make it work transparently IMO. This PR generally improves the behavior of the terminal however (also IMO), and I think it would be worthwhile for others to adopt a similar behavior.
Is there a reason we don't do that? I'd be happy to do that.
@j4james If we were to continue with this PR or Dustin's suggestion (not trimming trailing newlines on bracketed paste), would the newline warning dialog during paste not be a sufficient protection? |
I think the warning dialog would be sufficient if we didn't provide a super-easy method for attackers to turn it off. This is the point I was trying to make in #13014. And I personally don't care too much. This is basically a social engineering attack - you need to trick a user into pressing Ctrl+V, or right clicking, or whatever shortcut they have configured. If we don't think it's necessary to protect users from that kind of mistake, then that's fine. My concern is that people keep saying it's not a problem because bracketed paste (which doesn't work in Windows shells) or because of the warning (which the attacker can turn off). That gives me the impression that the risk isn't understood. And if you don't understand the risk, you can't make an informed decision to ignore it. |
Major changes. Looks like Dustin's got a stake in this too.
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
Alright, I think I understand it now. I recently had a discussion with some of our security folks who are working on closing another pretty big attack vector which relies on social engineering: websites putting content on the clipboard, and then telling users to press "Win-R Ctrl-V Enter" to, idk, win the lottery or fix their Windows Defender Alerts or what-have-you. Clipboard control is something of a pandora's box we've opened, eh. Thanks for being patient with us. |
…resw Co-authored-by: Dustin L. Howett <[email protected]>
ControlCore
toTerminalPage
.This requires adding a bunch of event types and logic.
This is technically not needed anymore after changing the
direction of this PR, but I kept it because it's better.
WarnAboutMultiLinePaste
enum to differentiate between"paste without warning always/never/if-bracketed-paste-disabled".
Closes #13014
Closes microsoft/edit#279
Validation Steps Performed