Skip to content

Check unsafe method calls made through bind() - #296

Open
owevertonguedes wants to merge 1 commit into
mozilla:mainfrom
owevertonguedes:fix/115-bound-method-calls
Open

Check unsafe method calls made through bind()#296
owevertonguedes wants to merge 1 commit into
mozilla:mainfrom
owevertonguedes:fix/115-bound-method-calls

Conversation

@owevertonguedes

Copy link
Copy Markdown

Fixes #115

The bug

checkCallExpression() treated a CallExpression callee as always fine, so a sink reached through Function.prototype.bind was never checked:

document.body.insertAdjacentHTML.bind(document.body)("afterend", evil); // silently allowed

The fix

Following the approach suggested in the issue, a bound call is rewritten into the direct call it actually performs, and that mock node is checked instead, the same way the SequenceExpression case already builds one.

Two details beyond the three examples in the issue:

  • Partial application. bind() prepends its own arguments, all but thisArg, to the ones of the eventual call, so insertAdjacentHTML.bind(el, "afterend")(evil) puts evil at argument 1 and is now reported. Without merging the arguments, that shape would be a silent bypass.
  • Only nameable callees. The rewrite happens when the bound function is an identifier, a member expression or another bind(). Recursing into anything else made the rule newly report benign code such as (class {}).bind(null)() as an unsupported callee, and made (foo.insertAdjacentHTML`x${evil}`).bind(null)() report twice. Both now behave exactly as they did before this patch.

A spread in the thisArg position hides how many arguments bind() prepends, so those calls are left alone rather than guessed at, in the spirit of #214.

Verification

npm test (193 passing, up from 180) and npm run lint are clean.

The property I was really after is that a bound call gets the same verdict as its direct equivalent, so I checked it by brute force rather than by eye: 972 pairs, generated by splitting every argument list of literals, identifiers, escaped templates and spreads across every possible bind()/call boundary, comparing insertAdjacentHTML(ARGS) against every insertAdjacentHTML.bind(el, PREFIX)(REST) that performs it. Zero divergences.

I also diffed the patched rule against the current main rule on exotic callee shapes (class and object expressions, yield, tagged templates, sequence expressions, chained binds, optional chaining, and the TypeScript and Flow parser configurations used in the test file) and confirmed no input changed verdict except the bound calls this patch is meant to catch.

Nine tests were added. The five invalid ones all fail against main; on the valid side, getInserter()('afterend', evil) and the spread thisArg case fail if the corresponding guard is removed. The remaining valid cases are the ones listed in the issue.

The method rule looked at the callee of a CallExpression and treated
another CallExpression as fine, so a sink reached through
Function.prototype.bind was never checked:

    document.body.insertAdjacentHTML.bind(document.body)("afterend", evil)

Rewrite such a call into the direct call it performs and check that
one instead, the same way the SequenceExpression case builds a mock
node. Arguments given to bind() are prepended to the ones of the
eventual call, so partial application is checked too.

The rewrite only happens when the bound function is an identifier, a
member expression or another bind(), so that binding an expression the
rule cannot name stays as quiet as calling it directly would be. A
spread in the thisArg position hides how many arguments bind()
prepends, so those calls are left alone.

Fixes mozilla#115
@owevertonguedes

Copy link
Copy Markdown
Author

@mozfreddyb this follows the approach you sketched on the issue: the bound call is rewritten into the direct call it performs, and that mock node is checked instead.

CI has not run yet, it is sitting on workflow approval since this is my first contribution here. Whenever you get a chance to release it.

Two parts go slightly beyond the three cases you listed, and both are separable if you would rather keep the change minimal: merging the arguments that bind() supplies, and limiting the rewrite to callees the rule can name. I wrote down why each is there in the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

treat foo.bind(something).bar() similar to foo.bar()

1 participant