From 94a18f283972b276150cdf72fef47e56512c11d7 Mon Sep 17 00:00:00 2001 From: Phil Pennock Date: Sun, 5 Sep 2010 16:29:07 -0400 Subject: [PATCH 1/1] Rework clamd response handling to be more robust. In particular, clamd's ExtendedDetectionInfo option broke our parsing. --- src/src/malware.c | 122 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 86 insertions(+), 36 deletions(-) diff --git a/src/src/malware.c b/src/src/malware.c index 6e8b3f36d..eb0c33cea 100644 --- a/src/src/malware.c +++ b/src/src/malware.c @@ -1298,7 +1298,7 @@ static int malware_internal(uschar **listptr, uschar *eml_filename, BOOL faking) uschar *clamd_options; uschar clamd_options_buffer[1024]; uschar clamd_options_default[] = "/tmp/clamd"; - uschar *p,*vname; + uschar *p, *vname, *result_tag, *response_end; struct sockaddr_un server; int sock,bread=0; unsigned int port; @@ -1338,6 +1338,15 @@ static int malware_internal(uschar **listptr, uschar *eml_filename, BOOL faking) else use_scan_command = FALSE; + /* See the discussion of response formats below to see why we really don't + like colons in filenames when passing filenames to ClamAV. */ + if (use_scan_command && Ustrchr(eml_filename, ':')) { + log_write(0, LOG_MAIN|LOG_PANIC, + "malware acl condition: clamd: local/SCAN mode incompatible with" \ + " : in path to email filename [%s]", eml_filename); + return DEFER; + } + /* socket does not start with '/' -> network socket */ if (*clamd_options != '/') { @@ -1615,10 +1624,25 @@ static int malware_internal(uschar **listptr, uschar *eml_filename, BOOL faking) return DEFER; } - /* Check the result. ClamAV Returns - infected: -> ": FOUND" - not-infected: -> ": OK" - error: -> ": ERROR */ + /* Check the result. ClamAV returns one of two result formats. + In the basic mode, the response is of the form: + infected: -> ": FOUND" + not-infected: -> ": OK" + error: -> ": ERROR + If the ExtendedDetectionInfo option has been turned on, then we get: + ": (:) FOUND" + for the infected case. Compare: +/tmp/eicar.com: Eicar-Test-Signature FOUND +/tmp/eicar.com: Eicar-Test-Signature(44d88612fea8a8f36de82e1278abb02f:68) FOUND + + In the streaming case, clamd uses the filename "stream" which you should + be able to verify with { ktrace clamdscan --stream /tmp/eicar.com }. (The + client app will replace "stream" with the original filename before returning + results to stdout, but the trace shows the data). + + We will assume that the pathname passed to clamd from Exim does not contain + a colon. We will have whined loudly above if the eml_filename does (and we're + passing a filename to clamd). */ if (!(*av_buffer)) { log_write(0, LOG_MAIN|LOG_PANIC, @@ -1626,50 +1650,76 @@ static int malware_internal(uschar **listptr, uschar *eml_filename, BOOL faking) return DEFER; } - /* strip newline at the end (won't be present for zINSTREAM) */ + /* strip newline at the end (won't be present for zINSTREAM) + (also any trailing whitespace, which shouldn't exist, but we depend upon + this below, so double-check) */ p = av_buffer + Ustrlen(av_buffer) - 1; - if( *p == '\n' ) *p = '\0'; + if (*p == '\n') *p = '\0'; DEBUG(D_acl) debug_printf("Malware response: %s\n", av_buffer); + while (isspace(*--p) && (p > av_buffer)) + *p = '\0'; + if (*p) ++p; + response_end = p; + /* colon in returned output? */ - if((p = Ustrrchr(av_buffer,':')) == NULL) { + if((p = Ustrchr(av_buffer,':')) == NULL) { log_write(0, LOG_MAIN|LOG_PANIC, - "malware acl condition: clamd: ClamAV returned malformed result: %s", + "malware acl condition: clamd: ClamAV returned malformed result (missing colon): %s", av_buffer); return DEFER; } /* strip filename */ - ++p; - while (*p == ' ') ++p; + while (*p && isspace(*++p)) /**/; vname = p; - if ((p = Ustrstr(vname, "FOUND"))!=NULL) { - *p=0; - for (--p;p>vname && *p<=32;p--) *p=0; - for (;*vname==32;vname++); - Ustrcpy(malware_name_buffer,vname); - malware_name = malware_name_buffer; - DEBUG(D_acl) debug_printf("Malware found, name \"%s\"\n", malware_name); - } - else { - if (Ustrstr(vname, "ERROR")!=NULL) { - /* ClamAV reports ERROR - Find line start */ - for (;*vname!='\n' && vname>av_buffer; vname--); - if (*vname=='\n') vname++; - - log_write(0, LOG_MAIN|LOG_PANIC, - "malware acl condition: clamd: ClamAV returned %s",vname); - return DEFER; - } - else { - /* Everything should be OK */ - malware_name = NULL; - DEBUG(D_acl) debug_printf("Malware not found\n"); - } + + /* It would be bad to encounter a virus with "FOUND" in part of the name, + but we should at least be resistant to it. */ + p = Ustrrchr(vname, ' '); + if (p) + result_tag = p + 1; + else + result_tag = vname; + + if (Ustrcmp(result_tag, "FOUND") == 0) { + /* p should still be the whitespace before the result_tag */ + while (isspace(*p)) --p; + *++p = '\0'; + /* Strip off the extended information too, which will be in parens + after the virus name, with no intervening whitespace. */ + if (*--p == ')') { + /* "(hash:size)", so previous '(' will do; if not found, we have + a curious virus name, but not an error. */ + p = Ustrrchr(vname, '('); + if (p) + *p = '\0'; + } + Ustrncpy(malware_name_buffer, vname, sizeof(malware_name_buffer)-1); + malware_name = malware_name_buffer; + DEBUG(D_acl) debug_printf("Malware found, name \"%s\"\n", malware_name); + + } else if (Ustrcmp(result_tag, "ERROR") == 0) { + log_write(0, LOG_MAIN|LOG_PANIC, + "malware acl condition: clamd: ClamAV returned: %s", + av_buffer); + return DEFER; + + } else if (Ustrcmp(result_tag, "OK") == 0) { + /* Everything should be OK */ + malware_name = NULL; + DEBUG(D_acl) debug_printf("Malware not found\n"); + + } else { + log_write(0, LOG_MAIN|LOG_PANIC, + "malware acl condition: clamd: unparseable response from ClamAV: {%s}", + av_buffer); + return DEFER; } - } + + } /* clamd */ + /* ----------------------------------------------------------------------- */ -- 2.30.2