1229804 - Incorrect string length specified in Windows sandbox logging.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

It seems that the Length in UNICODE_STRING that I use is in bytes, even though the buffer pointer in the same structure is effectively a wchar_t* (which is always 2 bytes on Windows).

Comment 1

[Bob Owen (:bobowen)]

Attachment: Use the correct string length in Windows sandbox logging

Comment 2

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb231036fe2d

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8694797 [details] [diff] [review]
Use the correct string length in Windows sandbox logging

Review of attachment 8694797 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/chromium-shim/sandbox/win/sandboxLogging.cpp
@@ +65,5 @@
>  {
>    if (sLogFunction) {
>      // Skip an extra frame to allow for this function.
>      LogBlocked(aFunctionName,
> +               base::WideToUTF8(std::wstring(aContext, aLengthInBytes / sizeof(wchar_t))).c_str(),

I think we could take advantage of the `std::wstring(const_pointer _First, const_pointer _Last)` constructor [1] to rewrite this instead as `base::WideToUTF8(std::wstring(aContext, aContext + aLengthInBytes)).c_str(),`

[1] https://msdn.microsoft.com/en-us/library/y22b7w38.aspx

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)
> Comment on attachment 8694797 [details] [diff] [review]
> Use the correct string length in Windows sandbox logging
> 
> Review of attachment 8694797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/sandbox/chromium-shim/sandbox/win/sandboxLogging.cpp
> @@ +65,5 @@
> >  {
> >    if (sLogFunction) {
> >      // Skip an extra frame to allow for this function.
> >      LogBlocked(aFunctionName,
> > +               base::WideToUTF8(std::wstring(aContext, aLengthInBytes / sizeof(wchar_t))).c_str(),
> 
> I think we could take advantage of the `std::wstring(const_pointer _First,
> const_pointer _Last)` constructor [1] to rewrite this instead as
> `base::WideToUTF8(std::wstring(aContext, aContext +
> aLengthInBytes)).c_str(),`
> 
> [1] https://msdn.microsoft.com/en-us/library/y22b7w38.aspx

There might be an off-by-one error in this suggestion. It might need to be `aContext + aLengthInBytes - 1`. I'm not sure from the documentation that I've read

Comment 5

[Bob Owen (:bobowen)]

Thanks Tim.

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)

> > +               base::WideToUTF8(std::wstring(aContext, aLengthInBytes / sizeof(wchar_t))).c_str(),
> 
> I think we could take advantage of the `std::wstring(const_pointer _First,
> const_pointer _Last)` constructor [1] to rewrite this instead as
> `base::WideToUTF8(std::wstring(aContext, aContext +
> aLengthInBytes)).c_str(),`

But aContext is a wchar_t* (wchar_t is 2 bytes on Windows), so with pointer arithmetic, as the length is in bytes _Last would be at a point double the length of the string.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29408f18b82e

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Bob Owen (:bobowen) from comment #5)
> Thanks Tim.
> 
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)
> 
> > > +               base::WideToUTF8(std::wstring(aContext, aLengthInBytes / sizeof(wchar_t))).c_str(),
> > 
> > I think we could take advantage of the `std::wstring(const_pointer _First,
> > const_pointer _Last)` constructor [1] to rewrite this instead as
> > `base::WideToUTF8(std::wstring(aContext, aContext +
> > aLengthInBytes)).c_str(),`
> 
> But aContext is a wchar_t* (wchar_t is 2 bytes on Windows), so with pointer
> arithmetic, as the length is in bytes _Last would be at a point double the
> length of the string.

Ah, good point. We'd have to do some casting or do the exact same math for that approach to work.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/29408f18b82e