Allow for arbitrary Asciidoctor extensions

[gzagatti]

First of all thanks for putting this project together.

I am currently using Asciidoctor to write all the posts for my website. However, I use custom made extensions to render my posts according to my specifications.

Unfortunately, Hugo comes with a hard coded pre-specificed list of allowed Asciidoctor extensions. When Hugo is preparing the Asciidoctor converter it basically checks whether the passed extension is on the list of allowed extension and if not it simply ignores the extension.

I have created a fork that simply removes the check and Hugo is able to use any arbitrary Asciidoctor extension which means I can render my website just by calling hugo.

This behavior is quite puzzling to me as I do not see any reason to not allow the user to determine the extensions which he/she wants to use. Of course, It should be the final user's responsibility to ensure that her/his desired extension is compatible with Hugo. I might be missing something here.Would it be possible to turn off this check on allowed extensions? Is there any reason otherwise.

Thanks again for developing this awesome software.

[xexio]

This will be very helpful. @gzagatti maybe you can add a PR to add this functionality.

[gzagatti]

Sure, will do. Just need some additional time.

[moorereason]

This behavior is quite puzzling to me as I do not see any reason to not allow the user to determine the extensions which he/she wants to use.

See #796, esp. starting with #796 (comment) and below. I doubt we'll merge your PR without safeguards in Hugo first.

[gzagatti]

Thanks for considering the PR and pointing to the right discussion.

With regards to the Asciidoc markup extension implemented in Hugo, it seems to be the case that Hugo is simply checking whether the allowed packages are installed in asciidoctor's execution path. Any extension which has been renamed to agree with these names could thus be executed, alas any arbitrary code could be executed. This is a workaround which I use on my own machine to ensure that I can use Hugo without modification.

The PR thus try to follow Asciidoc specification in honoring the SafeMode directives.

[moorereason]

I don't use asciidoctor, so help me understand. What is "asciidoctor's execution path"? Is it the system search path ($PATH) or a path more exclusive to asciidoctor? Can asciidoctor be used to execute arbitrary executable names in the system path? Can asciidoctor be used to execute scripts/executables within a hugo site?

[bep]

The PR thus try to follow Asciidoc specification in honoring the SafeMode directives.

