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

Potential Security Vulnerability #448

Open
jcubic opened this issue Dec 24, 2021 · 9 comments
Open

Potential Security Vulnerability #448

jcubic opened this issue Dec 24, 2021 · 9 comments

Comments

@jcubic
Copy link

jcubic commented Dec 24, 2021

Just found that this expression is valid according to escodegen:

!{
    "type": "TemplateLiteral",
    "expressions": [],
    "quasis": [
        {
            "type": "TemplateElement",
            "value": {
                "raw": "hello ${prompt('I CAN DO XSS')}!"
            }
        }
    ]
}

The string is not parsed and it's processed into output code as is. I think that it should be an error. This can lead into injection of JavaScript code and escaping from sandbox depending on how the application is using this library.

Just found it in my project https://gaiman.js.org/ just type code:

echo "hello ${prompt('I CAN DO XSS')}!"

I think that I will report this to NPM or GitHub.

EDIT: I think that only maintainer of the package can report Security Advisory.

@jsoref
Copy link

jsoref commented Jan 14, 2022

GitHub only accept[s] security advisories from the repo maintainers themselves, but you can file for a CVE which [they] will pick up on after publication.
https://www.cve.org/ResourcesSupport/ReportRequest

@jsoref
Copy link

jsoref commented Jan 14, 2022

That said, if this is a documented feature, please think through what you're proposing.

There's a library we use which supports a thing like this and it's documented as such, we're now stuck explaining that it's totally reasonable for a library to have a function that allows executing code and it's up to consumers to be intelligent about whether or not they allow users (or attackers) to send content to such functions.

@jcubic
Copy link
Author

jcubic commented Jan 14, 2022

@jsoref This is not a documentation issue. This can have potential security implications for people that use the library. Since if they don't know about this it can lead to vulnerability.

And as for AST, this is broken AST, you should not be allowed to put JavaScript code as a string in any way. raw is one place where this is problematic but the library should handle this.

In my library, I've had a similar issue, and I've marked the version as vulnerable because the user could not know that he need to sanitize the input before processing it by the library. And this is not a documentation issue, in my library I've disabled the feature that enabled the same thing by the attacker and now I have a section about security that to enable one feature the user needs to sanitize his input.

@jcubic
Copy link
Author

jcubic commented Jan 14, 2022

And about applying for CVE, I once wanted to report CVE and it was a complex process, that's why it's great that GitHub help with creating one. My last vulnerability in my own library I needed to report to NPM and because there was no CVE often I didn't get credit as the person that found it.

@jcubic
Copy link
Author

jcubic commented Jan 14, 2022

That said, if this is a documented feature, please think through what you're proposing.

So you don't know if this is documented or not? I've never found documentation that explains this. Also, this project has no documentation for AST. It doesn't even say what AST it is, I personally use Esprima syntax, which is never mentioned in the project.

@jsoref
Copy link

jsoref commented Jan 14, 2022

I'm not affiliated with any of this. I ran across your help request on github.community.

The repository README clearly says

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"

The code seems to be quite related to esprima, https://github.com/estools/escodegen/search?q=esprima

Offhand, it feels like https://docs.esprima.org/en/3.1/syntax-tree-format.html is "documentation" for the tree format.

Anyway, based on what I see, minus the lack of documentation, the raw appears to be a way to raw include things. If you want something else, it feels like this isn't what you want to use.

(it doesn't look like a <pre> html tag.)

@jcubic
Copy link
Author

jcubic commented Jan 14, 2022

AFIK Every open-source license is provided as is without any warranties.

And raw is the only way to use template literals. It seems it's documented:

https://docs.esprima.org/en/3.1/syntax-tree-format.html#tagged-template-expression

I usually just use Esprima to generate AST for the code I need, and then put that AST into my code to get the JavaScript. Template literals have only "raw" and "cooked" properties, but "cooked" doesn't work, so only raw can be used. But raw is not as is, this part of the string between expressions. So if you have:

`foo ${bar} baz`

You have 3 Nodes, first and third have property "raw": "foo ", and "raw": " baz".

You can see the output AST using AST Explorer.

@jcubic
Copy link
Author

jcubic commented Feb 14, 2022

If anyone is interested you only need to escape the string before you pass to escodegen.

function escape_quote(str) {
   return str.replace(/\$\{/g, '\\${');
}

@jcubic
Copy link
Author

jcubic commented Feb 14, 2022

I've reported the vulnerability to Snyk that can also assign CVE numbers.

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

No branches or pull requests

2 participants