Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Make inserted marker text secure-by-default #2064

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbs228
Copy link

@cbs228 cbs228 commented Sep 7, 2022

Per the documentation, Overviewer configuration authors must take special precautions to escape user-generated HTML and/or JavaScript from marker signs. In mide/minecraft-overviewer#117, we learned that:

  • This detail may escape the notice of downstream packagers and server deployments.

  • It is very straightforward to inject JavaScript into signs on servers where players have permission to use command blocks. It may also be possible on other servers.

Overviewer should conform to MDN's recommendations for safely inserting external content. Failure to protect against injected scripts may allow a malicious actor to gain control of other web resources on the same DNS name (i.e., via session hijacking). In all likelihood, of course, people will just make nuisances of themselves.

A small change in the way text is handled in JavaScript can eliminate the need to invoke html.escape() at all in python. The Leaflet bindPopup() method can accept a DOM node instead of a text string.

  • Leaflet inserts strings via an unsafe innerHTML assignment...
  • ... but DOM nodes are inserted verbatim [source].

To insert content safely, this patch creates a text-only DOM node that contains the marker text. Text nodes are not subject to further interpretation, and all HTML characters appear as literals. We use CSS to permit newline characters to render as line breaks. This preserves multi-line sign text if the configuration author elects to keep it.

This patch is marked RFC because there are a number of outstanding considerations not yet addressed, including:

  • Documentation

  • Examples

  • API-breakage:

    • Configurations which previously escaped text in python will print slightly garbled, escaped output on the map. This is safe and will not error, but it will look wrong.

    • Deployments might want to insert safe HTML into the Leaflet popup.

      • We could enable this by permitting the configuration author to define a template DOM tree into which text nodes can be safely inserted. Inserts should be done using JavaScript which retrieves nodes by ID and sets their textContent.
      • Or we could simply include a configuration flag to permit the user to do things "the old way" with their assurances that they've handled escaping.
    • In any event, we need to consider the upgrade path to the new behavior very carefully.

The following test demonstrates this PR:

  1. Insert an abusive sign into a test world with Op commands:

    /setblock ~1 ~ ~ minecraft:acacia_sign replace
    /data modify block ~1 ~ ~ Text1 set value '{"text":"<style onload=\\"alert(\'test\');\\"/>"}'
  2. Render and --genpoi with a configuration which

    • extracts all signs as markers; and
    • does not escape marker text
  3. Test that you are protected against injection attacks by clicking on the sign marker in Leaflet.

As a bonus, this also harmonizes the look of mouseover tooltip text with the popup text itself. In the most straightforward configurations, python's escape would cause the tooltip to be garbled.

Per the documentation, Overviewer configuration authors must take
special precautions to escape user-generated HTML and/or JavaScript
from marker signs. In mide/minecraft-overviewer#117 [1], we
learned that:

  * This detail may escape the notice of downstream
    packagers and server deployments.

  * It is very straightforward to inject JavaScript
    into signs on Creative servers. It may also be
    possible on Survival servers.

Overviewer should conform to MDN's recommendations for safely
inserting external content [2]. Failure to protect against injected
scripts may allow a malicious actor to gain control of other web
resources on the same DNS name (i.e., via session hijacking). In
all likelihood, of course, people will just make nuisances of
themselves.

A small change in the way text is handled in JavaScript can
eliminate the need to invoke `html.escape()` at all in python. The
Leaflet `bindPopup()` method can accept a DOM node instead of a
text string [3]. The leaflet method inserts text strings via an
*unsafe* `innerHTML` assignment. DOM nodes are inserted
verbatim [4].

This patch creates a text-only DOM node that contains the marker
text. Text nodes are not subject to further interpretation, and all
HTML characters appear as literals. We use CSS to permit newline
characters to render as line breaks. This preserves multi-line
marker text.

References

1. mide/minecraft-overviewer#117

2. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Safely_inserting_external_content_into_a_page

3. https://leafletjs.com/reference.html#layer-bindpopup

4. https://github.com/Leaflet/Leaflet/blob/f10b44b3afcd079febe5219f84dc67c68b379b5e/src/layer/DivOverlay.js#L280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant