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

Is the comment html code injection a bug or a feature? #1991

Closed
syonfox opened this issue Jun 1, 2022 · 7 comments
Closed

Is the comment html code injection a bug or a feature? #1991

syonfox opened this issue Jun 1, 2022 · 7 comments

Comments

@syonfox
Copy link

syonfox commented Jun 1, 2022

Looks like there is still some injection after
#1908

not sure if its related yet

Input code

/**
 * <script> alert("cool jsdocs code injection")</script>
 * @class BS
 * @namespace bs */
let bs = {
	/**
	* @param foo
	* @returns {*}
	*/
	myFunc:(foo)=>{console.log("hello World")}
}

JSDoc configuration

mkdir test
nano test; # past in above
npm install -g jsdoc
jsdoc test.js -d decs
firefox /docs/index.html

Expected behavior

Probably escape any js but allow injecting html ?

Current behavior

embed script tag in docs and they execute.
Mayby its a feature. But it should probably have an compile option
sorry if i missed somthing, Happy coding

Your environment

Software Version
JSDoc JSDoc 3.6.10 (Tue, 25 Jan 2022 02:05:39 GMT)
Node.js NA
npm 8.6.0
OS 10.0-14-amd64 #1 SMP Debian 5.10.113-1 (2022-04-29) x86_64 GNU/Linux
@andyedwardsibm
Copy link

Still there in jsdoc@4.0.1 and starting to be flagged by image scanning tools like Twistlock/Prisma

@hegemonic
Copy link
Contributor

Script injection should only be a major concern if you're using JSDoc to process untrusted user input. Does your use case involve untrusted user input? If so, can you say more about your use case?

Also, @andyedwardsibm, can you point me to an example of how and where JSDoc is getting flagged for this?

@andyedwardsibm
Copy link

@hegemonic unfortunately this is where it gets a bit messy...

Palo Alto Networks, who make the Twistlock image scanner, include a set of vulnerabilities in addition to CVEs and GHSAs that they assign "PRISMA" numbers to. These are usually for cases like this, where they've discovered a "security" issue that doesn't have a CVE. The problem is that they treat that as proprietary information and part of their competitive advantage ("buy Twistlock and we can find these extra vulnerabilities"), and they don't seem to publish those vulnerabilities in a publicly accessible forum.

The consequence is that I can't point you at a publicly visible example and I don't know what I'm allowed to say without breaking some contract with them. All I can say is that they can scan a Docker image, detect the npm package jsdoc, claim that the version installed has a vulnerability and link back to this issue

@hegemonic
Copy link
Contributor

Thanks for the info, @andyedwardsibm. My overall response—not to you, but to the scheme that you describe—is "ugh."

I stand by my view that there's no compelling reason for JSDoc to prevent script injection, which is primarily an issue for applications that process untrusted user input. (In contrast, the typical use case for JSDoc is that you're generating API documentation for code that you control to some extent.) I might be wrong about this, and I'm more than happy to change that view based on additional evidence, but right now I just don't think this is a real issue.

It'd be possible to prevent script injection in JSDoc if necessary. However, that would make JSDoc at least a little bit slower for everyone who uses it. I'd rather not do that based on secret, proprietary evidence.

If it's possible to share that feedback with the vendor, that would be much appreciated. If they're not receptive to your feedback, please consider asking them whether they've also evaluated all of the following API-doc and static-site generators, and the many others out there, for similar issues:*

* Maybe some of these applications prevent script injection; I have no idea. But I bet at least a few of them don't! And my current belief is that most of them don't need to.

@hegemonic hegemonic closed this as not planned Won't fix, can't repro, duplicate, stale Feb 23, 2023
@andyedwardsibm
Copy link

I agree with your assessment @hegemonic, and one could argue that "script injection" is a valid feature (i.e. it's not "script injection", it's "writing code"), and it's down to the code using jsdoc to consider the origin of their code.

One candidate case where JSDoc is used as a runtime dependency is https://www.npmjs.com/package/protobufjs-cli, which generates code to handle protobuf data, but yeah, the code is generated by that tool, and it's down to the user to know the origin of their protobuf definitions

@syonfox
Copy link
Author

syonfox commented Feb 23, 2023

Hi I would add my 2 cents that it kind of is a feature and perhaps there should be a layer added at the input level that sandboxed any untrusted jsdoc generation or sanitizes the input code. this is a whole other beast worthy of its own project. and there are existing implementations for this probably a google away. This does allow for some cool stuff like a working example to be embedded in your jsdoc page... An idea is to have an untrusted build flag which would escape all HTML tags. I am not to familure with the guts of jsdoc but would be a matter of escaping all <> with utf8 char codes or something.

An example of how this might be exploited may be running jsdoc on a project which has an unused and unknown dependency (node_modules) that is skipped by some vulnerability scanner. This could then inject a payload in your docs and propagate some explot between multiple developers. THis could be mitigated by scanning the generated doc pages. I do see the value in having it as an explicit feature and perhaps how HTML is handled by jsdocs should be documented. If it is and you know where please link it.

--escape-html // this would escape all HTML injection so you could put a "snipit" in your doc
                  // probably like how @example works so not really much different.
--escape-js // or maybe deny/allow idk but this would still allow HTML but strip any js

@Jmainguy
Copy link

Palo Alto has referenced this issue for their CVE (Which is not in NIST or accepted in the industry), PRISMA-2022-0226 which they report as a severity high finding with a CVSS 7.20

"
jsdoc package from all versions are vulnerable for HTML injection. It is possible to inject HTML content in the comments before the code is documented. Once code is commented and jsdoc was used to generate HTML website from the sourcefiles, accessing webpage where HTML was injected in comments will show the actual element. This issue allows page modification through the comments and exposes the user for different vulnerabilities such as Cross-Site Scripting (XSS).
"

As there was no hits on google for "PRISMA-2022-0226" I added this comment to the CLOSED issue. I personally believe this is not a real vulnerability but rather a feature of jsdoc.

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

4 participants