open-dsi
/
clamav
mirror of https://github.com/Cisco-Talos/clamav
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix several coverity warnings

Browse Source 
290424 Missing break in switch - In hash_match: Missing break
statement between cases in switch statement

290414 Resource leak - In cli_scanishield_msi: Leak of memory or
pointers to system resources. Memory leak in a fail case

288197 Resource leak - In decrypt_any: Leak of memory or pointers
to system resources. Memory leak in a fail case

290426 Resource leak - In cli_magic_scan: Leak of memory or pointers
to system resources. Leaked a file prefix when running with
--save-temps

192923 Resource leak - In cli_scanrar: Leak of memory or pointers to
system resources. Leaked a file descriptor if a virus was found in
a RAR file comment

225146 Resource leak - In cli_scanegg: Leak of memory or pointers
to system resources. Leaked a file descriptor if unable to write
a comment file to disk

290425 Resource leak - In scan_common: Leak of memory or pointers
to system resources. Memory leaks in various fail cases.

Also changes cli_scanrar to write out the file comment only if
--leave-temps is specified and scan the buffer (like what is done
in cli_scanegg) instead of writing the file out, scanning that,
and then deleting the file if --leave-temps is not specified.

The unit tests stopped working when correcting an issue with a
switch statement that determined what type of signature had matched
on a Google SafeBrowsing GDB rule. Looking into the unit tests, it
looks like the code had always assumed that the test cases would be
detected by a malware test rule in unit_tests/input/daily.gdb, but
now some of the tests get matched on the phishing test rule.
I updated the test logic to be more clear, and added tests for both
cases now.

Fix some memory leaks in libclamav/scanners.c
pull/125/head
Andrew 5 years ago committed by Micah Snyder (micasnyd)
parent e830b45ca7
commit 319bfb51a5
7 changed files with 129 additions and 70 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 7
      libclamav/ishield.c
  2. 5
      libclamav/pdf.c
  3. 3
      libclamav/phishcheck.c
  4. 3
      libclamav/regex_list.c
  5. 130
      libclamav/scanners.c
  6. 41
      unit_tests/check_regex.c
  7. 10
      unit_tests/input/daily.gdb

7
libclamav/ishield.c
Unescape View File

