Fix xss due to handlebars variables in javascript #535

mnutt · 2021-04-08T01:24:44Z

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 · 2022-04-13T14:27:10Z

@petrsloup is there any interest in this PR?

acalcutt · 2022-09-23T04:04:01Z

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 · 2022-09-23T23:58:03Z

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 · 2022-09-24T00:05:17Z

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

mancontr · 2023-01-17T07:28:53Z

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 · 2023-01-17T12:48:20Z

@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 · 2023-01-18T04:52:29Z

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 · 2023-01-18T14:19:00Z

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 · 2023-01-18T15:06:42Z

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

acalcutt · 2023-01-18T15:26:15Z

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

mnutt · 2023-01-18T15:48:50Z

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 · 2023-01-18T15:53:38Z

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 · 2023-01-18T15:57:32Z

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 · 2023-01-18T16:22:13Z

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 · 2023-01-18T18:34:42Z

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 · 2023-01-18T18:36:00Z

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 · 2023-01-18T18:54:30Z

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 · 2023-01-18T20:01:23Z

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 · 2023-01-18T20:47:22Z

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 · 2023-01-18T21:30:23Z

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.

Signed-off-by: Michael Nutt <michael@nuttnet.net>

vinayakkulkarni · 2023-01-18T15:26:26Z

public/templates/viewer.tmpl

    if (preference == 'vector') {
-      maplibregl.setRTLTextPlugin('{{public_url}}mapbox-gl-rtl-text.js{{&key_query}}');
+      maplibregl.setRTLTextPlugin('{{public_url}}mapbox-gl-rtl-text.js' + keyParam);


Suggested change

maplibregl.setRTLTextPlugin('{{public_url}}mapbox-gl-rtl-text.js' + keyParam);

maplibregl.setRTLTextPlugin(`{{public_url}}mapbox-gl-rtl-text.js${keyParam}`);

vinayakkulkarni · 2023-01-18T15:26:45Z

public/templates/viewer.tmpl

      var map = new maplibregl.Map({
        container: 'map',
-        style: '{{public_url}}styles/{{id}}/style.json{{&key_query}}',
+        style: '{{public_url}}styles/{{id}}/style.json' + keyParam,


Suggested change

style: '{{public_url}}styles/{{id}}/style.json' + keyParam,

style: `{{public_url}}styles/{{id}}/style.json${keyParam}`,

vinayakkulkarni · 2023-01-18T15:27:38Z

public/templates/viewer.tmpl

@@ -49,7 +54,7 @@
 		new L.Control.Zoom({ position: 'topright' }).addTo(map);

 		var tile_urls = [], tile_attribution, tile_minzoom, tile_maxzoom;
-		var url = '{{public_url}}styles/{{id}}.json{{&key_query}}';
+		var url = '{{public_url}}styles/{{id}}.json' + keyParam;


Suggested change

var url = '{{public_url}}styles/{{id}}.json' + keyParam;

var url = `{{public_url}}styles/{{id}}.json${keyParam}`;

vinayakkulkarni · 2023-01-18T15:27:52Z

public/templates/data.tmpl

 	var map = L.map('map', { zoomControl: false });
 	new L.Control.Zoom({ position: 'topright' }).addTo(map);

 	var tile_urls = [], tile_attribution, tile_minzoom, tile_maxzoom;
-	var url = '{{public_url}}data/{{id}}.json{{&key_query}}';
+	var url = '{{public_url}}data/{{id}}.json' + keyParam;


Suggested change

var url = '{{public_url}}data/{{id}}.json' + keyParam;

var url = `{{public_url}}data/{{id}}.json${keyParam}`;

vinayakkulkarni · 2023-01-18T15:30:00Z

public/templates/data.tmpl

@@ -76,11 +80,15 @@
  <h1 style="display:none;">{{name}}</h1>
  <div id='map'></div>
  <script>
+  const { searchParams } = new URL(document.location);
+  const accessKey = searchParams.get('key');
+  const keyParam = accessKey ? `?key=${accessKey}` : '';


I think the ? would cause issue cause it could be another search param already present, then that would be:

base_url/index.json?path=...&?key=accessKey, instead we should check if there are any searchParams and then add ?key=${accessKey} if none else &key=${accessKey}

vinayakkulkarni · 2023-01-18T15:30:26Z

public/templates/data.tmpl

@@ -44,7 +48,7 @@
      sources: {
        'vector_layer_': {
          type: 'vector',
-          url: '{{public_url}}data/{{id}}.json{{&key_query}}'
+          url: '{{public_url}}data/{{id}}.json' + keyParam


Suggested change

url: '{{public_url}}data/{{id}}.json' + keyParam

url: `{{public_url}}data/{{id}}.json${keyParam}`

vinayakkulkarni · 2023-01-25T11:45:18Z

public/templates/data.tmpl

+  <link rel="stylesheet" type="text/css" href="{{public_url}}maplibre-gl.css" />
+  <link rel="stylesheet" type="text/css" href="{{public_url}}maplibre-gl-inspect.css" />
+  <script>if (typeof Symbol !== 'undefined') { document.write('<script src="{{public_url}}maplibre-gl.js"><\/script>'); } else { document.write('<script src="{{public_url}}maplibre-gl-compat.js"><\/script>'); }</script>
+  <script>if (typeof Symbol !== 'undefined') { document.write('<script src="{{public_url}}maplibre-gl-inspect.min.js"><\/script>'); } else { document.write('<script src="{{public_url}}maplibre-gl-inspect-compat.min.js"><\/script>'); }</script>


Uhh, this is causing issue with serving the static assets via a proxy (Kong) 😭 as query has a custom access_key which is required in each request served.

mnutt force-pushed the viewer-xss branch from b84d1bf to 3860690 Compare September 24, 2022 00:04

acalcutt force-pushed the viewer-xss branch from 3860690 to e3638a5 Compare January 15, 2023 03:04

acalcutt force-pushed the viewer-xss branch from e3638a5 to 2172de3 Compare January 17, 2023 12:46

mnutt force-pushed the viewer-xss branch from e879ecd to c71269e Compare January 18, 2023 15:42

mnutt requested a review from acalcutt January 18, 2023 15:49

mnutt added 3 commits January 18, 2023 21:18

fix xss due to handlebars variables in javascript

ce36be8

fix: update data viewer against XSS

072b1d2

Signed-off-by: Michael Nutt <michael@nuttnet.net>

fix: remove key_query from all static assets to prevent XSS

75868ca

Signed-off-by: Michael Nutt <michael@nuttnet.net>

acalcutt force-pushed the viewer-xss branch from 2c719b1 to 75868ca Compare January 19, 2023 02:18

acalcutt approved these changes Jan 19, 2023

View reviewed changes

acalcutt merged commit 78c1777 into maptiler:master Jan 19, 2023

vinayakkulkarni reviewed Jan 25, 2023

View reviewed changes

vinayakkulkarni mentioned this pull request Jan 25, 2023

Issue with query params for static assets in template viewer #729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix xss due to handlebars variables in javascript #535

Fix xss due to handlebars variables in javascript #535

mnutt commented Apr 8, 2021

mnutt commented Apr 13, 2022

acalcutt commented Sep 23, 2022

mnutt commented Sep 23, 2022

mnutt commented Sep 24, 2022

mancontr commented Jan 17, 2023 •

edited

Loading

acalcutt commented Jan 17, 2023 •

edited

Loading

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023 •

edited

Loading

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

vinayakkulkarni commented Jan 18, 2023

vinayakkulkarni commented Jan 18, 2023

acalcutt commented Jan 18, 2023

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023 •

edited

Loading

mancontr commented Jan 18, 2023

vinayakkulkarni Jan 18, 2023

vinayakkulkarni Jan 18, 2023

vinayakkulkarni Jan 18, 2023

vinayakkulkarni Jan 18, 2023

vinayakkulkarni Jan 18, 2023

vinayakkulkarni Jan 18, 2023

vinayakkulkarni Jan 25, 2023

	maplibregl.setRTLTextPlugin('{{public_url}}mapbox-gl-rtl-text.js' + keyParam);
	maplibregl.setRTLTextPlugin(`{{public_url}}mapbox-gl-rtl-text.js${keyParam}`);

	style: '{{public_url}}styles/{{id}}/style.json' + keyParam,
	style: `{{public_url}}styles/{{id}}/style.json${keyParam}`,

	var url = '{{public_url}}styles/{{id}}.json' + keyParam;
	var url = `{{public_url}}styles/{{id}}.json${keyParam}`;

	var url = '{{public_url}}data/{{id}}.json' + keyParam;
	var url = `{{public_url}}data/{{id}}.json${keyParam}`;

	url: '{{public_url}}data/{{id}}.json' + keyParam
	url: `{{public_url}}data/{{id}}.json${keyParam}`

Fix xss due to handlebars variables in javascript #535

Fix xss due to handlebars variables in javascript #535

Conversation

mnutt commented Apr 8, 2021

mnutt commented Apr 13, 2022

acalcutt commented Sep 23, 2022

mnutt commented Sep 23, 2022

mnutt commented Sep 24, 2022

mancontr commented Jan 17, 2023 • edited Loading

acalcutt commented Jan 17, 2023 • edited Loading

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023 • edited Loading

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023

vinayakkulkarni commented Jan 18, 2023

vinayakkulkarni commented Jan 18, 2023

acalcutt commented Jan 18, 2023

mnutt commented Jan 18, 2023

acalcutt commented Jan 18, 2023 • edited Loading

mancontr commented Jan 18, 2023

vinayakkulkarni Jan 18, 2023

Choose a reason for hiding this comment

vinayakkulkarni Jan 18, 2023

Choose a reason for hiding this comment

vinayakkulkarni Jan 18, 2023

Choose a reason for hiding this comment

vinayakkulkarni Jan 18, 2023

Choose a reason for hiding this comment

vinayakkulkarni Jan 18, 2023

Choose a reason for hiding this comment

vinayakkulkarni Jan 18, 2023

Choose a reason for hiding this comment

vinayakkulkarni Jan 25, 2023

Choose a reason for hiding this comment

mancontr commented Jan 17, 2023 •

edited

Loading

acalcutt commented Jan 17, 2023 •

edited

Loading

acalcutt commented Jan 18, 2023 •

edited

Loading

acalcutt commented Jan 18, 2023 •

edited

Loading