A ReDoS vulnerability can be exploited after version 0.2.0

[SugarP1g]

There is a weak expression can be exploited to launch a DOS attack.

Execution stack is as follow:

POC:

Run result:

[mxro]

Thank you for raising this issue. Have added this to the Readme for the project until it is resolved.

Any ideas for resolving thie welcome.

I assume this can still be caught when setting an executor and max CPU time?

[SugarP1g]

Thank you for raising this issue. Have added this to the Readme for the project until it is resolved.

Any ideas for resolving thie welcome.

I assume this can still be caught when setting an executor and max CPU time?

Yes.

The cause of the problem is the defect of the regular engine.

This link can be referenced: https://checkmarx.com/wp-content/uploads/2015/03/ReDoS-Attacks.pdf

[junweili]

Hi @mxro , is there any plan to fix this in near term? It is listed under https://nvd.nist.gov/vuln/detail/CVE-2021-40660#match-8086489

Thank you

[mxro]

Thanks for reaching out!

I guess the way forward here would be to replace the RegEx with more deterministic logic?

[junweili]

Yes, such complex RegEx is vulnerable. Out of curiosity about the reason of injecting interruption call by using POISON_PILLS, is this for the purpose of terminating script execution to cap cpu time?

[mxro]

Yes, it's injecting a bit of code that triggers the test for how much CPU time is used.

However, there is an additional fallback the sandbox provides, in that it can do a hard shutdown of the thread as well. But using the injected statements should be smoother.

[asilism]

@mxro
How about remove all comment lines before run jsSanitizer?
I have commented on a similar issue in the past. ( #108 )

I didn't know the exact reason such as ReDoS, but I knew that "POISONPIL + Comments" was the cause.
So, I am using the logic to remove all comments in the code before eval(), and there has been no slowdown since then.

I don't know this approach is a real solution to this vulnerability, but I'm leaving a post in case it helps.

[mxro]

@asilism Thank you for the idea!

So that would mean moving

https://github.com/javadelight/delight-nashorn-sandbox/blob/master/src/main/java/delight/nashornsandbox/internal/JsSanitizer.java#L201

to

delight-nashorn-sandbox/src/main/java/delight/nashornsandbox/internal/JsSanitizer.java

Line 279 in 23ba3d7

final String beautifiedJs = beautifyJs(js);

?

[asilism]

@mxro
Yes, before call beautifyJs.
but 2 Things must be clear. I think.

1 ) Does RemoveComments function completely remove all of comments line ?
2 ) it is necessary to confirm whether there is a need to change POISON_PIL regular expression.

[huzongxin1994]

@mxro I found that this issue was not resolved in version 0.2.4.Excuse me, when will the problem be solved? If multiple lines of non-function code are transferred, \n is not added to the end of the line after beautifyJs processing.

[mxro]

1 ) Does RemoveComments function completely remove all of comments line ?

I think it should!

2 ) it is necessary to confirm whether there is a need to change POISON_PIL regular expression.

Pretty sure the RegEx would need to be changed.

[mxro]

@huzongxin1994 Yes, this issue is still open.

Looking for good ideas to fix the Regex or avoid the problem altogether!

[huzongxin1994]

Modify the regular expression or split the regular logic, I think it's a better solution.Can you tell me more about what this regularity \n(?![\W]*(//.+[\W]+)else) means, and then we're trying to deal with it together?The \n(?*else) regular expression triggers a backtracking trap.

[mxro]

@huzongxin1994 This link @SugarP1g shared earlier describes the backtracking vulnerability: https://checkmarx.com/wp-content/uploads/2015/03/ReDoS-Attacks.pdf

By avoiding it altogether, I was thinking of if there could be a solution that doesn't use a regular expression at all!

[lgtest1789]

@huzongxin1994 This link @SugarP1g shared earlier describes the backtracking vulnerability: https://checkmarx.com/wp-content/uploads/2015/03/ReDoS-Attacks.pdf

By avoiding it altogether, I was thinking of if there could be a solution that doesn't use a regular expression at all!

When can this question be updated? Thank you.

[MrLi2018]

@mxro Hello, sorry to interrupt, I think if this problem can not change the regular expression in the short term, removing the comment may be a good way, I am removing the comment, dos attack no longer exists.

[MrLi2018]

You can also set timeout limits for regular expressions and ask users to modify the corresponding JS code.

[MrLi2018]

Like this:
static class InterruptibleCharSequence implements CharSequence {
CharSequence inner = "";

    public InterruptibleCharSequence(CharSequence inner) {
        super();
        this.inner = inner;
    }

    @Override
    public char charAt(int index) {
        if (Thread.currentThread().isInterrupted()) {
            throw new ScriptCPUAbuseException("compile timeout Interrupted!",true,null);
        }
        return inner.charAt(index);
    }

    @Override
    public int length() {
        return inner.length();
    }

    @Override
    public CharSequence subSequence(int start, int end) {
        return new InterruptibleCharSequence(inner.subSequence(start, end));
    }

    @Override
    public String toString() {
        return inner.toString();
    }
}

