Backport security fixes for #1736 #1751

bitwiseman · 2021-05-19T08:22:25Z

I would not file a PR for this except that the security advisory has already been filed.
This PR addresses the vulnerability in compat mode and documents the lack of vulnerability in strict mode.
@nknapp

Generally we like to see pull requests that

Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
Maintain the existing code style
Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
Have good commit messages
Have tests
Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.

…1736)

bitwiseman · 2021-05-19T08:25:26Z

spec/security.js

+            expectTemplate('{{constructor.name}}')
+              .withCompileOptions(compileOptions)
+              .withInput({})
+              .toThrow(TypeError);


This was already the case in 3.x before this change and remains after this change currently. strict mode resulted in a TypeError when accessing constructor, __proto__, etc. We can fix this but it would require more changes and would not improve the security.

bitwiseman · 2021-05-19T18:09:56Z

lib/handlebars/runtime.js

    },
    lookup: function(depths, name) {
      const len = depths.length;
      for (let i = 0; i < len; i++) {
-        if (depths[i] && depths[i][name] != null) {
+        let result = depths[i] && container.lookupProperty(depths[i], name);


This covers compat mode.

bitwiseman · 2021-05-19T18:15:52Z

lib/handlebars/runtime.js

@@ -69,12 +69,28 @@ export function template(templateSpec, env) {
      if (!(name in obj)) {
        throw new Exception('"' + name + '" not defined in ' + obj);
      }
-      return obj[name];
+      return container.lookupProperty(obj, name);


It appears strict mode is already covered by the check in javascript-compiler.js:

handlebars.js/lib/handlebars/compiler/javascript-compiler.js

Lines 16 to 19 in 16bd606

if (dangerousPropertyRegex.test(name)) {

const isOwnProperty = [ this.aliasable('Object.prototype.hasOwnProperty'), '.call(', parent, ',', JSON.stringify(name), ')'];

return ['(', isOwnProperty, '?', _actualLookup(), ' : undefined)'];

}

However, the reason why this vulnerability appeared in 4.x was because that check was removed and this change was present. Better to make this change to make it completely clear.

batmat · 2021-05-25T10:35:29Z

@nknapp hello! Any chance you would be able to look into this some day soon? We are ready to help as need be, please let us know :-).
Thanks a lot for your hard work!

bitwiseman · 2021-05-25T18:51:18Z

@ErisDS @Afanyiyu @kpdecker @lawnsea @wycats
Sorry for pinging you all but this is a security issue that it would be great to get backported sooner rather than later.

bitwiseman · 2021-11-29T21:56:50Z

@jaylinski
Hey, thanks for the label update. If there is anything I can do to make this PR merge sooner, please just say so.
@car-roll @batmat @jtnord may also want to be updated on this.

jaylinski · 2021-12-30T00:29:56Z

Memo to myself:

Think about back-porting GitHub CI code for tests
Don't use npm lock-file version v2, keep it to v1
Test fixes against exploits (https://snyk.io/vuln/npm:handlebars)
Close RCE possible in compat and strict mode #1736 when this PR is merged and tagged

nknapp · 2022-01-07T16:39:14Z

@bitwiseman I think the compat-mode (f058970) part is worth backporting, but I wouldn't touch the part that is no real security issue in version 3.

Changes in version 3 are only done on a strict "need to be there" basis, as far as I am concerned...

nknapp · 2022-01-07T16:54:37Z

can you modify your PR, so that only f058970 is backported?

windusayles · 2022-08-03T16:31:25Z

@bitwiseman @nknapp do we need the modification you mentioned, f058970, in order to merge this fix?

wycats · 2022-08-18T18:51:33Z

@windusayles yeah 👍

Can you update the PR to limit it to just f058970?

jaylinski · 2023-09-21T20:46:55Z

Closing since v3.0.8 only has a few thousand downloads a week. So most people moved on to the latest (and more secure) version.

https://www.npmjs.com/package/handlebars?activeTab=versions

Also: v3.x is now marked as EOL in our security policy (35f0018).

bitwiseman and others added 2 commits May 18, 2021 14:51

fix: check dangerous property access in strict-mode (handlebars-lang#…

e61e016

…1736)

fix: escape property names in compat mode (handlebars-lang#1736)

3315df5

bitwiseman commented May 19, 2021

View reviewed changes

Synchronize pagckage.json and package-lock.json

5519e05

bitwiseman mentioned this pull request May 19, 2021

RCE possible in compat and strict mode #1736

Closed

bitwiseman commented May 19, 2021

View reviewed changes

jaylinski self-assigned this Nov 23, 2021

jaylinski added the bug label Nov 23, 2021

jaylinski closed this Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport security fixes for #1736 #1751

Backport security fixes for #1736 #1751

bitwiseman commented May 19, 2021 •

edited

bitwiseman May 19, 2021 •

edited

bitwiseman May 19, 2021

bitwiseman May 19, 2021

batmat commented May 25, 2021 •

edited

bitwiseman commented May 25, 2021 •

edited

bitwiseman commented Nov 29, 2021

jaylinski commented Dec 30, 2021 •

edited

nknapp commented Jan 7, 2022

nknapp commented Jan 7, 2022

windusayles commented Aug 3, 2022

wycats commented Aug 18, 2022

jaylinski commented Sep 21, 2023

	if (dangerousPropertyRegex.test(name)) {
	const isOwnProperty = [ this.aliasable('Object.prototype.hasOwnProperty'), '.call(', parent, ',', JSON.stringify(name), ')'];
	return ['(', isOwnProperty, '?', _actualLookup(), ' : undefined)'];
	}

Backport security fixes for #1736 #1751

Backport security fixes for #1736 #1751

Conversation

bitwiseman commented May 19, 2021 • edited

bitwiseman May 19, 2021 • edited

Choose a reason for hiding this comment

bitwiseman May 19, 2021

Choose a reason for hiding this comment

bitwiseman May 19, 2021

Choose a reason for hiding this comment

batmat commented May 25, 2021 • edited

bitwiseman commented May 25, 2021 • edited

bitwiseman commented Nov 29, 2021

jaylinski commented Dec 30, 2021 • edited

nknapp commented Jan 7, 2022

nknapp commented Jan 7, 2022

windusayles commented Aug 3, 2022

wycats commented Aug 18, 2022

jaylinski commented Sep 21, 2023

bitwiseman commented May 19, 2021 •

edited

bitwiseman May 19, 2021 •

edited

batmat commented May 25, 2021 •

edited

bitwiseman commented May 25, 2021 •

edited

jaylinski commented Dec 30, 2021 •

edited