Skip to content

Commit

Permalink
Fixes A Coverity TAINTED_SCALAR issue
Browse files Browse the repository at this point in the history 
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.
```
  • Loading branch information
@IlanTruanovsky
IlanTruanovsky committed Feb 8, 2023
1 parent 0eabaa9 commit a37cf9a
Showing 1 changed file with 30 additions and 44 deletions.
74 changes: 30 additions & 44 deletions lib/pkg_editor/src/pkg_editor.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit a37cf9a

Please # to comment.