search

3scale / apisonator

Red Hat 3scale API Management Apisonator backend
https://3scale.net
Apache License 2.0
36 stars 27 forks source link

Disable path traversal protection on internal API to allow slashes in app IDs #366

Closed mayorova closed 4 months ago

mayorova commented 4 months ago

This is related to https://github.com/3scale/pisoni/pull/33

Currently, when app IDs or app keys have slashes in them, either / or \, the API doesn't work as intended. This is because rack-protection's path traversal

so, the ID one/two\three (in Ruby "one/two\three") is encoded by pisoni correctly into one%2Ftwo%5Cthree, but the path traversal decodes encoded slashes, and also replaces \ with / before routing takes place, so the path ends up completely wrong. So, for example for an app with app ID and app key which is one/two\three, the path gets converted from

/internal/services/500/applications/one%2Ftwo%5Cthree/keys/one%2Ftwo%5Cthree

to

/internal/services/500/applications/one/two/thee/keys/one/two/thee

I believe disabling path traversal protection is only an issue when the application serves static files. I think we are pretty confident that this would not happen on the backend Internal API, so I think that's OK.

Alternatively, we can just disallow both backward and forward slashes (the forward ones are already disallowed in porta), but it may not be ideal in case OpenID connect is used with an SSO provider that generates client secrets with special characters including slashes.

akostadinov commented 4 months ago

Do we have a "public" endpoint? It might be worth checking whether we have and whether it relies on path protection. But would be really lame in case serving files relies on this path traversal "protection".

akostadinov commented 4 months ago

But this exception seems to affect only internal API. Not the whole server. So should be legit either way.

mayorova commented 4 months ago

I wouldn't merge this until we verify that Apisonator doesn't serve static files.

@jlledom Well, my reasoning is:

  1. Sinatra docs (search for :static) say that static pages are enabled by default when public directory exists. public directory does not exist, and public_folder is not set either. Also, the inspection of the code shows that it is true, and :static would be set to false in our case.

  2. It looks like even without protection, trying to access files that are outside of the public folder would result in 404, as per this test

So, I'd say I am confident enough that for the internal API (and for the rest of the API on apisonator) we are not using static files, and disabling path traversal protection will not affect security.