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

OSInfo: fix new Windows 10 OSInfo to not include NUL values in the displayable version string. #2470

Merged
merged 1 commit into from Aug 6, 2016

Conversation

@mkrautz
Copy link
Member

commented Jul 26, 2016

These strings from the registry are NUL terminated, but in practice, this
isn't guaranteed.

This means that the displayable version string for Windows 10 currently
contains NUL values.

The Mumble client doesn't care, but it isn't very nice. It breaks things
like CVP providers that don't sanitize Murmur's output.

To fix the problem, this change introduces a regString function that
converts a wchar_t string to QString. Any NUL value in the input
string will terminate the string.

Fixes #2469

@mkrautz mkrautz force-pushed the mkrautz:osinfo-win10-fix-zero-terminal branch from c523c61 to f22e80f Jul 26, 2016

@mkrautz mkrautz added mumble and removed murmur labels Jul 31, 2016

break;
}
}
return QString::fromWCharArray(string, size);

This comment has been minimized.

Copy link
@hacst

hacst Jul 31, 2016

Member

There are already functions for what the loop does in the *nlen family:

static QString regString(wchar_t *string, int size) {
    return QString::fromWCharArray(string, wcsnlen(string, size));
}

Ought to work the same I think (not tested, might need casts).

@hacst

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

To bad we can't just switch to RegGetValue which guarantess zero termination. Then we could just use fromWcharArray without passing a size and be good. I guess we could achieve the same with making buf one larger and ensuring it stays a \0 or doing the usual "set last element to \0" dance...not that fond of that though as it is quite error prone. I guess given those constraints having a helper for it makes sense.

The regString name is fine. Maybe qStringFromWCharNString would be a closer fit to the C string handling function nomenclature but that's neither here nor there (and might be confused for a real qt function).

TL;DR: LGTM besides the simplification for the function I mentioned ;)

Did you see any other locations where we might have assumed zero termination where it isn't guaranteed?

@hacst

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

W.r.t to #2469 specifically: Does this PR actually "fix" it? I mean the clients can still send whatever they want in those fields so if we want to protect to trusty third party tooling we might want to add sanitization to murmur where we reasonably can (can't really do so for PA identity as there we actively use 0 in strings but besides that I don't think we really need non-zero-terminated strings). Also it might make sense to emphasize that the strings aren't necessarily zero-terminated and hence might contain 0 where that is the case.

@mkrautz mkrautz force-pushed the mkrautz:osinfo-win10-fix-zero-terminal branch from f22e80f to fb46d5a Aug 6, 2016

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2016

@hacst PTAL.

// QString. If the string contains a NUL value, that
// NUL value will terminate the string.
static QString regString(wchar_t *string, int size) {
if (size <= 0) {

This comment has been minimized.

Copy link
@hacst

hacst Aug 6, 2016

Member

Is that just being cautious w.r.t to negative values and ==0 is thrown in there because we can or do wcsnlen or fromWCharArray have issues with size == 0 I missed?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 6, 2016

Author Member

wcsnlen doesn't document what happens when numberOfElements is < 0, or == 0. That alone makes me want to have this check. :-)

QString::fromWCharArray only mentions that -1 is the default value that causes it to look for NUL terminators.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 6, 2016

Author Member

And yes, I thought that == 0 might as well be there. By checking for <= 0, we're sure that we only pass postive integers to the functions, which is what we're sure they're coded to handle.

This comment has been minimized.

Copy link
@hacst

hacst Aug 6, 2016

Member

If they don't handle == 0 then I would consider it a doc bug. Just reading those they ought to handle it just fine. <0 is another thing altogether as wcsnlen takes a size_t so no meaningful thing would happen with that ;) But checking both is 100% fine, I just wanted to make sure I didn't misread the docs so I don't produce potential bugs by misusing them under wrong assumptions going forwards.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 6, 2016

Author Member

I agree. == 0 should be handled by the other two functions.

But their behavior for <0 isn't documented.

This comment has been minimized.

Copy link
@hacst

hacst Aug 6, 2016

Member

Well. -1 is documented for the qt function but the rest of the negative range isn't. I don't expect it to do anything good ;) But that gets it input from wcsnlen which doesn't take a signed element count so no behavior to document there in the first place ;)

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 6, 2016

Author Member

Oh yeah, I added the if <= 0 because of the static_cast<size_t>.
Should I drop the == 0 and make it just if (len < 0) ?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Aug 6, 2016

Author Member

I think I prefer the way it is currently. An early return for a case we can handle w/o calling external functions.

This comment has been minimized.

Copy link
@hacst

hacst Aug 6, 2016

Member

As I said. It's 100% fine as is. I just wanted to make sure I hadn't taken away something wrong from the documentation of the functions.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2016

Oh, and also: I agree with the thought that we should probably sanitize some of our strings in Murmur.
However, this one is caused by ourselves, and is obviously a bug on its own.

Sanitizing Murmur's strings in general is tough. Ideally, we'd do it for all strings, because protobuf allows sending NUL bytes in ALL strings. But that breaks down for things like plugin context.

// If string contains a NUL, adjust the size such
// that the NUL is not included in the returned
// string.
size_t adjustedSize = wcsnlen(string, static_cast<size_t>(size));

This comment has been minimized.

Copy link
@hacst

hacst Aug 6, 2016

Member

could be const ;)

@hacst

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

LGTM

@hacst

This comment has been minimized.

Copy link
Member

commented Aug 6, 2016

W.r.t to sanitizing stuff in murmur: I think our use of NUL in strings is extremely localized so mabe it's not that painful to do.

OSInfo: fix new Windows 10 OSInfo to not include NUL values in the di…
…splayable version string.

These strings from the registry are NUL terminated, but in practice, this
isn't guaranteed.

This means that the displayable version string for Windows 10 currently
contains NUL values.

The Mumble client doesn't care, but it isn't very nice. It breaks things
like CVP providers that don't sanitize Murmur's output.

To fix the problem, this change introduces a regString function that
converts a wchar_t string to QString. Any NUL value in the input
string will terminate the string.

Fixes #2469

@mkrautz mkrautz force-pushed the mkrautz:osinfo-win10-fix-zero-terminal branch from fb46d5a to 2df5f2f Aug 6, 2016

@mkrautz mkrautz merged commit 0a9c69d into mumble-voip:master Aug 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.