So, without discussing how potentially unsafe Asciidoctor is (I don't know, and that is certainly beyhond the scope of this PR), I'm having problem seeing how this PR changes this in any direction?

Note that my concerns comes from "me downloading and building a random Hugo site from the internet".

[helfper]

The PR thus try to follow Asciidoc specification in honoring the SafeMode directives.

I don't understand how this honors Asciidoctor specification. I don't see anything in the Asciidoctor documentation saying "use unsafe mode to be able to include any extension that is not allowed by Hugo otherwise".

@bep What if it was an environment variable that cannot be changed in the config.toml?

Current behaviour:

unset HUGO_ASCIIDOCTOR_ALLOWED_EXTENSIONS

Disable all extensions (including the ones in AllowedExtensions):

export HUGO_ASCIIDOCTOR_ALLOWED_EXTENSIONS=

Allow extensions asciidoctor-foo and asciidoctor-bar only:

export HUGO_ASCIIDOCTOR_ALLOWED_EXTENSIONS=asciidoctor-foo,asciidoctor-bar

Allow all extensions:

export HUGO_ASCIIDOCTOR_ALLOWED_EXTENSIONS=*

[gzagatti]

Asciidoctor allows for any arbitrary extension that is passed with the -r flag. Extensions are simply ruby code that is loaded via ruby require from this line in Asciidoctor CLI. In general the code will connect to Asciidoctor extension points, so they are usually classes that inherit from Asciidoctor otherwise they would not be executed while Asciidoctor converts the document.

For instance, my use case is to have a custom converter such as the simplified example below:

require 'asciidoctor'
require 'asciidoctor/converter/html5'

class Converter::MyCustomHtml5Converter < Asciidoctor::Converter::Html5Converter
    register_for 'html_5'

    def convert node, transform = node.node_name, opts = nil
        if transform == 'custom_transform'; return convert_custom node
        else; return super
        end
    end

    def convert_custom node
        'foo'
    end
end

I place it on static/asciidoctor-extension.rb. My config.yaml looks like this:

...
markup:
    asciidocext:
        extensions:
            - ./static/asciidoctor-extension.rb

I am not sure exactly whether Asciidoctor's SafeMode option plays any role on calling these extensions. From browsing their source code and testing it on my computer it does not seem to have any impact. So the document will render with the extensions even if SafeMode is set to secure which is the most strict. I would actually be fine removing this check and go with @helfper or with the configuration I suggested above.

I hope this helps to understand how Asciidoctor works when loading extensions.

[bep]

markup:
    asciidocext:
        extensions:
            - ./static/asciidoctor-extension.rb

The above and variants of the above is not something I'm prepared to allow in Hugo at the moment.

Note that I can sympathize with the argument about "this is my site and I own the source and configuration", but that isn't alway the case. One example would be me downloading arbitrary Hugo sites helping people out with problems on the forum.

[gzagatti]

Fair enough, but as long as there was a method like the one proposed by @helfper in which we can specify the allowed ruby libraries that would be more than enough. For instance, the configuration could go like this:

markup:
    asciidocext:
        extensions:
            - asciidoctor-my-extension

This would fail unless you have installed the ruby package in your system. An arbitrary Hugo site would not be able to install or use an arbitrary ruby extension if you did not install it before.

In my case, it would be a matter of packaging the application and installing on my system. Would that pose significant security risks beyond what is already allowed in the current implementation?

[moorereason]

This would fail unless you have installed the ruby package in your system. An arbitrary Hugo site would not be able to install or use an arbitrary ruby extension if you did not install it before.

Can you demonstrate that for us? If the module is in the root of the hugo site, can you prove that asciidoctor can't locate it? I hate ruby (no, seriously 😆), so I'm not very familiar with it's package lookup process. This is the first time I've really paid much attention to our asciidoctor integration.

If we allowed arbitrary extensions in the site config, how would you recommend that we sanitize the values? No path separators allowed?

[gzagatti]

I did some investigation with regards to ruby require and it seems that it incorporates some basic sanity checks. You can find the official documentation for the require method here.

This link (pointed from Ruby's documentation) contains a discussion about the require method, it states:

Also note that even if you are running current_file.rb from it's containing directory (so that the current working directory of the process is the same as the location of current_file.rb), it is not possible to do something like require 'myfile' although require './myfile' would work. This is because since Ruby 1.9.2, the current working directory is no longer part of the load path. In the UNIX world, this is considered a good security practice since it avoids the issue of accidentally running an unexpected script by simply executing a program from a directory that just happens to have a file with the same name. Since require_relative is requiring relative to the current file instead of the current process, it doesn't have this security issue. That's one more reason to use require_relative for relative requires.

If you did not modify ruby's $LOAD_PATH from its default, it will not load files in your working directory. I performed some checks of my own by trying to build a website with a fake extension called asciidoctor-diagram placed in the root of the hugo site using hugo's master branch. I did not have asciidoctor-diagram installed in my computer. Note that this is an allowed extension and based on the hypothesis that ruby would load this extension because it is placed in the working directory, the website should render successfully. However, it fails to render:

> hugo serve -v
...
INFO 2021/01/13 23:09:22 Rendering posts/50f0be91-05d3-481a-ad65-102a3db51dbc.adoc with /home/gzagatti/.linuxbrew/bin/asciidoctor using asciidoctor args [-r asciidoctor-diagram -r asciidoctor-bibtex -r asciidoctor-katex --no-header-footer --verbose --trace -] ...
ERROR 2021/01/13 23:09:22 about.adoc: <internal:/home/gzagatti/.linuxbrew/lib/ruby/site_ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require': cannot load such file -- asciidoctor-diagram (LoadError)
...

Thus, I think that if we sanitize the extensions by not allowing path separators or dots (ie. . and ..), it would safeguard from malicious execution.

[moorereason]

@gzagatti,
Thanks for looking deeper into this. Ruby 1.9.2 was released in 2010, so that's good news.

Given everything you've detailed here, I think we can work around my security concerns. I've created a POC of what I'm thinking here: https://play.golang.org/p/kolDZ_g8Fw7. Essentially, don't allow any path separators, regardless of the platform. We can possibly fail the site build if a path separator was found instead of silently sanitizing the value.

Cc: @bep

[gzagatti]

@moorereason, thanks for considering this alternative.

I would go even further and disallow dots in the extension name, since they serve no purpose except for calling relative paths. Here is some additional documentation about how Ruby gems should be named. I have modified your POC slightly to suggest how possible extension names should be resolved: https://play.golang.org/p/JQEnoCS1YPZ.

If you're happy with that, I could give it a go and modify the PR to do as you suggested, "fail the site build if a path separator was found instead of silently sanitizing the value".

[moorereason]

I would go even further and disallow dots in the extension name...

Sounds good. 👍

I could give it a go and modify the PR...

Yes, please update the PR.

[gzagatti]

I have updated PR #8131 by allowing only arbitrary asciidoc extensions that do not have path separators (either \, / or .), meaning that these extensions can only be invoked if they are in one's ruby's $LOAD_PATH (ie. most likely installed by the user). Any extension declared relative to the website's path should not be accepted.

I hope this address the issues raised so far. I am looking forward to hearing back.

[gzagatti]

@moorereason @bep I would like to follow up on this conversation and PR #8131 where I incorporated the issues raised in this discussion.

[gzagatti]

Thanks so much! It was a pleasure to contribute with this project.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.