[Users] That won't work.

[wwp]

Hello Dave,


On Wed, 14 Oct 2020 22:39:54 +0100 Dave Howorth <dave at howorth.org.uk> wrote:

> 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.

Nice catch. Even capturing the program output in a 256-byte buffer
seems a bit short! In the other popen call func, we use MAX_PATH, which
seems more reasonable.


Regards,

-- 
wwp
https://useplaintext.email/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.claws-mail.org/pipermail/users/attachments/20201015/11a6e7c5/attachment.sig>