Repository navigation
fix: guard AST node property lookups against prototype pollution (fixes #3414) - #3470
Open
Ayoub-glitsh wants to merge 2 commits into
Open
Ayoub-glitsh wants to merge 2 commits into
Ayoub-glitsh wants to merge 2 commits into
Conversation
Unsafe property accesses like if (node.block) or ode.type === 'X' traverse the prototype chain, allowing an attacker who has already achieved prototype pollution to escalate to RCE by injecting values such as Object.prototype.block, .type, .val, .code, .line, etc. Fix all vulnerable lookup sites in pug-walk, pug-code-gen, pug-linker, and pug-filters by using Object.prototype.hasOwnProperty.call() before reading any property that influences control flow or code generation. Fixes pugjs#3414
|
This PR will not result in a new version of the following packages as there are no user facing changes:
Add changes to trigger a release
|
Author
|
@RollingVersions bump pug-walksecurity
pug-code-gensecurity
pug-linkersecurity
pug-filterssecurity
|
- Add hasOwnProperty guard for mixin.code in parser (fixes issue pugjs#3414 path C) - Add hasOwnProperty guard for valueResponse.val in lexer (fixes issue pugjs#3414 path D) - Add comprehensive security regression tests for all four vulnerability paths - Ensure normal functionality is preserved for legitimate use cases This completes the security fix addressing all four exploitation paths: 1. code.block in code generation (already fixed in base branch) 2. tag.code in code generation (already fixed in base branch) 3. mixin.code in parser (fixed in this commit) 4. valueResponse.val in lexer (fixed in this commit) The fix prevents prototype pollution escalation to RCE by using Object.prototype.hasOwnProperty.call() to ensure only own properties are used for control flow and code generation decisions.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Several packages accessed properties on AST node objects without verifying they were own properties. In JavaScript,
obj.proptraverses the prototype chain — so if an attacker has already achieved prototype pollution elsewhere in the app, they can inject values likeObject.prototype.block,.type,.val,.code,.lineand escalate to RCE during template compilation.The three PoC gadgets from #3414 all exploit this pattern:
Object.prototype.blockwith a maliciousTextnode containing arbitrary JS inlineObject.prototype.codewith a maliciousvalObject.prototype.valinjecting code into attribute generationFix
Guard all property accesses that influence control flow or code generation with
Object.prototype.hasOwnProperty.call(node, 'prop')before reading the value.Files changed
pug-walk/index.js— the most critical, used by all other packages.switch (ast.type)and allif (ast.block/alternate/consequent)checks are now guarded.pug-code-gen/index.js—node.line,node.block,tag.code,node.isInline,node.val,node.typechecks guarded.pug-linker/index.js—node.type,node.call,node.mode,node.block,node.ignore,node.parents,node.textOnlychecks guarded.pug-filters/lib/handle-filters.js—node.type,node.filters,node.blockchecks guarded.Tests
648/650 tests pass. The 2 failures are pre-existing and unrelated — they test for old Node.js error message wording (
Cannot read propertyvs the newerCannot read properties) introduced in Node 16+.