fix: guard AST node property lookups against prototype pollution (fixes #3414) - #3470
Open
Ayoub-glitsh wants to merge 1 commit into
Open
fix: guard AST node property lookups against prototype pollution (fixes #3414)#3470Ayoub-glitsh wants to merge 1 commit into
Ayoub-glitsh wants to merge 1 commit 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:
|
Author
|
@RollingVersions bump pug-walksecurity
pug-code-gensecurity
pug-linkersecurity
pug-filterssecurity
|
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+.