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

Added more checks #3428

Closed
wants to merge 3 commits into from
Closed

Added more checks #3428

wants to merge 3 commits into from

Conversation

molangning
Copy link

@molangning molangning commented Feb 26, 2024

If templateName gets injected attackers can inject arbitrary code.

Example payload: templateName="a(){}; function template (locals) {var pug_html = process.mainModule.require('fs').readFileSync('/etc/passwd').toString(), pug_mixins = {}, pug_interp;var pug_debug_filename, pug_debug_line;try {;//"

Checks are added to restrict template names to 0-9, a-z, A-Z, -, _

Special conditions are needed for this to happen and proper circumstances are needed for this vulnerability to be triggered.

Additionally, if node.line get affected due to prototype pollution code is also injectable

Example payload: Object.prototype.block = {"type": "Text", "line": "console.log(process.mainModule.require('child_process').execSync('id').toString())"};

Reference: https://po6ix.github.io/AST-Injection/

A check is added to to ensure the line number is really a line.

#3414 is also worth a follow up, and maps could be used for options to prevent prototype pollution but changing all of those may break some things so I did not touch those.

Copy link

There is no change log for this pull request yet.

Create a changelog

Comment on lines 318 to +323
if (debug && node.debug !== false && node.type !== 'Block') {
if (typeof node.line !== 'number') {
throw new Error(
'node.line is not a valid number.'
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (debug && node.debug !== false && node.type !== 'Block') {
if (typeof node.line !== 'number') {
throw new Error(
'node.line is not a valid number.'
);
}
if (debug && node.debug !== false && node.type !== 'Block' && typeof node.line === 'number') {

isnt should better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would, but that meant that it’s checking for all three instead of checking for the required two and failing silently. Throwing an error is better as it can alert the user that there may be a pollution.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm i understand

Copy link

@victor-0x29a victor-0x29a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@samuzora
Copy link

Hi! I'm the original discoverer of this bug - I wrote a CTF challenge to demonstrate a PoC of this in late Feb 2024 here. @molangning was a participant in the CTF, and I gave him permission afterwards to open a PR to introduce the necessary checks to mitigate the issue.

In the original challenge, the vulnerability was introduced by exposing the compileBody internal function which does no checks/restrictions on the templateName option. This, together with passing of the query object to options, allows for server-side RCE. However, I did not consider this a bug that was worth reporting because:

  1. It requires the exposure of internal functions that aren't exported in the regular Pug module
  2. Passing in of the options object is generally considered wrong usage of JS templating engines (eg. ejs's homepage https://ejs.co/)

However, since then, I did a re-audit of the Pug source, and realized that the exported functions compileClient, compileClientWithDependenciesTracked and compileFileClient are all vulnerable to this bug. The main line of contention is in here, where options.name is passed into templateName with no checks.

Minimal working example:

const express = require("express")
const pug = require("pug")
const runtimeWrap = require('pug-runtime/wrap');

const PORT = 3000

const app = express()

app.get("/", (req, res) => {
  const out = runtimeWrap(pug.compileClient('string of pug', req.query))
  res.send(out())
})

app.listen(PORT, () => {
  console.log(`Server is running on port ${PORT}`)
})

The minimal working payload for server-side RCE is template() { var x = global.process.mainModule.require; return x("child_process").execSync('ls').toString(); }; function asdf. Passing this to the name query parameter will result in RCE.

Still, this is contentious usage of the compileClient family of functions - these are usually used for compiling the rendering function server-side, then sending it to the client to be rendered client-side. The passing of arbitrary options to compileClient is also still required. Nevertheless, arbitrary JavaScript execution is still possible client-side, leading to possible XSS and CSRF.

The urgency of this issue is a lot higher now since it doesn't require the intentional exposure of internal functions. Just a small suggestion regarding the regex check - it should be /^[0-9a-zA-Z\_]+?$/ without the -, to prevent unintentional SyntaxError from the dash. Other than that, the PR should be merged as soon as possible.

I apologize if this was a breach of the security report guidelines for Pug. I was not aware that the bug could be exploited in legitimate applications of the module, and hence allowed @molangning to publish a PR directly.

@benedekh
Copy link

Hi all, would it be possible to prioritize merging this pull request? 🙏 GitHub dependanbot and npm audit have picked up this CVE and might be blocking the CI pipelines of several products worldwide, which transitively depend on pug.

/cc @ForbesLindesay

@molangning
Copy link
Author

Making the jump from ctf to cve is something I didn't see coming. Also yeah this should get merged asap 🙏

@samuzora
Copy link

The fix has been merged and released in v3.0.3, under this PR: #3438. But I think the registries need to be updated to mark the latest version as patched.

@y-nk
Copy link

y-nk commented Jun 27, 2024

@samuzora let's close this PR then?

@molangning
Copy link
Author

I’m closing this pr as the requested changes has been merged.

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.

5 participants