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

Update to new Multer version #3597

Closed
sorryididntmeantto opened this issue Oct 13, 2016 · 3 comments
Closed

Update to new Multer version #3597

sorryididntmeantto opened this issue Oct 13, 2016 · 3 comments

Comments

@sorryididntmeantto
Copy link

Keystone depends on multer and uses a really old version, vulnerable to a very simple DoS which brings the server down on a malformed multipart body. Also, even if updated, the way it is used is strongly discouraged and would allow a more complex DoS by filling up the disk with unhandled files.

The details about the first vulnerability are on expressjs/multer#224. The note about the second vulnerability is on that project's README.

This vulnerability has already been exploited on one of my servers, probably accidentally by some badly written crawler, but bringing any Keystone instance down is rather simple until this gets fixed.

Steps to reproduce the behavior

Send an incomplete or malformed multipart body as specified in expressjs/multer#224.

Expected behavior

The server returns a 400 error because of bad input.

Actual behavior

The server crashes.

@VinayaSathyanarayana
Copy link

Would recommend that we also do a nsp check to check for other issues

@JedWatson JedWatson changed the title DoS with simple POST Update to new Multer version Oct 15, 2016
@JedWatson
Copy link
Member

@sorryididntmeantto Not sure if your username and otherwise new account is a reference to just this issue... I would honestly have preferred a more responsible disclosure strategy but here we are. Hope you don't mind that I've updated the issue title to lower visibility until we can get a fix released; let me know if you think this should be dealt with differently.

@VinayaSathyanarayana agreed

Regarding the work to update multer, there were several blocking tasks that needed to be completed first, because (as you mention) the way it is used is not ideal, but Keystone has historically made a number of fundamental assumptions about handling uploaded files based on that way from before this vulnerability was discovered which means it's been very difficult to update from the old version (to put it simply, with the v0.3.x architecture we simply didn't have way to know which fields to expect in the middleware configuration, which is a problem more specific to generic solutions and less of a concern for applications implementing multer directly)

Anyway, the new architecture doesn't have these problems and in light of this issue I'm going to push through the remaining work to upgrade to the new version as fast as I can. If anybody wants to lend a hand (especially with testing) please let me know.

For what it's worth, @sorryididntmeantto I'm sorry you've had issues in production due to this - that's never fun, and hopefully it won't be an issue much longer.

@stennie
Copy link
Contributor

stennie commented Jul 7, 2018

Closing as a duplicate of #4428, which will be addressed in Keystone 4.0.0-rc.2.

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

No branches or pull requests

4 participants