Fix xss due to handlebars variables in javascript

[mnutt]

Fixes #532.

Using handlebars within javascript is pretty difficult to protect against, so this sidesteps it by using javascript to pull the query param off the page location and append to the RTL URL.

I used a regex because I thought perhaps this page was aiming for compatibility with very old browsers. If that's not the case, we could parse using URLSearchParams or something.

[mnutt]

@petrsloup is there any interest in this PR?

[acalcutt]

I'm guessing this is still an issue in my template, since I didn't protect key. Do you think this fix still should be fixed this way?

Is this one of those things you were asking about doing on the frontend?

[mnutt]

Yes, a frontend framework would resolve this but I think the most straightforward fix is probably just applying the above. I can rebase it off of latest upstream.

[mnutt]

@acalcutt it's rebased and the merge conflicts are resolved.

[mancontr]

I think that the PR does not cover all the places where the problem happens. The same changes are needed on data.tmpl too. Also, lines 10 and 11 on both template files also use key_query unsafely, and would need to be fixed.

Vulnerability scanners are picking this issue already, so it would be awesome if the PR could be updated and merged...

[acalcutt]

@mnutt , do you want to take a look at the other template file mentioned?

Do you still think regex is the way to go with this if it is needed in two places? Didn't we use a sanitize plugin somewhere else?

[mnutt]

What do you think of using URL() here instead of a regex? Released on all modern browsers as of 2014 so it seems fairly safe.

My hope is that we can actually just get rid of all of the instances where we merge the key into the static css/js URLs, although there may be some reason for it I don't yet understand.

[acalcutt]

I'd say in the current release, one of the aims was to maintain internet explorer compatibility... so I'm not sure URL() would work there would it?

On the topic of IE support though, maybe in a next PR we can start getting rid of IE support in these templates and simplify them a bit. Maplibre-gl currently works in IE using my combability library which would be nice to get rid of and leaflet is on their last version with IE support (next release drops IE). Maybe this would be a 5.x release since it dropping IE support is a major change I think.

I do think for now, for this PR though, maybe we should wait on URL() and put it in the dropping IE PR.

[mnutt]

Makes sense, I'll switch back to regex for the time being.

[acalcutt]

That {&key_query} is used all through the head of the viewer template too... hmm

[mnutt]

It looks like the templating engine knows a little bit about XSS and is smart enough to escape {{key_query}} when it's actually inside an HTML attribute. The only really dangerous one is where we use a script tag to choose which script to inject, because to the templating engine just sees it as a javascript string and leaves it alone. I've removed {{key_query}} from all of the static assets and it looks like the XSS hole is closed.

[acalcutt]

Thanks, I think it makes sense to remove it like you did. As for the injecting script, that is part of the IE support that I would like to remove in a future PR.

[acalcutt]

I wish I knew more about how key is used in real life in relation to tileserver-gl... can tileserver-gl require a key right now?

[acalcutt]

The only scenario I can think of where not having the key could fail is if you are behind some type of proxy server that requires the key to access the assets... (like haproxy/nginx level blocking). Like if you were blocking access to your own server by key.

[vinayakkulkarni]

The only scenario I can think of where not having the key could fail is if you are behind some type of proxy server that requires the key to access the assets... (like haproxy/nginx level blocking). Like if you were blocking access to your own server by key.

but even then, key key in the key=value key-pair should be dynamic ?

[vinayakkulkarni]

Thanks, I think it makes sense to remove it like you did. As for the injecting script, that is part of the IE support that I would like to remove in a future PR.

Maybe this is the remanent code which maptiler/server uses key for?

[acalcutt]

The only scenario I can think of where not having the key could fail is if you are behind some type of proxy server that requires the key to access the assets... (like haproxy/nginx level blocking). Like if you were blocking access to your own server by key.

but even then, key key in the key=value key-pair should be dynamic ?

I was more talking about the urls in head we are removing the key from. If the key was required to access those files, then not having it would break the view pages.

For example, the documentation at https://maptiler-tileserver.readthedocs.io/en/latest/deployment.html#securing suggests Nginx can be used to add protection via https, password, referrer, IP address restriction, access keys, etc.

[mnutt]

Hmm, I had assumed that meant nginx could do that stuff as its own thing rather than anything relating to the query param.

Currently it looks like the only thing the app does with key query param is to receive it as a query param and reflect it into the template?

[acalcutt]

I'm honestly not sure if that was the intent. I have seen that before where you need the key to access any resource.

I'm just thinking, if tileserver was behind a proxy like that had required "access keys", you may need the keys to access any file hosted in that server, like those files in head. So if your were accessing one of these pages and specified an access key, it would need to pass it to the other resources on the page so it could access those files.

That could be why this was added here, since that is similar to what their pay service does.

[mancontr]

It's likely it was used as a key for cache busting purposes, so it can be changed whenever it was needed to have browsers reload all assets. This is a very common usage for such a parameter.