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

Big security vulnerability #75

Closed
benstevens48 opened this issue Jan 27, 2018 · 18 comments · Fixed by #89
Closed

Big security vulnerability #75

benstevens48 opened this issue Jan 27, 2018 · 18 comments · Fixed by #89

Comments

@benstevens48
Copy link

@benstevens48 benstevens48 commented Jan 27, 2018

Hi,

I've just realised that it is very easy to create a big security vulnerability when using this package to upload files (I almost did this on my site).

Suppose you have JSON body parsing enabled on a POST or PUT route, say '/upload-files', as well as multipart parsing. This is quite easy to do e.g. if you add JSON parsing as global middleware. Suppose it expects the files to be in a field named 'uploads'. Then you can make a POST or PUT request to '/upload-files' with a JSON body that looks something like {"files": {"uploads": [{"path": "/any/file/path"}]}}, making the request handler think a file has been uploaded to /any/file/path. Now suppose that the handler is set up to copy uploaded files into a public uploads folder. By choosing the path appropriately I can make the server copy any file I like on the server into the public uploads folder and then view its contents. So by using well-known paths of sensitive files I can read private keys, passwords etc. and maybe even gain full access to the server this way.

I haven't tried actually doing this, but I think it's correct (sorry if I'm wrong). In my opinion, it would be better to put the files object on ctx.request instead of ctx.request.body, so that we know it can be trusted.

Thanks.

@HelloWorld017
Copy link

@HelloWorld017 HelloWorld017 commented Jan 27, 2018

I finished testing Proof-of-Concept.

Screenshot

@ChalkPE
Copy link

@ChalkPE ChalkPE commented Jan 27, 2018

Now suppose that the handler is set up to copy uploaded files into a public uploads folder.

It really does!

$ curl -X POST \
       -H "Content-Type: application/json" \
       -d '{"files":{"file":{"name":"lol","path":"/etc/passwd"}}}' \
       localhost:3000

file sent, copying /etc/passwd -> /home/chalk/github/koa-body-poc/public/lol

@HelloWorld017
Copy link

@HelloWorld017 HelloWorld017 commented Jan 27, 2018

This vulnerability is really a big issue and should be patched asap.

@benstevens48
Copy link
Author

@benstevens48 benstevens48 commented Jan 27, 2018

Thanks for testing it out! I also think it should be addressed asap, but I'm not sure if it can be 'patched' without changing the API. As I suggested before, putting the files object (and possibly fields as well although that's not so important) on ctx.request instead of ctx.request.body would be a good solution I think.

Something else that could maybe slightly limit the vulnerability would be not enabling parsing other content types by default, but then again it's easy to add global JSON parsing middleware anyway without thinking so this wouldn't provide much extra security.

HelloWorld017 added a commit to HelloWorld017/koa-body that referenced this issue Jan 27, 2018
@ChalkPE
Copy link

@ChalkPE ChalkPE commented Jan 27, 2018

It's guaranteed to ctx.request.body.files is safe if ctx.is('multipart'), so this way would be a temporary solution for this vulnerability?

@HelloWorld017
Copy link

@HelloWorld017 HelloWorld017 commented Jan 27, 2018

This pull request changes whole body into "fields" field, which matches api. When this patch is applied, if files field is created by body, it becames ctx.request.body.fields.files, which fulfills original api.

@benstevens48
Copy link
Author

@benstevens48 benstevens48 commented Jan 27, 2018

I think this will cause problems if someone is trying to support both multipart and JSON request bodies on the same route (even though I don't see why they would want to)...

@HelloWorld017
Copy link

@HelloWorld017 HelloWorld017 commented Jan 27, 2018

Then I think we should remove files property from the body.

@ChalkPE
Copy link

@ChalkPE ChalkPE commented Jan 27, 2018

@HelloWorld017 That way also causes problem if you have normal files field (like <input name="files" type="text">)... though that is not common case.

@HelloWorld017
Copy link

@HelloWorld017 HelloWorld017 commented Jan 27, 2018

That might be a crash with normal multipart routung.

@ChalkPE
Copy link

@ChalkPE ChalkPE commented Jan 27, 2018

As I suggested before, putting the files object (and possibly fields as well although that's not so important) on ctx.request instead of ctx.request.body would be a good solution I think.

to maintain backward compatibility: both on ctx.request and on ctx.request.body?

@benstevens48
Copy link
Author

@benstevens48 benstevens48 commented Jan 27, 2018

Erm well I think people ought to be discouraged from using ctx.request.body.files. You could keep it to maintain compatibility but there should be a very obvious warning about the dangers in the readme. Or you could remove it and increase the major version number, or add a deprecation warning and then remove ctx.request.body.files if you get round to releasing another major version.

@imkimchi
Copy link

@imkimchi imkimchi commented Jan 27, 2018

@dlau I made a PR that puts file objects into ctx.request.files instead of ctx.request.body.files. even changed a couple tests due to those changes :) #77

@damianb
Copy link
Contributor

@damianb damianb commented May 15, 2018

@dlau is this on your road map to address, or is this module no longer being maintained?

@kingjerod
Copy link

@kingjerod kingjerod commented May 18, 2018

Anybody looking for an alternative, you can use https://github.com/felixge/node-formidable

For a Koa wrapper: https://github.com/GaryChangCN/koa2-formidable

carlansley added a commit to carlansley/swagger2-koa that referenced this issue May 23, 2018
MarkHerhold added a commit that referenced this issue Jun 3, 2018
Use `ctx.request.files` instead of `ctx.request.body.files` - Fixes #75
MarkHerhold added a commit that referenced this issue Jun 4, 2018
Use `ctx.request.files` instead of `ctx.request.body.files` - Fixes #75
@mmiszy
Copy link

@mmiszy mmiszy commented Feb 12, 2019

@MarkHerhold this was patched, right? Should someone report to Snyk to close this? https://snyk.io/vuln/npm:koa-body:20180127

@MarkHerhold
Copy link
Contributor

@MarkHerhold MarkHerhold commented Feb 12, 2019

@mmiszy Yes, this was patched in koa-body v3 and v4, which replaced v1 and v2. I didn't know about the related Snyk page and will have to figure out how to get in touch. Thanks for the heads-up.

Also see the README note here: https://github.com/dlau/koa-body#breaking-changes-in-v34

Edit: moved note to CHANGELOG

Also see the CHANGELOG note here: https://github.com/dlau/koa-body/blob/master/CHANGELOG.md#breaking-changes-in-v34

@MarkHerhold
Copy link
Contributor

@MarkHerhold MarkHerhold commented Mar 7, 2019

The Snyk team has now marked the vulnerability as only affecting versions < 3.0.0

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

Successfully merging a pull request may close this issue.

8 participants