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

block-crypto: Fix off-by-one in keypath #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crogers1
Copy link

@crogers1 crogers1 commented Feb 12, 2024

Commit 6ffa1d8 replaced the use
of strncpy with safe_strncpy. When we calculate the length here,
we calculate it up to the separator, but don't include the sep.
When the string is passed to safe_strncpy, that function subtracts an
extra 1 byte to make room for the null character, which ends up
cutting off the last character in the path since the length was
exact, and relied on the 0-initialized, statically allocated buffer
to null terminate the string by default.

This commit increases the length value by one before calling
safe_strncpy to avoid losing the last byte of data. This essentially
copies the path, including the separator which was omitted before,
and then replaces the separator with a null character. It also
adds MIN() to make sure we don't write outside keydir.

@crogers1
Copy link
Author

Fixed the author/signoff mismatch.

@@ -143,7 +143,7 @@ find_keyfile(char **keyfile, const char *dirs,
safe_strncpy(keydir, dirs, sizeof(keydir));
dirs = NULL;
} else {
size_t len = sep - dirs;
size_t len = (sep - dirs) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may fix the newly introduced bug but the code is still buggy because nothing guarantees that len < sizeof(keydir). I think something like this should work:

size_t len = sep - dirs;
strncpy(keydir, dirs, MIN(len, sizeof(keydir) - 1));
...

This should result in no more than 255 characters copied and the 256th char is guaranteed to be NUL since keydir is zero-initialized.

Copy link
Author

@crogers1 crogers1 Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change it to add that guarantee. Also just to clarify, I assume you want to keep the call to safe_strncpy instead of going back to the old strncpy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crogers1 tbh given that this code is exclusively within an #ifdef OPEN_XT we really couldn't care less about whether you use strncpy or safe_strncpy as we don't even compile this code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'd still like to keep it consistent with the rest of blktap so I'll leave the safe_strncpy in. Appreciate y'all giving this a review.

  Commit 6ffa1d8 replaced the use
  of strncpy with safe_strncpy.  When we calculate the length here,
  we calculate it up to the separator, but don't include the sep.
  When the string is passed to safe_strncpy, that function subtracts an
  extra 1 byte to make room for the null character, which ends up
  cutting off the last character in the path since the length was
  exact, and relied on the 0-initialized, statically allocated buffer
  to null terminate the string by default.

  This commit increases the length value by one before calling
  safe_strncpy to avoid losing the last byte of data. This essentially
  copies the path, including the separator which was omitted before,
  and then replaces the separator with a null character. It also
  adds MIN() to make sure we don't write outside keydir.

Signed-off-by: Chris Rogers <[email protected]>
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.

3 participants