Current cspSsrNonce plugin can result in security problem in some cases

[maple3142]

Environment

- Operating System: Linux
- Node Version:     v22.11.0
- Nuxt Version:     3.14.1592
- CLI Version:      3.17.2
- Nitro Version:    2.10.4
- Package Manager:  [email protected]
- Builder:          -
- User Config:      default
- Runtime Modules:  [email protected]
- Build Modules:    -

Nuxt Security Version

v2.1.5

Default setup used?

Yes, the bug happens even if the security option is not customized

Security options

Reproduction

https://github.com/maple3142/nuxt-security-nonce-bug-repro

1. yarn install && yarn build
2. node .output/server/index.mjs
3. Visit http://localhost:3000/?statusMessage=1&html=%3Cdiv+id=%22%3Cscript%3E%3Cscript%3Ealert(origin)%3C/script%3E%22%3E%3C/div%3E from browser

Description

The browser would pop an alert when visiting that url (so XSS happens here), despite https://github.com/maple3142/nuxt-security-nonce-bug-repro/blob/master/pages/index.vue does sanitize the incoming html using DOMPurify.

The main problem is that nuxt security incorrectly add nonces to script tags using simple string replacement here:

security/src/runtime/nitro/plugins/40-cspSsrNonce.ts

Lines 53 to 73 in 70cd67a

	for (const section of sections) {
	html[section] = html[section].map((element) => {
	// Skip non-string elements
	if (typeof element !== 'string') {
	return element;
	}
	// Add nonce to all link tags
	element = element.replace(LINK_RE, (match, rest) => {
	return `<link nonce="${nonce}"` + rest
	})
	// Add nonce to all script tags
	element = element.replace(SCRIPT_RE, (match, rest) => {
	return `<script nonce="${nonce}"` + rest
	})
	// Add nonce to all style tags
	element = element.replace(STYLE_RE, (match, rest) => {
	return `<style nonce="${nonce}"` + rest
	})
	return element
	})
	}

. And v-html can result in unescaped <script> to appear in rendered html in SSR mode.

I think nuxt-security should not process html in from v-html, and should not naively use string replacement to do that because it may accidentally replace fake script tags inside element attributes.

Additional context

No response

Logs

[vejja]

Hey @maple3142
I'm still turning my head around the potential implications of this, but your example is not a bug.

What you are doing here is not XSS. You are SSR-ing a dynamic page.
Your setup uses an SSR server, and you are instructing this server to generate a page that contains a <script> element.
Your server is following your instructions faithfully, there is nothing wrong about this.

XSS happens when a page is modified on the client-side.
The CSP security model prevents client-side injections, but assumes that anything delivered by the server is valid.
It is therefore the server's responsibility to generate safe content.

Using v-html with a random user-provided input string is a security hole that you are opening yourself on the server side.
CSP will not prevent the script from executing, because it assumes that your server is safe.

That being said, the specifics of your example rely on using DOMPurify to sanitize the user input. I'm not knowledgeable about this library so I won't be able to comment specifically. However it looks like the user input is not sanitized from potential script tags.

[maple3142]

The problem here is that DOMPurify does it job correct, because the <script> is inside an element's attribute, which does not need to be escaped.

For example, the following html is completely safe:

<div id="<script>">content</div>

so DOMPurify will left it as is. If that script tag is not inside that attribute then DOMPurify will remove it as you would expect.

The problem here is that nuxt-security does a simple string replacement to add nonce to script tags, without considering whether it is actually a script tag or not. In this case, nuxt-security does it on <script> on text inside a html attribute.

You can try to uninstall nuxt-security from my example repo and try again the url that triggers XSS, and you will see it does not happen anymore.

[vejja]

I get this part:

The problem here is that nuxt-security does a simple string replacement to add nonce to script tags, without considering whether it is actually a script tag or not.

Which is a valid comment.

However I still don't understand where is the XSS ?

[maple3142]

The input to DOMPurify is the following HTML:

<div id="<script><script>alert(origin)</script>"></div>

and after DOMPurify's sanitization, it becomes:

<div id="<script><script>alert(origin)</script>"></div>

(no changes)

When this html is rendered by SSR, it does not result in XSS because it is not a script tag.

But if nuxt-security is installed, it becomes:

<div id="<script nonce="NONCE"><script nonce="NONCE">alert(origin)</script>"></div>

and this does result in the alert being executed by browsers, which is an XSS introduced by nuxt-security.

