TTLabs / EvaporateJS

Javascript library for browser to S3 multipart resumable uploads
1.82k stars 206 forks source link

Security questions #425

Open LadislavBohm opened 5 years ago

LadislavBohm commented 5 years ago

Hello I have couple of questions regarding security as I was not able to find clear answers in documentation of this library or AWS.

  1. Do I have any control over what filename is going to be uploaded to S3? I noticed that if I upload file that already exists in S3 (has same filename), evaporate is going to overwrite it. That means if more users are uploading from client side, they could overwrite each others file since I have no server-side control over filename that is going to be uploaded (it is generated on the client). Is this correct?

  2. I can see that V4 signature is generated from datestamp, does that mean that this signature is not valid after the date expires? Is the V4 signature usable even after multipart upload completes?

  3. Does the V4 signature only allow user to upload a specific chunk to specific file or can he use this signature to potentially upload completely different file to different location (perhaps numerous files, using single signature)? Can he use this signature to delete other files etc.?

  4. As far as I understand, there is generally no way to know what is going to uploaded and how big the file is going to be? I can limit content/type and file size using pre-signed URLs and they generally seem like something more suitable for direct uploads to S3 from client side.

Thank you

paulmorrishill commented 5 years ago

Hi, I am currently implementing this library in one of my projects and I believe that concerns you have raised are completely valid. As far as I understand it the default implementation suggested by the wiki is highly vulnerable to malicious use.

Essentially the proposed signing mechanism is to trust that a URL generated on the client should be signed and to sign it without confirming anything about it.

I have implemented a more secure mechanism by sending the canonical request (which this library does expose to the custom signing function) and generating the URL to be signed on the server side. This approach means you don't need to send anything except the canonical request to the server.

When you generate the URL server side based on the canonical request you are able to parse the request and confirm that the file upload is pointing to the bucket and key that you explicitly want and deny the request in the case it does not.

The file name can be configured client side by passing the name value into the add function. This could be generated server side by a separate web request before actually beginning the upload. This combined with the above signing approach prevents file operations on anything but the S3 bucket and key you have authorized them to use.

jakubzitny commented 5 years ago

To @LadislavBohm:

  1. Yes.
  2. Yes. No.
  3. No.
  4. Where would you like to know the size and destination? On your backend? The custom signing mechanism (via customAuthMethod) is a better way to sign and control more things. The best way is to call your backend (or lambda) that modifies the access details on s3 and custom-signs the request and only start the upload after that.

Regarding the filenames, it would be a better practice to have users upload to separate paths on s3. And you can rename the files on the client, by creating a new file object with a new name. But you can also change the name directly in [Evaporate#add](https://github.com/TTLabs/EvaporateJS/wiki/Evaporate.prototype.add()) config object.

We are using the customAuthMethod and I'd be happy to prepare a simplified example of that, it takes a lot of time to do clearly and properly though. You can submit your examples as well though 😉

RomanHotsiy commented 5 years ago

@jakubzitny would be awesome if you can prepare an example.

I understand it takes lots of time though. I would be happy to open a PR if you give any clues on controlling filenames on signing server.

I've checked all the params of customAuthMethod and looks like there is no s3 key among them. Also, I'm not sure if the key is included in stringToSign in some form. Without that I can't be sure my signature won't be used to upload as the other key.

Thanks in advance!

jakubzitny commented 5 years ago

We pass the s3 key from our backend via/to customAuthMethod and the key is created for a specific upload, with precise size limit and location on s3 where the backend allowed it to be uploaded. That's exactly the purpose of the customAuthMethod. For it to be save you have to generate the stuff on your backend (or lambda), if it's on client it does not make sense, but you can try it with a static key.

I've already drafted an example in #402. Have a look at it.

And for the filenames, I already shared the link above: [Evaporate#add](https://github.com/TTLabs/EvaporateJS/wiki/Evaporate.prototype.add()). Where is the problem?

RomanHotsiy commented 5 years ago

@jakubzitny thanks. Now I got it after reading some source code and AWS docs.

I just wanted to skip implementing string-to-sign and canonical request construction on the server.

So I ended up sending both stringToSign and canonicalRequest to the server and parsing canonicalRequest to verify S3 Key (also I verify canonicalRequest is correct by hash it and comparing to its hash from stringToSign).

That way I omit reimplementing canonicalRequest construction.