Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix XSS vulnerability in expression parser. #3019

Merged
merged 3 commits into from Dec 21, 2020
Merged

Fix XSS vulnerability in expression parser. #3019

merged 3 commits into from Dec 21, 2020

Conversation

jheer
Copy link
Member

@jheer jheer commented Dec 21, 2020

vega-expression

  • Fix XSS vulnerability in expression parser and code generator.

Fix #3018.

domoritz
domoritz previously approved these changes Dec 21, 2020
@cgvwzq
Copy link

cgvwzq commented Dec 21, 2020

Hi, not sure if a patch at that level will be enough:

{
  "data": [
    {
      "name": "data",
      "values": [{}],
      "transform": [
        {
          "type": "filter",
          "test": "0/ /1/-'\\\n,alert(1))))//'"
        }
      ]
    }
  ]
}

In this case it builds a wrong AST and the code generation removes the whitespace, inserting a comment.

@domoritz domoritz requested review from domoritz and removed request for domoritz December 21, 2020 12:28
@domoritz domoritz dismissed their stale review December 21, 2020 12:29

See comment

@domoritz domoritz removed their request for review December 21, 2020 12:29
@jheer jheer mentioned this pull request Dec 21, 2020
@jheer
Copy link
Member Author

jheer commented Dec 21, 2020

Thanks @cgvwzq, I've updated the PR to safeguard the code generation as well.

@cgvwzq
Copy link

cgvwzq commented Dec 21, 2020

Glad to help :)

However, I'm not sure this is the right way to fix the overall threat, as this patch only prevents the specific payload I used. My experience with JS sandboxing based on parsing/rewriting is that it is extremely hard to get right, and if someone looks into it more security issues will certainly emerge. Just looking at some other features, signals and events seem another source of similar problems.

I do not want to turn this into a cat-and-mouse game (as fun as it could be :P), so I would rather suggest to try to rethink the overall eval business. If you need it, I would at least try to make sure that all generated code runs in a sandboxed iframe in isolation (you can rely on postMessage to communicate all data (and events?).

@nename0
Copy link

nename0 commented Dec 21, 2020

Is there a specify reason that you have to use eval?

If you already have an AST couldn't you also just evaluate that AST?
Something along those lines:

{
  BinaryExpression: n => {
    switch(n.operator) {
      case '*': return visit(n.left) * visit(n.right);
      case '+': return visit(n.left) + visit(n.right);
      case '==': return visit(n.left) == visit(n.right);
      ...
     }
  },

  UnaryExpression: n =>{
    switch(n.operator) {
      case '!': return !visit(n.argument);
      case '-': return -visit(n.argument);
      ...
    } 
  },
  ...
}

That would at least prevent any mismatch between interpretation by your parser and the evaluation.

@domoritz
Copy link
Member

@jheer actually added an interpreter mode to Vega but it's slightly slower so it's not the default: https://github.com/vega/vega/tree/master/packages/vega-interpreter.

@jheer jheer merged commit eba3eba into master Dec 21, 2020
@jheer jheer deleted the jh/xss branch December 21, 2020 20:22
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.

XSS in transform filter
4 participants