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

Stop executing YAML tags for mkdocs_theme.yml, warn about third-party projects #3465

Merged
merged 4 commits into from Nov 21, 2023

Conversation

oprypin
Copy link
Contributor

@oprypin oprypin commented Nov 11, 2023

@jorgectf
This is the outcome of #3418
Which warned about arbitrary code execution via installing a theme and that theme having malicious YAML tags.
I found that there are a lot more ways for arbitrary code execution if you install an untrusted package and import it.
As such, this change only slightly reduces the attack surface, but at the same time I don't really consider it a security issue in the first place, and I don't think it'd be useful for users to see all versions of MkDocs as having a security vulnerability and then also see that this change fixes it (but actually not really).

But I agree:

  • executing YAML tags in mkdocs_theme.yml is pointless.
  • the documentation could definitely use a note about the dangers of third-party packages.

So, the changes are in accordance to that:

  • Stop allowing arbitrary YAML tags for mkdocs_theme.yml
  • Docs: warn about installing third-party plugins

Bonus change:

  • Cache the result of the function get_themes()

Copy link

@jorgectf jorgectf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR, especially for warning about the perils of using external plugins.

@@ -15,6 +15,8 @@ appropriate package name and install it using `pip`:
pip install mkdocs-foo-plugin
```

WARNING: Installing an MkDocs plugin means installing a Python package and executing any code that the author has put in there. So, exercise the usual caution; there's no attempt at sandboxing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WARNING: Installing an MkDocs plugin means installing a Python package and executing any code that the author has put in there. So, exercise the usual caution; there's no attempt at sandboxing.
WARNING: By installing an MkDocs plugin you are installing a Python package and executing any code embedded by the author. There's no attempt at sandboxing, so exercise the usual caution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I like my wording better :s

@@ -196,6 +196,8 @@ theme supports the following options:

A list of third party themes can be found at the [community wiki] page and [the ranked catalog][catalog]. If you have created your own, please add them there.

WARNING: Installing an MkDocs theme means installing a Python package and executing any code that the author has put in there. So, exercise the usual caution; there's no attempt at sandboxing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WARNING: Installing an MkDocs theme means installing a Python package and executing any code that the author has put in there. So, exercise the usual caution; there's no attempt at sandboxing.
WARNING: By installing an MkDocs plugin you are installing a Python package and executing any code embedded by the author. There's no attempt at sandboxing, so exercise the usual caution.

@oprypin oprypin merged commit 646987d into master Nov 21, 2023
30 checks passed
@oprypin oprypin deleted the secu branch November 21, 2023 20:46
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.

None yet

2 participants