Security implications of loading custom reports (was: Re: Custom reports do not load after upgrade to 2.6.4 on OSX)

[John Ralls]

On Oct 21, 2014, at 9:27 AM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:

> I have moved this thread from gnucash-user as it was getting rather technical...
>  
> On Tuesday 21 October 2014 08:43:04 John Ralls wrote:
> > > > 
> > > > One possible solution is to get Guile out of the file-opening
> > > > loop.
> > > > Having a configuration file that’s directly executed by Guile is a
> > > > rather glaring security hole; it would be much better to make it
> > > > an
> > > > ini-style file and do everything in C/C++ up to passing the
> > > > contents
> > > > of the files to a Guile function that registers them as add-on
> > > > reports. That’s not really a suitable change for the maint branch,
> > > > but we should consider it for 2.8.
> > > 
> > > I understand your reasoning. However a custom report in itself is
> > > essentially unrestricted scheme code as well. So how is loading
> > > such a file via C/C++ any less of a glaring security hole than
> > > having it done in guile ?
> > It’s not if the C/C++ code just passes arbitrary code to Guile for
> > immediate execution. I’m proposing that the custom report should be
> > passed to the report-registration code instead so that the report
> > must be selected from the menu to execute.
>  
> I'm not sure this is possible in guile only. A report is written as a guile module. Loading the module already executes code (gnc:define-report). That code can be abused do bad things when loading a custom report.

Wow. That’s an incredible failure for something that’s promoted as an application scripting language.

>  
> Thinking further on your .ini file we could perhaps indeed split the current report in two parts:
> - an ini file as you propose which holds the parameters we currently set in the call to gnc:define-report
> - a guile module with all the functions that can be called by the report.
> That would reduce the execution of arbitrary code to when the report is effectively called.
>  
> Note that our built-in reports use the same loading mechanism and for user-friendliness we probably want to keep only one load interface. Which means we would want to convert our built-in reports as well.
>  
> Next if the goal is to prevent arbitrary code execution we will also have to rewrite the saved-report code, because it is also running a guile file from DOT_GNUCASH_DIR. And loading of stylesheets.
>  
> By now it starts to look like a rewrite of the report system. Which I'm not sure I would want to do in guile again…

That might be overstating it a bit, though it would require a fair amount of surgery to convert all of the existing reports. That might be doable with a bit of perl to extract the data into an ini-style file and remove the block from each report’s scheme file.

Not that I like the current system: I’d prefer a report definition consisting of a query for the data feeding canned graphing and table-drawing functions with CSS to make it pretty. Depending on how the query part is implemented it might run the risk of SQL injection, but that’s less of a risk than allowing arbitrary code as we do now. I would prefer that such a thing be written in a different language from Scheme.

Regards,
John Ralls