-
Notifications
You must be signed in to change notification settings - Fork 72
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
various string-handling issues in pvscan #51
Comments
Thanks for the analysis, the limits and/or errors need some cleanup. I've run a couple of quick tests. vgcreate allows you to create a VG with a name up to 127 characters long. But you can't create an LV in that VG given the combined name lengths. If you want to create an LV (with a one character name) in a VG, then the VG can have a max name length of 123 characters. If you want longer LV names, then the VG name needs to be shorter. It seems that 124 is the max combined length of VG name plus LV name. |
Probably not hugely important except the more I look at this the more afraid of it I am. Casual inspection makes it look like it's hard to trigger anything obviously bad, because there's usually other guards around things, but the whole thing feels like it's halfway through a refactor.
#define NAME_LEN 128
If there's no vgname in the path, we return 1, indicating success, after having not touched vgname here. It appears that nearby callers are passing in zeroed buffers, but that may not always be true. On the other hand, we're still returning 1 rather than 0 in what appears to be an error case, so in that case we'd be handing back an empty vgname buffer even though the function returned 1; maybe this is why everything ignores the return code and instead checks vgname[0].
More concerningly, though: strncpy only nul-terminates strings if there's enough space. If the maximum allowed name length is 128 characters, and you use all 128, you don't get a nul. Furthermore, you could use more than 128; it won't run PAST the buffer, but it'll fill the buffer and not terminate it.
I have no idea whether this is exploitable or anything, I just noticed it because someone mentioned an unrelated question about the code and I happened to see a strncpy and got nervous.
I'm also definitely nervous about dm_snprintf, which is restoring the semantics snprintf had in glibc prior to 1999, which seems like long enough ago that I'm a little distrustful of it.
The text was updated successfully, but these errors were encountered: