lvmteam/lvm2

various string-handling issues in pvscan

Closed this issue · 2 comments

seebs commented

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

static void _online_pvid_file_remove_devno(int major, int minor)
{
	char path[PATH_MAX];
	char file_vgname[NAME_LEN];```

But wait, what about online_pvid_file_read, which is used to populate this?


/* vgname points to an offset in buf */
if ((name = _vgname_in_pvid_file_buf(buf)))
	strncpy(vgname, name, NAME_LEN);
else
	log_debug("No vgname in %s", path);

return 1;```

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.

teigland commented

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.