From a37cf9a1803c78a5f6658aa05a104add2e3385c3 Mon Sep 17 00:00:00 2001 From: Ilan Truanovsky Date: Wed, 8 Feb 2023 06:48:24 -0800 Subject: [PATCH] Fixes A Coverity `TAINTED_SCALAR` issue The info.file_length value was never checked. Since this was being passed into a malloc call, it would be possible for the malloc call to use too much memory if the info.file_length number was purposefully set to be large. There were two possible solutions to this that I could think of: - Add an upper bound to the size of info.file_length. This maintains much of the same code, but also increases the restrictions of the data being passed into the acl_pkg_unpack_buffer_or_file function. - Do not malloc an array of length info.file_length and instead reuse a constant sized array. This is the solution I implemented. The unit tests did not indicate any performance problems due to having to loop over and reuse a constant sized array. Fixes: ``` lib/pkg_editor/src/pkg_editor.c:1681:11: Type: Untrusted allocation size (TAINTED_SCALAR) lib/pkg_editor/src/pkg_editor.c:1591:3: Tainted data flows to a taint sink 1. path: Condition "buffer != NULL", taking false branch. lib/pkg_editor/src/pkg_editor.c:1596:5: 2. path: Condition "input != NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1596:5: 3. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1601:3: 4. path: Condition "ret != 0", taking false branch. lib/pkg_editor/src/pkg_editor.c:1612:3: 5. path: Condition "z_info.strm.avail_in > 0", taking false branch. lib/pkg_editor/src/pkg_editor.c:1612:3: 6. path: Condition "input != NULL", taking true branch. lib/pkg_editor/src/pkg_editor.c:1612:3: 7. path: Condition "!feof(input)", taking true branch. lib/pkg_editor/src/pkg_editor.c:1614:5: 8. path: Condition "!read_data(&info, 20UL /* sizeof (info) */, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1619:5: 9. path: Condition "info.magic != 3203399403U", taking false branch. lib/pkg_editor/src/pkg_editor.c:1627:5: 10. path: Condition "info.kind == PACK_END", taking false branch. lib/pkg_editor/src/pkg_editor.c:1632:5: 11. path: Condition "!read_data(name, info.name_length, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1642:5: 12. path: Condition "out_dir_length + 2 > 12288UL /* 3 * 4096 */", taking false branch. lib/pkg_editor/src/pkg_editor.c:1652:5: 13. path: Condition "info.kind == PACK_DIR", taking true branch. lib/pkg_editor/src/pkg_editor.c:1654:5: 14. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1711:3: 15. path: Jumping back to the beginning of the loop. lib/pkg_editor/src/pkg_editor.c:1612:3: 16. path: Condition "z_info.strm.avail_in > 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1614:5: 17. path: Condition "!read_data(&info, 20UL /* sizeof (info) */, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1619:5: 18. path: Condition "info.magic != 3203399403U", taking false branch. lib/pkg_editor/src/pkg_editor.c:1627:5: 19. path: Condition "info.kind == PACK_END", taking false branch. lib/pkg_editor/src/pkg_editor.c:1632:5: 20. path: Condition "!read_data(name, info.name_length, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1642:5: 21. path: Condition "out_dir_length + 2 > 12288UL /* 3 * 4096 */", taking false branch. lib/pkg_editor/src/pkg_editor.c:1652:5: 22. path: Condition "info.kind == PACK_DIR", taking true branch. lib/pkg_editor/src/pkg_editor.c:1654:5: 23. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1711:3: 24. path: Jumping back to the beginning of the loop. lib/pkg_editor/src/pkg_editor.c:1612:3: 25. path: Condition "z_info.strm.avail_in > 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1614:5: 26. tainted_argument: Calling function "read_data" taints argument "info". lib/pkg_editor/src/pkg_editor.c:1530:3: Tainted data flows to a taint sink 26.1. var_assign_parm: Assigning: "z_info->strm.next_out" = "data". lib/pkg_editor/src/pkg_editor.c:1534:5: 26.2. path: Condition "z_info->strm.avail_in == 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1537:7: 26.3. path: Condition "in_fd == NULL", taking false branch. lib/pkg_editor/src/pkg_editor.c:1537:7: 26.4. path: Condition "feof(in_fd)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1541:7: 26.5. tainted_data_argument: Calling function "fread" taints parameter "*z_info->buffer". [Note: The source code implementation of the function has been overridden by a builtin model.] lib/pkg_editor/src/pkg_editor.c:1542:7: 26.6. path: Condition "count < 1", taking false branch. lib/pkg_editor/src/pkg_editor.c:1547:7: 26.7. var_assign_alias: Assigning: "z_info->strm.next_in" = "z_info->buffer", which taints "z_info->strm.next_in". lib/pkg_editor/src/pkg_editor.c:1550:5: 26.8. tainted_data_transitive: Calling function "inflate" with tainted argument "*z_info->strm.next_in" taints "*z_info->strm.next_out". lib/pkg_editor/src/pkg_editor.c:1551:5: 26.9. path: Condition "ret != -2", taking true branch. lib/pkg_editor/src/pkg_editor.c:1551:5: 26.10. path: Falling through to end of if statement. lib/pkg_editor/src/pkg_editor.c:1552:5: 26.11. path: Condition "ret == 1", taking true branch. lib/pkg_editor/src/pkg_editor.c:1554:7: 26.12. path: Condition "z_info->strm.avail_out == 0", taking false branch. lib/pkg_editor/src/pkg_editor.c:1614:5: 27. path: Condition "!read_data(&info, 20UL /* sizeof (info) */, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1619:5: 28. path: Condition "info.magic != 3203399403U", taking false branch. lib/pkg_editor/src/pkg_editor.c:1627:5: 29. path: Condition "info.kind == PACK_END", taking false branch. lib/pkg_editor/src/pkg_editor.c:1632:5: 30. path: Condition "!read_data(name, info.name_length, &z_info, input)", taking false branch. lib/pkg_editor/src/pkg_editor.c:1642:5: 31. path: Condition "out_dir_length + 2 > 12288UL /* 3 * 4096 */", taking false branch. lib/pkg_editor/src/pkg_editor.c:1652:5: 32. path: Condition "info.kind == PACK_DIR", taking false branch. lib/pkg_editor/src/pkg_editor.c:1657:7: 33. path: Condition "out_file == NULL", taking false branch. lib/pkg_editor/src/pkg_editor.c:1663:7: 34. path: Condition "info.file_length > 0", taking true branch. lib/pkg_editor/src/pkg_editor.c:1663:7: 35. lower_bounds: Checking lower bounds of unsigned scalar "info.file_length" by taking the true branch of "info.file_length > 0U". lib/pkg_editor/src/pkg_editor.c:1665:9: 36. path: Condition "info.file_length < 65536UL /* sizeof (buf) */", taking false branch. lib/pkg_editor/src/pkg_editor.c:1665:9: 37. lower_bounds: Checking lower bounds of unsigned scalar "info.file_length" by taking the false branch of "info.file_length < 65536UL". lib/pkg_editor/src/pkg_editor.c:1681:11: 38. tainted_data: Passing tainted expression "info.file_length" to "malloc", which uses it as an allocation size. lib/pkg_editor/src/pkg_editor.c:1681:11: 39. remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range. ``` --- lib/pkg_editor/src/pkg_editor.c | 74 +++++++++++++-------------------- 1 file changed, 30 insertions(+), 44 deletions(-) diff --git a/lib/pkg_editor/src/pkg_editor.c b/lib/pkg_editor/src/pkg_editor.c index 4e61143b..daf9e374 100644 --- a/lib/pkg_editor/src/pkg_editor.c +++ b/lib/pkg_editor/src/pkg_editor.c @@ -1668,51 +1668,37 @@ static int acl_pkg_unpack_buffer_or_file(const char *buffer, size_t buffer_size, inflateEnd(&z_info.strm); return 0; } - if (info.file_length > 0) { - char buf[64 * 1024]; - if (info.file_length < sizeof(buf)) { - if (!read_data(buf, info.file_length, &z_info, input)) { - fprintf(stderr, "%s: Error reading file data for %s from buffer\n", - routine_name, full_name); - fclose(out_file); - inflateEnd(&z_info.strm); - return 0; - } - if (fwrite(buf, info.file_length, 1, out_file) != 1) { - fprintf(stderr, "%s: Failed to write to %s: %s\n", routine_name, - full_name, strerror(errno)); - fclose(out_file); - inflateEnd(&z_info.strm); - return 0; - } - } else { - char *buf2 = malloc(info.file_length); - if (buf2 == NULL) { - fprintf(stderr, "%s: Failed to allocate buffer to write %s: %s\n", - routine_name, full_name, strerror(errno)); - fclose(out_file); - free(buf2); - inflateEnd(&z_info.strm); - return PACK_END; - } - if (!read_data(buf2, info.file_length, &z_info, input)) { - fprintf(stderr, "%s: Error reading file data for %s from buffer\n", - routine_name, full_name); - fclose(out_file); - free(buf2); - inflateEnd(&z_info.strm); - return 0; - } - if (fwrite(buf2, info.file_length, 1, out_file) != 1) { - fprintf(stderr, "%s: Failed to write to %s: %s\n", routine_name, - full_name, strerror(errno)); - fclose(out_file); - free(buf2); - inflateEnd(&z_info.strm); - return 0; - } - free(buf2); + fclose(out_file); + out_file = fopen(full_name, "ab"); + if (out_file == NULL) { + fprintf(stderr, "%s: Unable to open %s for appending: %s\n", + routine_name, full_name, strerror(errno)); + inflateEnd(&z_info.strm); + return 0; + } + char buf[64 * 1024]; + size_t left_to_read = info.file_length; + for (;;) { + size_t num_to_read = + sizeof(buf) < left_to_read ? sizeof(buf) : left_to_read; + if (!read_data(buf, num_to_read, &z_info, input)) { + fprintf(stderr, "%s: Error reading file data for %s from buffer\n", + routine_name, full_name); + fclose(out_file); + inflateEnd(&z_info.strm); + return 0; + } + if (fwrite(buf, num_to_read, 1, out_file) != 1) { + fprintf(stderr, "%s: Failed to write to %s: %s\n", routine_name, + full_name, strerror(errno)); + fclose(out_file); + inflateEnd(&z_info.strm); + return 0; + } + if (left_to_read <= sizeof(buf)) { + break; } + left_to_read -= sizeof(buf); } fclose(out_file); }