Although I ended up not spending much time on this year’s RWCTF, I did (with the help of my awesome teammates) solve one problem: RWDN. The intended solution involved a bug in one of their middleware handlers that was designed incorrectly and allowed attackers to bypass a crucial check. However, I found that there was an alternate bypass that would have worked even if their code was correct. Let’s discuss what the bug is, and why it could be a problem for “real world” applications.

Update

The author, @wupco1996, got back to me to let me know that this was actually the intended solution, and the other more obvious one was just a normal, everyday bug. Props to them for a really clever problem, and it goes to show that even the best security experts can slip up from time to time.

Original

Before I describe the intended and unintended bugs, let me give a quick overview of this half of the problem. Users were presented with a page that allowed them to upload files to a CDN. The application would take users’ uploads and pass them through a content-type filter (based off of file extension alone) and reject if it did not match a small list of approved types.

However, the way that it performed this check as a little bit odd. The rendered upload page had a file input on it with a dynamically chosen name, and a form whose action included that name as a query parameter:

<form ref='uploadForm'
      id='uploadForm'
      action='/upload?formid=form-404bae6d-d475-4fd4-96b7-3f3badc86fc6'
      method='post'
      encType="multipart/form-data">
    <input type="file" name="form-404bae6d-d475-4fd4-96b7-3f3badc86fc6" />
    <input type='submit' value='Upload!' />
</form>

Now while this is strange, it is not inherently wrong. When the server then goes to fetch the file, it takes the file whose name is given by this query parameter (with no further verification). This is important for both the intended and unintended solutions, but not a bug on its own.

The Expected Bug

In the intended solution, a piece of middleware is used to check that every file uploaded is correct:

module.exports = () => {
    return (req, res, next) => {
        if (!req.query.formid || !req.files || Object.keys(req.files).length === 0) {
            console.log("Something error:", req.query.formid, req.files);
            res.status(400).send('Something error.');
            return;
        }
        Object.keys(req.files).forEach(function(key) {
            var filename = req.files[key].name.toLowerCase();
            var position = filename.lastIndexOf('.');
            if (position == -1) {
                return next();
            }
            var ext = filename.substr(position);
            var allowexts = ['.jpg', '.png', '.jpeg', '.html', '.js', '.xhtml', '.txt', '.realworld'];
            if (!allowexts.includes(ext)) {
                res.status(400).send('Something error.');
                return;
            }
            return next();
        });
    };
};

However, it has a silly but fatal bug wherein it calls next() within a forEach function. As a result, if you upload two files, one whose name is valid, and one whose name is not, then the server will both send a 400 status and continue processing the file. This means that you cannot see the result, but the upload happens nonetheless.

But what if this were written correctly? Well, it turns out that we still have a bypass.

The Gift that Keeps on Giving

Let’s take a look at the file upload library that this problem uses. A prior version of it had a security advisory related to prototype pollution when using nested files (essentially a qs/php style name parsing). Now, this version has since been fixed and nested files are disabled. However, prototype pollutions have a nasty habit of cropping up when you’re not used to looking for them.

Let’s take a look at these two files

/lib/processMultipart.js:116-125

req.files = buildFields(req.files, field, fileFactory({
    buffer: complete(),
    name: filename,
    tempFilePath: getFilePath(),
    hash: getHash(),
    size,
    encoding,
    truncated: file.truncated,
    mimetype: mime
}, options));

/lib/utilities.js:79-95

const buildFields = (instance, field, value) => {
    // Do nothing if value is not set.
    if (value === null || value === undefined) return instance;
    instance = instance || {};
    // Non-array fields
    if (!instance[field]) {
        instance[field] = value;
        return instance;
    }
    // Array fields  
    if (instance[field] instanceof Array) {
        instance[field].push(value);
    } else {
        instance[field] = [instance[field], value];
    }
    return instance;
};

There are a couple of things to note here.

1. If the first argument of buildFields (in this case req.files) is nullish, then it gets assigned to an empty object with Object.prototype as it’s prototype.
2. The key used for the object is field, a user controlled value
3. If instance[field] already exists, then the value is turned into an array containing the old values and the new one.

By now a sense of dread should be setting in. This is, undoubtedly, a prototype pollution.

Bringing it home

I don’t want to say that I have a favorite bug class, but I do and it’s prototype pollution. Even something as simple as this can be exploited for flags and profit.

Here’s the short version: If we name our malicious file __proto__, then when it gets parsed, it will look to see whether req.files.__proto__ exists, and when it does, will reassign it to be [Object.prototype, { /* our parsed file info */ }].

Why does this help? Object.keys will only return keys on the object itself, not its prototype. This means that the middleware that checks whether our file names are correct will never even look at this file.

However, this means that trying to access the file by its name, __proto__, cannot work because __proto__ is an array, not the object we expect. Fortunately, the file object we care about still exists, just at req.files.__proto__[1] and therefore req.files[1]. Unfortunately we’ve hit another roadblock, our index is the number 1, and we can only query a string filename.

Just kidding! This isn’t an issue at all because JavaScript arrays aren’t keyed by numbers, they’re keyed by strings. Seriously.

This means that if we set our formid query parameter to 1 then it will correctly access our file without it having every been checked against the whitelist.

Putting that together, we can use the following cURL command to upload a file with a bad name to the CDN.

curl --request POST \
  --url 'http://127.0.0.1:8000/upload?formid=1' \
  --form [email protected] \
  --form [email protected]

As an added bonus, no crash means that we get to see the generated file name without having to compute the hash ourselves for part 2!

Broader implications

Obviously in this instance the impact stems in part from the decision to control the file access via query parameters. Yet, in order to exploit the prototype pollution, all we needed to do was access a file whose name was an integer greater than zero. Using numeric keys for forms is not that unusual of a practice, and it means that when using express-fileupload, the programmer cannot trust that a key is on req.files itself.

Fortunately, this is an easy fix, both for express-fileupload (which should replace instance = instance || {}; with instance = instance || Object.create(null);) as well as the user (who can install middleware that explicitly sets req.files to Object.create(null)).

All-in-all this is not a high impact bug, but it could have allowed RWDN to omit an intentional bug without impacting the problem’s solvability, all while making it that much more “real”.