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?