@ -241,7 +241,12 @@ int cli_scanishield_msi(cli_ctx *ctx, off_t off)
/* FIXMEISHIELD: cleanup the spam below */
cli_dbgmsg("ishield-msi: File %s (csize: %llx, unk1:%x unk2:%x unk3:%x unk4:%x unk5:%x unk6:%x unk7:%x unk8:%x unk9:%x unk10:%x unk11:%x)\n", key, (long long)csize, fb.unk1, fb.unk2, fb.unk3, fb.unk4, fb.unk5, fb.unk6, fb.unk7, fb.unk8, fb.unk9, fb.unk10, fb.unk11);
if (!(tempfile = cli_gentemp(ctx->sub_tmpdir))) return CL_EMEM;
if (!(tempfile = cli_gentemp(ctx->sub_tmpdir))) {
if (NULL != filename) {
free(filename);
}
return CL_EMEM;
}
if ((ofd = open(tempfile, O_RDWR | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR)) < 0) {
cli_dbgmsg("ishield-msi: failed to create file %s\n", tempfile);
free(tempfile);

5
libclamav/pdf.c
Unescape View File

@ -1265,6 +1265,7 @@ char *decrypt_any(struct pdf_struct *pdf, uint32_t id, const char *in, size_t *l
memcpy(q, in, *length);
if (false == arc4_init(&arc4, result, n)) {
noisy_warnmsg("decrypt_any: failed to init arc4\n");
free(q);
return NULL;
}
arc4_apply(&arc4, q, (unsigned)*length); /* TODO: may truncate for very large lengths */
@ -1281,10 +1282,6 @@ char *decrypt_any(struct pdf_struct *pdf, uint32_t id, const char *in, size_t *l
break;
case ENC_AESV3:
cli_dbgmsg("decrypt_any: enc is aesv3\n");
if (pdf->keylen == 0) {
cli_dbgmsg("decrypt_any: no key\n");
return NULL;
}
aes_256cbc_decrypt((const unsigned char *)in, length, q, pdf->key, pdf->keylen, 1);

3
libclamav/phishcheck.c
Unescape View File

@ -1208,10 +1208,13 @@ static cl_error_t hash_match(const struct regex_matcher* rlist,
break;
case '1':
*phishing_verdict = CL_PHISH_HASH1;
break;
case '2':
*phishing_verdict = CL_PHISH_HASH2;
break;
default:
*phishing_verdict = CL_PHISH_HASH0;
break;
}
}
}

3
libclamav/regex_list.c
Unescape View File

@ -472,6 +472,9 @@ cl_error_t load_regex_matcher(struct cl_engine *engine, struct regex_matcher *ma
if (!*buffer)
continue; /* skip empty lines */
if (buffer[0] == '#')
continue;
if (functionality_level_check(buffer))
continue;

130
libclamav/scanners.c
Unescape View File

@ -272,37 +272,35 @@ static cl_error_t cli_scanrar(const char *filepath, int desc, cli_ctx *ctx)
/* If the archive header had a comment, write it to the comment dir. */
if ((comment != NULL) && (comment_size > 0)) {
int comment_fd = -1;
if (!(comment_fullpath = cli_gentemp_with_prefix(ctx->sub_tmpdir, "comments"))) {
status = CL_EMEM;
goto done;
}
comment_fd = open(comment_fullpath, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
if (comment_fd < 0) {
cli_dbgmsg("RAR: ERROR: Failed to open output file\n");
} else {
cli_dbgmsg("RAR: Writing the archive comment to temp file: %s\n", comment_fullpath);
if (0 == write(comment_fd, comment, comment_size)) {
cli_dbgmsg("RAR: ERROR: Failed to write to output file\n");
} else {
/* Scan the comment file */
status = cli_magic_scan_file(comment_fullpath, ctx, NULL);
/* Delete the tempfile if not --leave-temps */
if (!ctx->engine->keeptmp)
if (cli_unlink(comment_fullpath))
cli_dbgmsg("RAR: Failed to unlink the extracted comment file: %s\n", comment_fullpath);
if (ctx->engine->keeptmp) {
int comment_fd = -1;
if (!(comment_fullpath = cli_gentemp_with_prefix(ctx->sub_tmpdir, "comments"))) {
status = CL_EMEM;
goto done;
}
if ((status == CL_VIRUS) && SCAN_ALLMATCHES) {
status = CL_CLEAN;
viruses_found++;
}
if ((status == CL_VIRUS) || (status == CL_BREAK)) {
goto done;
comment_fd = open(comment_fullpath, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
if (comment_fd < 0) {
cli_dbgmsg("RAR: ERROR: Failed to open output file\n");
} else {
cli_dbgmsg("RAR: Writing the archive comment to temp file: %s\n", comment_fullpath);
if (0 == write(comment_fd, comment, comment_size)) {
cli_dbgmsg("RAR: ERROR: Failed to write to output file\n");
}
close(comment_fd);
}
close(comment_fd);
}
/* Scan the comment */
status = cli_magic_scan_buff(comment, comment_size, ctx, NULL);
if ((status == CL_VIRUS) && SCAN_ALLMATCHES) {
status = CL_CLEAN;
viruses_found++;
}
if ((status == CL_VIRUS) || (status == CL_BREAK)) {
goto done;
}
}
@ -657,7 +655,6 @@ static cl_error_t cli_scanegg(cli_ctx *ctx, size_t sfx_offset)
goto done;
}
free(prefix);
prefix = NULL;
comment_fd = open(comment_fullpath, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0600);
if (comment_fd < 0) {
@ -666,10 +663,8 @@ static cl_error_t cli_scanegg(cli_ctx *ctx, size_t sfx_offset)
cli_dbgmsg("EGG: Writing the archive comment to temp file: %s\n", comment_fullpath);
if (0 == write(comment_fd, comments[i], nComments)) {
cli_dbgmsg("EGG: ERROR: Failed to write to output file\n");
} else {
close(comment_fd);
comment_fd = -1;
}
close(comment_fd);
}
free(comment_fullpath);
comment_fullpath = NULL;
@ -3471,7 +3466,6 @@ cl_error_t cli_magic_scan(cli_ctx *ctx, cli_file_t type)
void *parent_property = NULL;
#endif
char *fmap_basename = NULL;
char *old_temp_path = NULL;
char *new_temp_path = NULL;
@ -3509,6 +3503,7 @@ cl_error_t cli_magic_scan(cli_ctx *ctx, cli_file_t type)
}
if (ctx->engine->keeptmp) {
char *fmap_basename = NULL;
/*
* Keep-temp enabled, so create a sub-directory to provide extraction directory recursion.
*/
@ -3518,6 +3513,7 @@ cl_error_t cli_magic_scan(cli_ctx *ctx, cli_file_t type)
* The fmap has a name, lets include it in the new sub-directory.
*/
new_temp_path = cli_gentemp_with_prefix(ctx->sub_tmpdir, fmap_basename);
free(fmap_basename);
if (NULL == new_temp_path) {
cli_errmsg("cli_magic_scan: Failed to generate temp directory name.\n");
ret = CL_EMEM;
@ -4582,6 +4578,7 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
time_t current_time;
struct tm tm_struct;
fmap_t **fmap_head = NULL;
if (NULL == map) {
return CL_ENULLARG;
@ -4599,17 +4596,21 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
memcpy(ctx.options, scanoptions, sizeof(struct cl_scan_options));
ctx.found_possibly_unwanted = 0;
ctx.containers = cli_calloc(sizeof(cli_ctx_container), ctx.engine->maxreclevel + 2);
if (!ctx.containers)
return CL_EMEM;
if (!ctx.containers) {
rc = CL_EMEM;
goto done;
}
cli_set_container(&ctx, CL_TYPE_ANY, 0);
ctx.dconf = (struct cli_dconf *)engine->dconf;
ctx.cb_ctx = context;
ctx.fmap = cli_calloc(sizeof(fmap_t *), ctx.engine->maxreclevel + 3);
if (!ctx.fmap)
return CL_EMEM;
fmap_head = cli_calloc(sizeof(fmap_t *), ctx.engine->maxreclevel + 3);
if (!fmap_head) {
rc = CL_EMEM;
goto done;
}
if (!(ctx.hook_lsig_matches = cli_bitset_init())) {
free(ctx.fmap);
return CL_EMEM;
rc = CL_EMEM;
goto done;
}
/*
@ -4618,7 +4619,7 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
* fmap's file descriptor in the virus found callback (like for deferred
* low-seveerity alerts).
*/
ctx.fmap++;
ctx.fmap = fmap_head + 1;
*ctx.fmap = map;
perf_init(&ctx);
@ -4658,7 +4659,8 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
if (!localtime_r(&current_time, &tm_struct)) {
#endif
cli_errmsg("scan_common: Failed to get local time.\n");
return CL_ESTAT;
rc = CL_ESTAT;
goto done;
}
if ((ctx.engine->keeptmp) &&
@ -4669,7 +4671,8 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
new_temp_prefix = cli_calloc(1, new_temp_prefix_len + 1);
if (!new_temp_prefix) {
cli_errmsg("scan_common: Failed to allocate memory for temp directory name.\n");
return CL_EMEM;
rc = CL_EMEM;
goto done;
}
strftime(new_temp_prefix, new_temp_prefix_len, "%Y%m%d_%H%M%S-", &tm_struct);
strcpy(new_temp_prefix + strlen("YYYYMMDD_HHMMSS-"), target_basename);
@ -4679,7 +4682,8 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
new_temp_prefix = cli_calloc(1, new_temp_prefix_len + 1);
if (!new_temp_prefix) {
cli_errmsg("scan_common: Failed to allocate memory for temp directory name.\n");
return CL_EMEM;
rc = CL_EMEM;
goto done;
}
strftime(new_temp_prefix, new_temp_prefix_len, "%Y%m%d_%H%M%S-scantemp", &tm_struct);
}
@ -4689,14 +4693,16 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
free(new_temp_prefix);
if (NULL == new_temp_path) {
cli_errmsg("scan_common: Failed to generate temp directory name.\n");
return CL_EMEM;
rc = CL_EMEM;
goto done;
}
ctx.sub_tmpdir = new_temp_path;
if (mkdir(ctx.sub_tmpdir, 0700)) {
cli_errmsg("Can't create temporary directory for scan: %s.\n", ctx.sub_tmpdir);
return CL_EACCES;
rc = CL_EACCES;
goto done;
}
cli_logg_setup(&ctx);
@ -4801,6 +4807,9 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
}
}
cli_logg_unsetup();
done:
if (NULL != ctx.sub_tmpdir) {
if (!ctx.engine->keeptmp) {
(void)cli_rmdirs(ctx.sub_tmpdir);
@ -4808,16 +4817,33 @@ static cl_error_t scan_common(cl_fmap_t *map, const char *filepath, const char *
free(ctx.sub_tmpdir);
}
if (NULL != target_basename) {
free(target_basename);
}
if (NULL != ctx.target_filepath) {
free(ctx.target_filepath);
}
free(ctx.containers);
cli_bitset_free(ctx.hook_lsig_matches);
ctx.fmap--; /* Restore original fmap pointer */
free(ctx.fmap);
free(ctx.options);
cli_logg_unsetup();
perf_done(&ctx);
if (NULL != ctx.perf) {
perf_done(&ctx);
}
if (NULL != ctx.hook_lsig_matches) {
cli_bitset_free(ctx.hook_lsig_matches);
}
if (NULL != fmap_head) {
free(fmap_head);
}
if (NULL != ctx.containers) {
free(ctx.containers);
}
if (NULL != ctx.options) {
free(ctx.options);
}
return rc;
}

41
unit_tests/check_regex.c
Unescape View File

@ -219,11 +219,16 @@ static const struct rtest {
"http://www.paypal.com", "pics.ebay.com", RTR_WHITELISTED},
{NULL, "http://somefakeurl.example.com", "someotherdomain-key.com", RTR_CLEAN},
{NULL, "http://somefakeurl.example.com", "someotherdomain.key.com", RTR_PHISH},
{NULL, "http://1.test.example.com/something", "test", RTR_BLACKLISTED},
{NULL, "http://1.test.example.com/2", "test", RTR_BLACKLISTED},
{NULL, "http://user@1.test.example.com/2", "test", RTR_BLACKLISTED},
{NULL, "http://user@1.test.example.com/2/test", "test", RTR_BLACKLISTED},
{NULL, "http://user@1.test.example.com/", "test", RTR_BLACKLISTED},
{NULL, "http://malware-test.example.com/something", "test", RTR_BLACKLISTED},
{NULL, "http://phishing-test.example.com/something", "test", RTR_BLACKLISTED},
{NULL, "http://sub.malware-test.example.com/2", "test", RTR_BLACKLISTED},
{NULL, "http://sub.phishing-test.example.com/2", "test", RTR_BLACKLISTED},
{NULL, "http://user@malware-test.example.com/2", "test", RTR_BLACKLISTED},
{NULL, "http://user@phishing-test.example.com/2", "test", RTR_BLACKLISTED},
{NULL, "http://user@malware-test.example.com/2/test", "test", RTR_BLACKLISTED},
{NULL, "http://user@phishing-test.example.com/2/test", "test", RTR_BLACKLISTED},
{NULL, "http://user@malware-test.example.com/", "test", RTR_BLACKLISTED},
{NULL, "http://user@phishing-test.example.com/", "test", RTR_BLACKLISTED},
{NULL, "http://x.exe", "http:///x.exe", RTR_CLEAN},
{".+\\.ebayrtm\\.com([/?].*)?:[^.]+\\.ebay\\.(de|com|co\\.uk)/",
"http://srx.main.ebayrtm.com",
@ -430,8 +435,17 @@ static void do_phishing_test(const struct rtest *rtest)
"this should be blacklisted, realURL: %s, displayURL: %s",
rtest->realurl, rtest->displayurl);
if (*ctx.virname) {
char *phishingFound = strstr((const char *)*ctx.virname, "Heuristics.Safebrowsing.Suspected-malware_safebrowsing.clamav.net");
ck_assert_msg(phishingFound != NULL, "\n\t should be: Heuristics.Safebrowsing.Suspected-malware_safebrowsing.clamav.net,\n\t but is: %s\n", *ctx.virname);
char *phishingFound = NULL;
char *detectionName = NULL;
if (strstr(rtest->realurl, "malware-test")) {
detectionName = "Heuristics.Safebrowsing.Suspected-malware_safebrowsing.clamav.net";
} else if (strstr(rtest->realurl, "phishing-test")) {
detectionName = "Heuristics.Safebrowsing.Suspected-phishing_safebrowsing.clamav.net";
}
ck_assert_msg(detectionName != NULL, "\n\t Blacklist test case error - malware-test or phishing-test not found in: %s\n", rtest->realurl);
phishingFound = strstr((const char *)*ctx.virname, detectionName);
ck_assert_msg(phishingFound != NULL, "\n\t should be: %s,\n\t but is: %s\n", detectionName, *ctx.virname);
}
}
break;
@ -512,8 +526,17 @@ static void do_phishing_test_allscan(const struct rtest *rtest)
"this should be blacklisted, realURL: %s, displayURL: %s",
rtest->realurl, rtest->displayurl);
if (*ctx.virname) {
char *phishingFound = strstr((const char *)*ctx.virname, "Heuristics.Safebrowsing.Suspected-malware_safebrowsing.clamav.net");
ck_assert_msg(phishingFound != NULL, "\n\t should be: Heuristics.Safebrowsing.Suspected-malware_safebrowsing.clamav.net,\n\t but is: %s\n", *ctx.virname);
char *phishingFound = NULL;
char *detectionName = NULL;
if (strstr(rtest->realurl, "malware-test")) {
detectionName = "Heuristics.Safebrowsing.Suspected-malware_safebrowsing.clamav.net";
} else if (strstr(rtest->realurl, "phishing-test")) {
detectionName = "Heuristics.Safebrowsing.Suspected-phishing_safebrowsing.clamav.net";
}
ck_assert_msg(detectionName != NULL, "\n\t Blacklist test case error - malware-test or phishing-test not found in: %s\n", rtest->realurl);
phishingFound = strstr((const char *)*ctx.virname, detectionName);
ck_assert_msg(phishingFound != NULL, "\n\t should be: %s,\n\t but is: %s\n", detectionName, *ctx.virname);
}
}
break;

10
unit_tests/input/daily.gdb
Unescape View File

@ -1,4 +1,6 @@
S:P:d1b8a025
S:F:d1b8a0251d7555d016b6468ae623e4b1e830c7efccc54966d09447a3d0a85c60
S2:P:7f6fd541
S2:F:7f6fd541e625e7bc5d5a64f166e47ecfe13735464a74d160b48265c162a71089
# Google Safe Browsing Malware Site rule for malware-test.example.com/
S:P:4d8bef53
S:F:4d8bef53100a92ef3b9490dc234c6c4a6d0f45bf73cb13b166188cd802c1a080
# Google Safe Browsing Phishing Site rule for phishing-test.example.com/
S2:P:f396c58e
S2:F:f396c58e768d8b9ddc6a21bc17c90be9a68e019d964c8003699ee17b3e4dd4b6

Write Preview
Loading…
Cancel
Save