Big security vulnerability

[benstevens48]

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]

I finished testing Proof-of-Concept.

[chalkpe]

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]

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

[benstevens48]

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.

[chalkpe]

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]

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]

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]

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

[chalkpe]

@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]

That might be a crash with normal multipart routung.

[chalkpe]

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]

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]

@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 :) https://github.com/dlau/koa-body/pull/77

[katanacrimson]

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

[kingjerod]

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

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

[typeofweb]

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

[MarkHerhold]

@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]

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