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.

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

util: expose stripVTControlCharacters() #40214

Merged
merged 2 commits into from
Oct 2, 2021
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 25, 2021

This PR exposes the existing stripVTControlCharacters() method with docs and some additional input validation. It also improves the regex used by the function.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 25, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cjihrig cjihrig added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Oct 2, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 2, 2021

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2021
@nodejs nodejs deleted a comment from nodejs-github-bot Oct 2, 2021
@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 2, 2021
This commit exposes the existing stripVTControlCharacters()
method with docs and some additional input validation.

PR-URL: nodejs#40214
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#40214
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit 606bb52 into nodejs:master Oct 2, 2021
@cjihrig cjihrig deleted the strip-ansi branch October 2, 2021 02:59
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 2, 2021

Landed in 396b14b and 606bb52.

targos pushed a commit that referenced this pull request Oct 4, 2021
This commit exposes the existing stripVTControlCharacters()
method with docs and some additional input validation.

PR-URL: #40214
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2021
PR-URL: #40214
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@thoger
Copy link

thoger commented Oct 13, 2021

The regex improvement change (606bb52 / 66d3101) is actually a port of the ansi-regex fix for the CVE-2021-3807 regular expression DoS (ReDoS) issue. References for the fix in ansi-regex:

chalk/ansi-regex#37
chalk/ansi-regex@8d1d7cd

Are there any plans to apply this fix to 14 and 12 as well?

@mcollina
Copy link
Member

This regexp was not exposed prior to this PR and not exploitable.

@thoger
Copy link

thoger commented Oct 13, 2021

While stripVTControlCharacters() wasn't previously exposed directly (not counting uses with --expose-internals), it is used by getStringWidth(), which is used by other modules - readline, cli_table, source_map. I've not reviewed all these cases, only managed to trigger the regex via readline's rl.write(), just make it write the attack_str as used in the original reproducer: chalk/ansi-regex#37 (comment)

I'm not aware of any real world use case practically exploitable via this issue.

@thoger
Copy link

thoger commented Oct 13, 2021

readline reproducer:

const readline = require('readline');
const rl = readline.createInterface({
	input: process.stdin,
	output: process.stdout,
});

for(var i = 1; i <= 5; i++) {
	var time = Date.now();
	var attack_str = "\u001B["+";".repeat(i*10000);
	rl.write(attack_str);
	var time_cost = Date.now() - time;
	console.log("_attack_str.length: " + attack_str.length + ", time: " + time_cost + " ms")
}

rl.close()

@mcollina
Copy link
Member

Readline is a developer utility. This is not going to cause a DoS attack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants