Update regex to prevent ReDOS and add scanner #777

tjenkinson · 2024-04-13T09:37:00Z

Hey I noticed some regex in this tool and thought I'd scan it with my tool to see if there were any potential issues and it found some. So this PR fixes the vulnerable patterns and adds a check that runs on CI using eslint-plugin-redos-detector.

I initially thought on opening this as a security ticket, but given the main use case is running executables anyway and not random code it doesn't really feel like a vulnerability. Still makes sense to address though?

Tests pass
Appropriate changes to README are included in PR
Types updated

It's git ignored

tjenkinson · 2024-04-13T09:38:03Z

src/cli.ts

+  const jsCodeBlock = /^(```+|~~~+)(?:js|javascript)$/
+  const shCodeBlock = /^(```+|~~~+)(?:sh|bash)$/


thought it made sense to use a none capturing group here given we aren't using the value

tjenkinson · 2024-04-13T09:47:08Z

src/cli.ts

@@ -192,9 +192,9 @@ function transformMarkdown(buf: Buffer) {
  let state = 'root'
  let codeBlockEnd = ''
  let prevLineIsEmpty = true
-  const jsCodeBlock = /^(```+|~~~+)(js|javascript)$/
-  const shCodeBlock = /^(```+|~~~+)(sh|bash)$/
-  const otherCodeBlock = /^(```+|~~~+)(.*)$/


The problem here is that . won't match \n and a few other characters without the s flag.

Try running

/^(```+|~~~+)(.*)$/.test('```' + '`'.repeat(1000000) + '\n')

in the console and it will lock up.

So the solution could be add the s flag, but actually given we weren't using the last group I think this makes more sense

tjenkinson · 2024-04-13T09:49:47Z

src/deps.ts

@@ -90,9 +90,8 @@ const builtins = new Set([
  'zlib',
 ])

-const nameRe =
-  /^(?<name>(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*)\/?.*$/i


my tool doesn't support named groups (yet), so I switched it to a standard one, and then the issue was the .* at the end overlapping with the rest.

The simplest solution looks like removing that and the end anchor.

tjenkinson · 2024-04-13T09:51:41Z

src/deps.ts

@@ -90,9 +90,8 @@ const builtins = new Set([
  'zlib',
 ])

-const nameRe =
-  /^(?<name>(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*)\/?.*$/i
-const versionRe = /^@(?<version>[~^]?(v?[\dx*]+([-.][\d*a-z-]+)*))/i


The fix here makes the regex less precise, but I don't think that's a problem? I think it should be good enough to catch false positives?

The issue was in the second part.

Alternatively maybe we should use https://github.com/npm/node-semver to check if it's a valid version, but that would mean adding a dependency and not sure if it would be worth it?

tjenkinson · 2024-04-13T09:53:24Z

test/fixtures/js-project/package-lock.json

This is in the .gitignore but looks like it was accidentally checked in

antongolub · 2024-04-13T10:15:12Z

Not sure about the profit. I mean, the highlighted case is hypothetically possible, but what about the practice? For example, semver-regex should provide redos detection, it may accept an uncontrollable input, etc. But zx literally invokes bash commands, so regex performance is much less of a problem than $`rm -rf *` :-)

tjenkinson · 2024-04-13T10:20:50Z

Not sure about the profit. I mean, the highlighted case is hypothetically possible, but what about practice? For example, semver-regex should provide redos detection, it may accept an uncontrollable input, etc. But zx literally invokes bash commands, so regex performance is much less of a problem than $`rm -rf *` :-)

yeh, agree, like I mentioned above it's not really a vulnerability here given the purpose of the project anyway. But I think a pattern that's not susceptible to a ReDOS and therefore has slightly better performance is still better than one that does 🤷.

This also simplifies the patterns a bit

zloirock · 2024-04-13T10:24:15Z

.eslintrc

+  "parser": "@typescript-eslint/parser",
+  "parserOptions": { "ecmaVersion": "latest" },
+  "rules": {
+    "redos-detector/no-unsafe-regex": ["error"],


What are the advantages of this plugin compared, for example, with eslint-plugin-redos?

They take different approaches. With mine the goal was to never report a pattern as safe when it's not, so it may have false positives

Not sure if they have the same goal. With a fuzzing approach I think there is the potential to miss things but looks like sometimes it also uses an automation which maybe can't miss stuff

for what it's worth, looks like that one doesn't handle back references as well

/(a)\1*$/

is a false positive with that one

@tjenkinson I'm the author of eslint-plugin-redos, and, I found this comment luckily ;)

/(a)\1*$/ seems a vulnerable regex to me and my detector also reports it as vulnerable, so what is it problem?
(If you feel that discussion here is not beneficial, please email to me or open a new issue to makenowjust-labs/recheck instead.)

Hey @makenowjust !

/(a)\1*$/ is essentially /aa*$/ which is not vulnerable?

Ah I see maybe it is without the start anchor 🤔

Yes. /(a)\1*$/ is essentially /^.*aa*$/s, so it is vulnerable.

tjenkinson · 2024-04-13T17:39:40Z

This run also failed but the job passed overall 🤔

https://github.com/google/zx/actions/runs/8672721202/job/23788065046

# tests 78
# pass 73
# fail 5

tjenkinson · 2024-04-13T17:42:58Z

Might need to handle the unhandled rejection or add the cli flag for older node

https://nodejs.org/api/process.html#process_event_unhandledrejection

antonmedv · 2024-04-22T07:13:25Z

I don’t see why this is needed at this point. We are running bash scripts provided by user. So the code we run should already be trusted.

tjenkinson added 3 commits April 12, 2024 22:45

Add redos checker and run on CI

25fa570

Update regex's to prevene ReDOS

55f30d1

Remove js fixture package-lock.json

3c62c05

It's git ignored

tjenkinson commented Apr 13, 2024

View reviewed changes

test/fixtures/js-project/package-lock.json Outdated

Copy link

Author

tjenkinson Apr 13, 2024

Choose a reason for hiding this comment

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

This is in the .gitignore but looks like it was accidentally checked in

Add another markdown test when no language

3888516

tjenkinson marked this pull request as ready for review April 13, 2024 10:03

zloirock reviewed Apr 13, 2024

View reviewed changes

Fix version regex

fa5a4bc

antonmedv closed this Apr 22, 2024

zloirock mentioned this pull request Jul 7, 2024

Add eslint-plugin-redos-detector dustinspecker/awesome-eslint#233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update regex to prevent ReDOS and add scanner #777

Update regex to prevent ReDOS and add scanner #777

tjenkinson commented Apr 13, 2024 •

edited

Loading

tjenkinson Apr 13, 2024

tjenkinson Apr 13, 2024

tjenkinson Apr 13, 2024

tjenkinson Apr 13, 2024 •

edited

Loading

tjenkinson Apr 13, 2024

antongolub commented Apr 13, 2024 •

edited

Loading

tjenkinson commented Apr 13, 2024

zloirock Apr 13, 2024

tjenkinson Apr 13, 2024

tjenkinson Apr 13, 2024

makenowjust Apr 22, 2024

tjenkinson Apr 22, 2024

tjenkinson Apr 22, 2024

makenowjust Apr 22, 2024

tjenkinson commented Apr 13, 2024

tjenkinson commented Apr 13, 2024

antonmedv commented Apr 22, 2024

		const jsCodeBlock = /^(```+\|~~~+)(?:js\|javascript)$/
		const shCodeBlock = /^(```+\|~~~+)(?:sh\|bash)$/

Update regex to prevent ReDOS and add scanner #777

Update regex to prevent ReDOS and add scanner #777

Conversation

tjenkinson commented Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjenkinson Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antongolub commented Apr 13, 2024 • edited Loading

tjenkinson commented Apr 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjenkinson commented Apr 13, 2024

tjenkinson commented Apr 13, 2024

antonmedv commented Apr 22, 2024

tjenkinson commented Apr 13, 2024 •

edited

Loading

tjenkinson Apr 13, 2024 •

edited

Loading

antongolub commented Apr 13, 2024 •

edited

Loading