On 2007/11/17 13:54, Stuart Henderson wrote:
> - I am not too happy about this lot:
>
> char comm[4096];
> snprintf(comm,4096,"%s %s %s",MOVEIT,p->mailfile,config->virusdirbase);
> if(system(comm)) do_log(LOG_CRIT,"ERR: move");
> snprintf(comm,4096,"%s %s/p3scan.*",CHMODCMD,config->virusdirbase);
> do_log(LOG_DEBUG,"Forcing all files 0600 %s",comm);
> if(system(comm)) do_log(LOG_CRIT,"ERR: chmod");
> snprintf(comm,4096,"cat %s | %s -s '[Virus] found in a mail to %s'
> %s", mailx, config->mail, paramlist_get(p->params,
> "%USERNAME%"),config->extra);
> if(system(comm)) do_log(LOG_CRIT,"ERR: mailx");
> snprintf(comm,4096,"cat %s | %s -s '[Virus] found in a mail to %s'
> %s", mail, config->mail, paramlist_get(p->params,
> "%USERNAME%"),config->extra);
> if(system(comm)) do_log(LOG_CRIT,"ERR mail");
>
This might bear a little explanation..
For the move/chmod it doesn't look terrible, but it would be
better to just call rename(2)/chmod(2). Looks like in the 2.9
development versions these are gone completely.
The latter two calls, for sending mail, are nastier; an untrusted
user-provided string is passed directly to the shell (yes, system()
uses /bin/sh) and it doesn't appear to be cleaned first.
Using popen("/usr/sbin/sendmail -t") and feeding the headers
and the message down the file handle would be better. This does
still use the shell but only passes it a fixed string. Avoiding
the shell completely by pipe/fork/exec is another option.
scanner_bash also really should be cleaning strings which it
passes to the user-provided shell script. I'd just disable
that, I don't think it's especially useful - most people are
just going to be using clamav anyway.
Anyone else got input?