A fix for this is to modify cspSsrNonce to add nonce by properly parsing html so it no longer add nonce for <script> text inside attributes.

[vejja]

Ok, I get it now

@GalacticHypernova would you have an idea for this ?
@maple3142 actually has a valid point in his description above

The issue is that:

<div id="<script><script>alert(origin)</script>">
</div>

Gets transformed into:

<!-- here is what happens below -->
<!-- the opening quote of the nonce actually closes the id attribute -->
<!-- then the NONCE value is interpreted as a custom boolean attribute -->
<!-- finally the dangling closing quote is ignored -->

<!-- attribute     attribute   invalid_but_ignored -->
<!--  |                   |    | -->
<!--  v                   v    v -->
<div id="<script nonce=" NONCE "> 
  <!--                          ^ -->
  <!--                          | -->
  <!-- The closing bracket above should have been inside the id attribute -->
  <!-- but now it has been pushed outside and closes the div tag -->
  <!-- this causes a second script block to be inserted -->
  <!-- which also has a nonce, so it gets executed -->
  <script nonce="NONCE"> 
    alert(origin)
  </script>
  <!-- The 2 characters below don't have any sense but they get printed on screen -->
  <!-- because they are now interpreted as text content of the div -->
  "> 
</div>

Formatting, block indenting, extra spaces and comments are from me to increase legibility of what happens.
The attack is based on quote mismatch inside the id attribute.
It makes the assumption that the server :

1. provides a public route that accepts the injection of any html into the DOM
2. can do so because it relies on a 3rd-party library to sanitize user input (here, DOMPurify)

[GalacticHypernova]

The issue with this is as follows:

The ID attribute is primarily used for in-page navigation. It also generally shouldn't be non-standard, as the ideal is a selector that is valid both as a CSS selector and a JS selector (https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id#syntax).

This nonce issue only arises when we purposefully try to make a non-standard ID, which is just bad practice by the developer. In terms of real world usage, this situation will pretty much never happen, as the developer is in control of the ID.

If it allows user-defined HTML however, that's a different problem. In that case, for the same reason v-html is susceptible to XSS injections (because it bypasses any and all security measures), I believe the same applies here. Just like the developer would sanitize user-defined HTML before applying it to v-html, I think it would be reasonable to leave it to the developer's discretion to also sanitize attributes in the HTML.

