Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

Update express to 4.17.3 to address querystring vulnerability #66

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

eleichter-globality
Copy link
Contributor

@eleichter-globality eleichter-globality commented Dec 22, 2022

See GLOB-70859 and https://github.com/n8tz/CVE-2022-24999 for background on this vulnerability.

I managed to reproduce the exploit by borrowing from the vulnerability repo's POC in globalitydocx. I added a dummy route that looked like this:

    router.get('/api/qs-test', function(req, res, next) {
        const categories = req.query.categories;
        
        console.log('[server] Received query : ', req.query);
        console.log('[server] (categories instanceof Array) : ', (categories instanceof Array));
        
        if ( !categories || !(categories instanceof Array) ) {
            res.send('Categories is not an array, which is probably a good thing');
            return;
        }
        
        console.log('[server] Do : req.query.categories.indexOf("123")');
        console.time('[server] req.query.categories.indexOf("123")');
        req.query.categories.indexOf("123")
        console.timeEnd('[server] req.query.categories.indexOf("123")');
        
        res.send('Done!');
    });

Then sent a request to a locally-running server with this:

time curl --location -g 'http://localhost:3037/api/qs-test?categories[__proto__]&categories[__proto__]&categories[length]=100000000'

Without the update to this library, the server treated categories like an array and took 4 seconds to respond (even if its own copy of express was updated to the fix version). Once I got the local server to use this version of nodule-express, it responded to the same request quickly, and hit the Categories is not an array branch.

@@ -20,7 +20,7 @@
"connect-requestid": "^1.1.0",
"cors": "^2.8.4",
"dataloader": "^1.4.0",
"express": "^4.16.3",

Choose a reason for hiding this comment

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

Just want to verify that we want 4.17.3 and not ^4.17.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, it looks like we can. This gets us express 4.18.2, which has qs 6.11.0 which has the fix (that came from here). And I verified that I can't reproduce the vulnerability with this new version of nodule-express + express.

(I'm somewhat obsessively double-checking all this because I spent the better part of the day jumping through dependency hell to test this).

Copy link
Contributor

@jyoko-glob jyoko-glob left a comment

Choose a reason for hiding this comment

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

Appreciate you fighting with the dependencies! 🍻

@eleichter-globality eleichter-globality merged commit 9e67066 into master Jan 4, 2023
@eleichter-globality eleichter-globality deleted the GLOB-70859_update-express-for-vuln branch January 4, 2023 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants