[Users] That won't work.

[Dave Howorth]

On Tue, 13 Oct 2020 23:49:26 +0200
Michal Suchánek <msuchanek at suse.de> wrote:

> On Tue, Oct 13, 2020 at 09:52:43PM +0100, Dave Howorth wrote:
> > On Tue, 13 Oct 2020 21:25:09 +0200
> > Michael Rasmussen via Users <users at lists.claws-mail.org> wrote:
> >   
> > > On Tue, 13 Oct 2020 16:58:57 +0100
> > > Dave Howorth <dave at howorth.org.uk> wrote:
> > > 
> > > The problem here is that it is very difficult to know want an
> > > individual person accept as legitimit input (one mans roof is
> > > another mans floor ;-) so to solve it requires some kind of input
> > > validation which uses either a black list or a white list - for
> > > security reasons I would prefer a white list.  
> > 
> > I don't think that is the problem here. The problem is that the
> > invoking program (claws) invokes a shell and passes it stringified
> > arguments (presumably prepended by the stringified command).
> > 
> > If you'd care to tell me how to locate that invocation in the code,
> > I will propose a solution (i.e. a patch).  
> 
> Here is a backtrace of Claws applying a template

[snip]

> The code is hidden in quote_fmt_parse.y and is very difficult to
> follow. At least inserting breakpoints at fork() and clone() crashes
> claws so you can tell what it was doing to get there.

Thanks Michal for that very helpful reply :) I've now found the
offending system call, which is on line 524 of quote_fmt_parse.y in my
version:

static void quote_fmt_insert_program_output(const gchar *progname)
{
	FILE *file;
	char buffer[256];

	if ((file = popen(progname, "r")) != NULL) {
		while (fgets(buffer, sizeof(buffer), file)) {
			INSERT(buffer);
		}
		pclose(file);
	}
}

It's the popen() call and for anybody still reading who still doesn't
understand the problem I've also found a good explanation of the risk
at
https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152177

"Do not call system()
... The C Standard system() function executes a specified command by
invoking an implementation-defined command processor, such as a UNIX
shell ... The POSIX popen() ... functions also invoke a command
processor ...

Use of the system() function can result in exploitable vulnerabilities,
in the worst case allowing execution of arbitrary system commands. ..."

and as we have demonstrated, the case here is not the worst case (no
root privilege) but is quite bad.

As has already been suggested the cure is to use execvp() or similar.
Now given that implies some restrictions on what the user can do, I can
imagine some backwards-compatibility arguments against a change, so I
suspect the best way forward is to add a new option that uses execvp()
and in addition to documenting it, add a warning to the documentation
of the existing facility about the risks involved in using it. Then
people who want to be safe are able to be safe and people who think
they're fine and we're wittering nonsense can continue to use the
existing facility.

I'm stopping for today now. I'll have another look tomorrow.

Cheers, Dave

PS It's not too complicated although I admit it's been a few years since
I last looked at YACC code.

> Thanks
> 
> Michal