Technically speaking, we can make a change to the regex, but the whole situation is just not a very realistic one that will not happen in real life unless the developer purposefully makes a non-standard ID (which is bad practice), or when they accept user-defined HTML, in which case in my opinion they should also sanitize attributes (which is also bad practice if they don't).

[vejja]

Agree with your analysis.
However it would still be nice to modify our regex, would it be extremely difficult ?

[GalacticHypernova]

However it would still be nice to modify our regex, would it be extremely difficult ?

This would depend, I'd have to look into it, because it can be in the form of

<div
id="<script></script>"
/>

As well as

<div id="<script></script>" />

It definitely complicates the parsing process to a degree (which will in turn increase resource demand on constrained environments), but I'd have to figure out to which degree that would be exactly.

[GalacticHypernova]

If it turns out to be too resource-intesive to the point where it causes noticeable slowdown, I believe we should really think whether or not this is necessary.

If someone would really want to make an XSS in their website, they will be able to, even with the strictest settings and behavior imaginable.

Besides the security of the tools in use, there's also the responsibility of the developer to follow the web specs and best practices, including security.

[maple3142]

Just a note that this has nothing to do with the specific attribute id, as it can happen with any html attributes like href, alt or whatever.

Also, in normal usecase like <img :src="user_url" :alt="user_alt"> this attack would not apply, because vue will encode <> characters in attributes.

The main problem about this issue is when website want to allow user supplied html to some extent (or markdown/bbcode/whatever), so they apply html sanitizer like DOMPurify to do that and put it into v-html. I don't think there are other cases where this can apply in the context of nuxt.

[vejja]

Just a note that this has nothing to do with the specific attribute id, as it can happen with any html attributes like href, alt or whatever.

Yes, understood

Also, in normal usecase like <img :src="user_url" :alt="user_alt"> this attack would not apply, because vue will encode <> characters in attributes.

Exact, although we don't run at the Vue level - we only interact after the final HTML generation step so I don't think we need to worry about this

The main problem about this issue is when website want to allow user supplied html to some extent (or markdown/bbcode/whatever), so they apply html sanitizer like DOMPurify to do that and put it into v-html. I don't think there are other cases where this can apply in the context of nuxt.

Yes, but that's the main issue technically for us. At the time we interact, we don't have the information anymore whether this was generated from a v-html or something else

[vejja]

If it turns out to be too resource-intesive to the point where it causes noticeable slowdown, I believe we should really think whether or not this is necessary.

If someone would really want to make an XSS in their website, they will be able to, even with the strictest settings and behavior imaginable.

Besides the security of the tools in use, there's also the responsibility of the developer to follow the web specs and best practices, including security.

Agree again.

But the issue I'm seeing here is that we are the ones injecting the quotes that break the original HTML

[GalacticHypernova]

But the issue I'm seeing here is that we are the ones injecting the quotes that break the original HTML

True, but then again, that's just in a scenario that will pretty much never happen.

Just a note that this has nothing to do with the specific attribute id, as it can happen with any html attributes like href, alt or whatever.

I mean, what legitimate use case requires an attribute, any attribute, to be a script tag?

If the developer allows people to add custom script tags to a page, that's an XSS waiting to happen, no matter what security tool is used. Same with styles, you can introduce a key logging vulnerability with malicious css. And not to mention the real life overhead of moderating it if someone can just add an inappropriate background image or a script that navigates to it. And that, as I stated previously, would be bad practice regardless of the tools used.

This isn't to say I won't review it and attempt to fix it of course, I'm just saying that if the fix happens to be unproportionally expensive, then we should likely pick and choose our battles and go with the scenario that ultimately has real world impact. (We could even add a caveat in the docs about this specific case, just in case)

[maple3142]

I mean, what legitimate use case requires an attribute, any attribute, to be a script tag?

Not much I think, the only possible scenario I can think of is an image of HTML code, so its alt would contain HTML content to cater to screenreader users. However, this kind of content is still safe by itself so sanitizers like DOMPurify does keep it as it so such isseus definitely happen in real world.

If the developer allows people to add custom script tags to a page

I think I have to reclarify that in the scenario here, the developer does not allows people to add custom script tags to a page by using DOMPurify. It is just text that happens to be HTML content are embedded in a HTML attribute, which is safe so there is no reason to ban that.

I think it is fair to expect that if a nuxt app does not have security vulnerability, then adding nuxt-security to it should not introduce one. (at least in default configuration)

if the fix happens to be unproportionally expensive

Yeah, this is also what I would expect because the "proper" fix I have imagined would be using a proper HTML parser, but this might be expensive in terms of performance. Another possible fix would be disabling nonce based CSP by default and add a warning about this in docs, because I believe self and strict-dynamic CSP should be sufficient to most normal nuxt apps. Of course there might be some cases that I don't know isn't covered by these two CSP directives.

[GalacticHypernova]

only possible scenario I can think of is an image of HTML code, so its alt would contain HTML content to cater to screenreader users

Considering you can also phrase it as "html script tag responsible for xyz" (not to mention it is incorrect to place it in the format of an actual script tag anyways because the alt is an explanation, and that would mean plain text inside a format of a script tag) , it would still be bad practice to make it in the format of an actual script tag, wouldn't it? I'm also unsure whether screen readers even actually pick up <> as readable characters.

I think I have to reclarify that in the scenario here, the developer does not allows people to add custom script tags to a page by using DOMPurify.

Yes, I didn't mean in this context you referred to, my apologies if it seemed that way. As I stated in a previous message, if it's a script tag in an attribute, that's bad practice because there is definitely a better way of doing it. I was saying that if it isn't the case, then the only other affected use case would be developers that allow people to add custom scripts, and that's an XSS waiting to happen regardless.

Another possible fix would be disabling nonce based CSP by default and add a warning about this in docs, because I believe self and strict-dynamic CSP should be sufficient to most normal nuxt apps. Of course there might be some cases that I don't know isn't covered by these two CSP directives.

That's a slight problem, because strict-dynamic relies on nonces and hashes. It allows you to specify a nonce for the "root" script, that all other scripts loaded from it would automatically be accepted.

[vejja]

Yeah, this is also what I would expect because the "proper" fix I have imagined would be using a proper HTML parser, but this might be expensive in terms of performance.

Absolutely. As a matter of fact, in previous versions we used to rely on Cheerio to parse HTML.
But the performance issues were terrible. The latency impact was huge and we were hitting memory limits in worker environments.
That’s why we moved to regex-based parsing.