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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions drivers/block-crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ find_keyfile(char **keyfile, const char *dirs,
safe_strncpy(keydir, dirs, sizeof(keydir));
dirs = NULL;
} else {
size_t len = sep - dirs;
safe_strncpy(keydir, dirs, len);
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 •
edited
Loading

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.

safe_strncpy(keydir, dirs, MIN(len, sizeof(keydir)));
dirs = sep+1;
}

Expand Down