lcp / mokutil

The utility to manipulate machine owner keys
GNU General Public License v3.0
60 stars 37 forks source link

Fix an off-by-one reading passwords from a file. #79

Closed vathpela closed 4 months ago

vathpela commented 5 months ago

One of our static checkers noticed the following:

  Error: OVERRUN (CWE-119):
  mokutil-0.6.0/src/mokutil.c:378: cond_at_least: Checking ""read_len < 300L"" implies that ""read_len"" is at least 300 on the false branch.
  mokutil-0.6.0/src/mokutil.c:394: overrun-local: Overrunning array ""string"" of 300 bytes at byte offset 300 using index ""read_len"" (which evaluates to 300).
  #  392|       close (fd);
  #  393|
  #  394|->     if (string[read_len] != '\0') {
  #  395|           fprintf (stderr, ""corrupted string\n"");
  #  396|           return -1;

This is because the read loop limits itself to < BUF_SIZE, but the actual read() call uses BUF_SIZE - read_len as the size, and then updates read_len with the added number of characters:

  while (read_len < BUF_SIZE) {
    ssize_t rc = read (fd, string + read_len,
               BUF_SIZE - read_len - 1);
    ...
    read_len += rc;
  }

This means it's asking for BUF_SIZE characters, and as a result read_len can be adjusted to be BUF_SIZE, rather than BUF_SIZE - 1. The check for the string terminator (for "corruption"...) then overflows the buffer.

This patch changes the read() size parameter to always be BUF_SIZE - read_len - 1, so we'll never fill the last byte or adjust read_len to be BUF_SIZE. It further removes the "corrupted sting" test, as that cannot ever be the case.

Resolves: RHEL-27624

lcp commented 4 months ago

Thanks for patch!