Feature request: add a flag to disable converter template helpers loading to prevent arbitrary code execution

[yann-soubeyrand]

Hi,

When using Hugo with Asciidoctor, we’re trying to see if we could allow Hugo theme authors to define converter templates to customize the HTML generated by Asciidoctor: gohugoio/hugo#12314. In this context, we cannot take the risk of theme authors being able to execute arbitrary code on the user environment.

We think that with the Ruby implementation of Asciidoctor, tilt and Handlebars, it’s OK (we don’t see ways of executing arbitrary code for the moment), though we’d like to be sure of it. But, it’s clearly not OK with Asciidoctor.js and Handlebars, because of helpers: https://docs.asciidoctor.org/asciidoctor.js/latest/extend/converter/template-converter/#helpers-js-file.

I didn’t find a way to disallow these helpers and only allow templates and partials. Do you think it could be possible to add a flag to disable helpers loading?

[ggrossetie]

Hey!

I thought Hugo was using Asciidoctor (Ruby) CLI?

Do you think it could be possible to add a flag to disable helpers loading?

Do we need to a CLI flag?
We could use template_engine_options but it's only available from the API.
It can be tempting to disable converter template helpers using the safe modes but I'm a bit reluctant...

[ggrossetie]

We think that with the Ruby implementation of Asciidoctor, tilt and Handlebars, it’s OK (we don’t see ways of executing arbitrary code for the moment), though we’d like to be sure of it

I don't think that's true.
Asciidoctor requires a helpers.rb file, so if the content of this file is malicious, such as:

require 'fileutils'

FileUtils.rm_rf("/")

Then, it will perform this action.
@mojavelinux What do you think?

[yann-soubeyrand]

Hello,

Thanks for your reply.

I thought Hugo was using Asciidoctor (Ruby) CLI?

Hugo calls asciidoctor binary, so it can call the JS version too. I personally prefer this approach, because I find it easier to work with npm compared to bundler and because I often need npm anyway to fetch other dependencies.

I don't think that's true.
Asciidoctor requires a helpers.rb file, so if the content of this file is malicious, such as:

require 'fileutils'

FileUtils.rm_rf("/")

Then, it will perform this action.

Sadly, we missed that point.

[jmooring]

With inline_anchor.html.handlebars as the converter template...

require 'fileutils'
FileUtils.rm("/home/jmooring/temp/test-a.txt")
<a class="foo" href="{{ target }}" {{#if attributes.title}}title="{{ attributes.title }}"{{/if}}>{{ text }}</a>

...the file is not deleted (that's a good thing).

How can I delete the file from a handlebars converter template? If there's a vulnerability in our use case, I want to prove it.

[ggrossetie]

Add a helpers.rb file in your template directory:

https://github.com/asciidoctor/asciidoctor/blob/935a0a3a2fe05ba7d239d8e774ada20df5879599/lib/asciidoctor/converter/template.rb#L200-L206

[jmooring]

Before adding the --template-dir CLI flag, we reject the directory if it contains any files with extensions other than .handlebars. Is that sufficient to mitigate this vulnerability?

[jmooring]

@ggrossetie If we reject a --template-dir path when it contains anything other than .handlebars files, is there any other way that someone can execute arbitrary code?

[ggrossetie]

If we reject a --template-dir path when it contains anything other than .handlebars files, is there any other way that someone can execute arbitrary code?

I'm not a security expect but I guess that would prevent Asciidoctor from loading the helpers.rb. What do you mean by "reject"? the --template-dir directory won't contain the file?

[jmooring]

What do you mean by "reject"?

In Hugo's configuration file, the user would provide an array of converter template directory paths. Before passing those paths as --template-dir flag/value pairs, we will scan each directory. If the directory contains anything other than .handlebars files we won't add that flag/value pair to the asciidoctor command line when we render the page.