Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

vathpela
Copy link
Contributor

@vathpela vathpela commented Mar 1, 2024

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

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

Signed-off-by: Peter Jones <pjones@redhat.com>
@lcp lcp merged commit c587eb9 into lcp:master Mar 4, 2024
@lcp
Copy link
Owner

lcp commented Mar 4, 2024

Thanks for patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants