oxyno-zeta / s3-proxy

S3 Reverse Proxy with GET, PUT and DELETE methods and authentication (OpenID Connect and Basic Auth)
https://oxyno-zeta.github.io/s3-proxy/
Apache License 2.0
310 stars 34 forks source link

Provide a way to disable folder listing #370

Closed chrispiyux closed 1 year ago

chrispiyux commented 1 year ago

We are using your component in our application and during a pentest it was hightlighted that XSS vulnerabilities are present in folder listing and 404 template. We were able to modify the templates in order to avoid these but as an additionnal step, we would like to be able to disable folder listing as a global parameter. I could see it was present in your inspiration project pottava/aws-s3-proxy, maybe would be good to re-integrate it.

Describe the solution you'd like By providing a config parameter or environment variable, folder listing would be disabled and return 403

Describe alternatives you've considered Today we have updated folder-listing.tpl template and returned HTTP status code 403

Additional context NA

oxyno-zeta commented 1 year ago

Hello @chrispiyux ,

Thanks for the issue well described !

The problem I have with this is the following: why disabling the folder listing should return a 403 error ? I'm pretty sure that once this feature will be done like that, someone will say that it should return a 404 (and I'm more aligned with this by the way).

Changing the template and the status code is a good way to go I think. It allows more things. For example, you can return the folder for you as admin and a 403/404 for other people.

I can make an opinionated feature to return a 404 for sure but I must say I dislike those... A lot of things can be customized in the application for that reason.

I can also write a full documentation page on how to disable file listing with templates.

What do you think about this ?

Oxyno-zeta

chrispiyux commented 1 year ago

Hi Alexandre, thanks a lot for your quick answer. I was proposing 403 because it's what i remember IIS is doing when folder listing is disable, but i'm maybe wrong. 404 would actually be also ok.

As i said, yes we achieved to fix the issue by updating the template, but security has requested also to remove the listBuckets permission for the IAM role used by S3 proxy. Hence instead of reponding with the custom template, S3 proxy throw a 500 error which i'm not fan of.

That's why, if we could manage the directory browsing disabling before S3 proxy tries to list the bucket files it would really be great. Thanks a lot Christophe

oxyno-zeta commented 1 year ago

Hello Christophe,

Changing the template doesn't exclude the fact the application will make the request. That's why you have a 500 when you remove the right in the policy.

I've a better view now. I will try to work on this in the coming weeks and find a way to avoid having an opinionated answer.

Oxyno-zeta

chrispiyux commented 1 year ago

Hello Alexandre, thanks a lot, really looking forward to have this feature.

oxyno-zeta commented 1 year ago

Hello @chrispiyux ,

The good news is that I think I found a way to make it available without any opinionated option. Now I need to find a couple of hours to work on it...

I just wanted to tell you that I don't forget this subject.

chrispiyux commented 1 year ago

Hi @oxyno-zeta

very good news ! Thanks a lot

oxyno-zeta commented 1 year ago

Hello @chrispiyux ,

As I'm not sure I will be able to finish the new version (I want to change few other things too), I will make you a 4.11 alpha version for you (oxynozeta/s3-proxy:4.11.0-alpha.0-amd64).

I will build it soon.

The documentation is deployed too.

Tell me if it is working well please ;) .

Have a nice day !

chrispiyux commented 1 year ago

Hello @oxyno-zeta ,

thanks a lot, i will take a look and do some testing and keep you updated.

Thanks

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days