What is sanitize.



  • I was tasked with placing a NodeJs server written by a 3rd party onto a AWS instance, I dont know any NodeJs but my boss says no problem its all built out you just gotta set up the enviroment. I get the server up and running on a staging server to test everything out however every few days the server crashes, no apparent log output relating to this fatal behavior or anything out of the ordinary. After the 2nd time I decide to dig into the code and see if I can diagnose it.

    I do end up finding the bug which causes this server to crash, turns out web bots are actually requesting URL's which cause an uncaught exception in one of the controllers and the server crashes with no logging, turns out the server falling over isnt a big deal for this application you can just use this thing called a forever which boots it right back up... 😒 .

    Anyway I stumble across a bit of questionable code along the way which I decide to look into a bit deeper. This is a method which handles a requests made to a url pattern like /:submissionId/pdf. It calls the wkhtmltopdf application which lives outside of node of course to generate the PDF and output it to a directory that is accessible to the user and redirects the request to that url.

    exports.pdfResults = function(req,res,next){
        // the submission will be provided
        var submission = req.criteria;
        //if there isn't one, we can't continue
        if(!submission) return next(new Error('You are trying to view results for an invalid submission'));
    
        //now we can load the results
        results.loadBySubmissionId(submission.submissionId, function(error,result){
            if(error) return next(error);
            if(!result) return next(new Error('no results exist for this submission. Re-submit the results'));
            
            var targetPath = 'http://' + req.headers.host + '/' + submission.submissionId + '/print';
            var pathInPublic = "/pdf_results/" + submission.submissionId + ".pdf";
    
            child_process.exec(config.wkhtmltopdf + " " + targetPath + " " + "./public" + pathInPublic, {}, function(err, stdout, stderr) {
                if(typeof(err) !== 'undefined') console.warn("ERROR: ", err);
                res.redirect(pathInPublic);
            });
        });
    };
    

    Now I dont know much Node but the line var targetPath = 'http://' + req.headers.host + '/' + submission.submissionId + '/print'; looks an awful lot like someones appending the "Host" header into a string which is about to be run as its own command.... Thinking gosh that cant be right lets test it out:

    curl --header "Host: & mkdir command_injection &" http://localhost:3000/553bfd1cb87f4c602b5d4b33/pdf
    

    😱 yes, the directory was created.



  • What! No one will ever do that! -- Lead Developer.


  • Discourse touched me in a no-no place

    @nstuart said:

    Now I dont know much Node but the line var targetPath = 'http://' + req.headers.host + '/' + submission.submissionId + '/print'; looks an awful lot like someones appending the "Host" header into a string which is about to be run as its own command....

    Oh no. Not that vulnerability category. Again. :facepalm:

    No matter how convenient it is, using something which just gloms all the arguments together and reparses them is just asking for trouble. If you have to launch subprocesses at all, at least do it by using the API which lets you make sure that the arguments are exactly the strings that you expect and not some other reinterpretation. [spoiler](And that's also why Windows's way of delegating command argument splitting to the subprocess is such an annoyance; it makes it just so much harder to get this stuff right by construction.)[/spoiler]



  • https://github.com/SockDrawer/SockBot/blob/master/sock_modules/lojban.js#L30

    You mean like the API that they were using incorrectly? That one?



  • Ugh! Painful.

    Especially since he could have just used req.hostname.

    Other WTF:

    if(typeof(err) !== 'undefined') console.warn("ERROR: ", err);
    

    So, he just prints out a useless warning to the console, instead of notifying the user (next(err)). And what about stderr? If wkhtmltopdf chokes, there's not even console.log, the user is just redirected to the pdf, where they soak up 404 or get a previously generated PDF.

    But other than these two, the code seems competent, if mediocre. I've seen worse. Injection is, IMO, probably an honest mistake.



  • @dkf said:

    And that's also why Windows's way of delegating command argument splitting to the subprocess is such an annoyance; it makes it just so much harder to get this stuff right by construction.

    The windows way of passing the arguments as single string is _silly_â„¢, but most commands won't do that much evil even if they split wrong if you avoid invoking the shell. Besides, higher-level languages generally have appropriate function to handle this in their standard library.


Log in to reply