[mxro]

@MrLi2018 Thank you for your suggestion. That looks promising. Could you provide a few more details? Where would we use the new class?

[MrLi2018]

@mxro Hello, I'm sorry I won't submit open source code on GitHub. I can only communicate in this way.The above class can be used to break the infinite backtracking of regular expressions, and I've written a simple example you can refer to, and I think it can solve this problem.

    private static ThreadLocal<Integer> matchCount = ThreadLocal.withInitial(() -> 0);

	String injectInterruptionCalls(final String str) {
		String perform = RemoveComments.perform(str);
		try {
			String current = perform;
			for (final PoisonPil pp : POISON_PILLS) {
				final StringBuffer sb = new StringBuffer();
				final Matcher matcher = pp.pattern.matcher(new SecuInterruptibleCharSequence(current));
				while (matcher.find()) {
					matcher.appendReplacement(sb, ("$1" + pp.replacement));
				}
				matcher.appendTail(sb);
				current = sb.toString();
			}
			return current;
		} catch (Exception exp) {
			return perform;
		}
	}

	static class SecuInterruptibleCharSequence implements CharSequence {
		CharSequence inner = "";

		public SecuInterruptibleCharSequence(CharSequence inner) {
			super();
			this.inner = inner;
		}

		@Override
		public char charAt(int index) {
			matchCount.set(matchCount.get()+1);
			if (matchCount.get()>10000){
				Thread thread = Thread.currentThread();
				thread.interrupt();
			}
			if (Thread.currentThread().isInterrupted()) {
				throw new ScriptCPUAbuseException("compile timeout Interrupted!", true, null);
			}
			return inner.charAt(index);
		}

		@Override
		public int length() {
			return inner.length();
		}

		@Override
		public CharSequence subSequence(int start, int end) {
			return new SecuInterruptibleCharSequence(inner.subSequence(start, end));
		}

		@Override
		public String toString() {
			return inner.toString();
		}
	}

Or you can use a separate thread, but I think you can use the sandbox to set the thread pool at build time.As follows:

    private static ExecutorService executorService = Executors.newSingleThreadExecutor();

	String injectInterruptionCalls(final String str) {
		AtomicReference<String> current = new AtomicReference<>(RemoveComments.perform(str));
		Future<String> submit = executorService.submit(() -> {
			for (final PoisonPil pp : POISON_PILLS) {
				final StringBuffer sb = new StringBuffer();
				final Matcher matcher = pp.pattern.matcher(new SecuInterruptibleCharSequence(current));
				while (matcher.find()) {
					matcher.appendReplacement(sb, ("$1" + pp.replacement));
				}
				matcher.appendTail(sb);
				current.set(sb.toString());
			}
			return current.get();
		});
		String result = null;
		try {
			result = submit.get(10, TimeUnit.MILLISECONDS);
		} catch (Exception exp) {
			submit.cancel(true);
			result = current.get();
		}
		return result;
	}

	static class SecuInterruptibleCharSequence implements CharSequence {
		CharSequence inner = "";

		public SecuInterruptibleCharSequence(CharSequence inner) {
			super();
			this.inner = inner;
		}

		@Override
		public char charAt(int index) {
		
			if (Thread.currentThread().isInterrupted()) {
				throw new ScriptCPUAbuseException("compile timeout Interrupted!", true, null);
			}
			return inner.charAt(index);
		}

		@Override
		public int length() {
			return inner.length();
		}

		@Override
		public CharSequence subSequence(int start, int end) {
			return new SecuInterruptibleCharSequence(inner.subSequence(start, end));
		}

		@Override
		public String toString() {
			return inner.toString();
		}
	}

[mxro]

Thank you @MrLi2018 for the further clarifications!

I've included this in PR #139. I would be very grateful for one or more reviews confirming this addresses the identified security issues. Thanks for @SugarP1g for initially identifying this - I've tried to model the unit test as closely as possible to what you have submitted.

[MrLi2018]

Thank you for your contribution to the sandbox, but another way is to use Hyperscan to parse regulars, because it's not a backtracking mechanism, but it seems that you need to modify the regulars, and if you want to remove the regulars, it seems that unless we can get involved in the JS loop itself. @mxro

[mxro]

Fix released to Maven central with version 0.3.1.

[gregorywaynepower]

Fix released to Maven central with version 0.3.1.

@mxro Does this mean that there is a fix to CVE-2021-40660?

[mxro]

@gregorywaynepower Yes, absolutely!

Not sure how I can get the CVE closed though?!

[gregorywaynepower]

@gregorywaynepower Yes, absolutely!

Not sure how I can get the CVE closed though?!

I can do some digging on how to update an existing GitHub Security Advisory Contribution.

Edit: Looks like you can make a pull request to this file and add a fixed tag using the Open Source Vulnerability format.

[mxro]

Thanks heaps for the information ❤ - submitted a PR 👆

[mxro]

Please note this has been merged in the advisory database - thanks for your help @gregorywaynepower github/advisory-database#3256

[gregorywaynepower]

Fantastic news @mxro ! Thanks for taking this on.