Proxy for GetRawInputDeviceInfoW() causes a buffer overflow

[marti4d]

Describe what you noticed and did

See Firefox Bug 1921198 - Crash in [@ memcpy | sbiedll.dll | GetRawInputDeviceInfo] caused by sandboxie

How often did you encounter it so far?

It would likely happen to any user that is using a gamepad on Firefox in Sandboxie.

Expected behavior

Sandboxie's implementation needs to be changed so that it only considers the pcbSize argument to be wide-chars when uiCommand == RIDI_DEVICENAME.

Affected program

Firefox

Download link

https://mozilla.org

Where is the program located?

Not relevant to my request.

Did the program or any related process close unexpectedly?

Yes, it did, but no .dmp file has been created in the system.

Crash dump

No response

What version of Sandboxie are you running now?

I'm not the one experiencing the crash, but it should happen on the lastest release.

Is it a new installation of Sandboxie?

I recently did a new clean installation.

Is it a regression from previous versions?

¯\_(ツ)_/¯

In which sandbox type you have this problem?

Not relevant to my request.

Can you reproduce this problem on a new empty sandbox?

I can confirm it also on a new empty sandbox.

What is your Windows edition and version?

All versions of Windows since Windows XP

In which Windows account you have this problem?

Not relevant to my request.

Please mention any installed security software

N/A

Did you previously enable some security policy settings outside Sandboxie?

No response

Trace log

No response

Sandboxie.ini configuration

No response

[bastik-1001]

Thank you for the pull request.

For my curiosity, is there another reason to use a gamepad with a browser, other than playing games with it?

[gcp]

I'm not sure why you're asking us, we're just observing that Sandboxie causes applications that it wraps to crash :-)

In theory it can be used as a generic wireless controller, if the webpage support it, or with an extension, but I suspect this is very rare.

Firefox won't let the GamePad be detected unless the user is using it while the page is active, so at least it is useless for fingerprinting (provided you use Firefox...) That makes it more likely that the users we see crashing were at least trying to do something useful with it.

[DavidXanatos]

how to test this i have an xbox controller any pages with free gamies wher i could test it?

[offhub]

https://hardwaretester.com/gamepad

[DavidXanatos]

great seams to work :D

[gcp]

@DavidXanatos something went wrong when merging this, you made a commit immediately afterwards which reverted the security fixes: 3878f30

Notice the comments explaining why *pcbSize can't be accessed due to UB were deleted and the code was reverted back to the insecure version. Leaving the UB in seems very dangerous as the compiler is now free to delete all the null pointer checks!

[DavidXanatos]

memcpy(&req->cbSize, pcbSize, sizeof(*pcbSize));
and
req->cbSize = *pcbSize;

booth take 4 bytes of memory at the location pointed to by pcbSize and write them to the location pointed to by req->cbSize
booth variables are of the same type hence no conversion takes place,

its just something like

move rax, [pcbSize]
move [&req->cbSize], rax

I don't see how memcpy would here be any safer.
in any case the pcbSize must be a valid pointer pointing to a valid memory location.
And whats located there is irrelevant as its being copied without thinking to the new location.

[marti4d]

@DavidXanatos Please read my comment about why this is "Undefined Behavior". Even if it works today, it may not work tomorrow, and future compilers with more aggressive optimizers are allowed to assume that you never do what you're doing and may produce invalid code. The two pieces of code you just posted may look identical, but they are not. One of them says "read the int value at pcbSize and write it to req->cbSize" and the other says "blindly copy the bytes at pcbSize to the memory at &req->cbSize without interpreting those bytes as having any meaning". That last part is very important.

[gcp]

It's not anything with what the code seems to do.

The problem is that the C/C++ standards specifically says you are not allowed to do this (undefined behavior). It doesn't matter the types are the same, the spec only allows accessing uninitialized memory for "narrow character types".

The compiler is allowed to assume you never do anything that is not allowed. So it's legal for it to delete all code paths leading up to this. The "illegal" operation only happens if cbSize is not null, so the compiler should now assume cbSize IS null, else the impossible happens. And every branch if (cbSize) {} should be deleted, as we know cbSize must be 0, else something impossible could happen.

https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP53-CPP.+Do+not+read+uninitialized+memory

Here's an example: https://godbolt.org/z/dbY6rE8WE
If you look at the code Clang generates, it never actually checks what's in the memory, it just forces the program to crash 🤣

The canonical example here is de-referencing a pointer and then doing a null check afterwards. The compiler is allowed to remove the null checks, so you can crash with a null pointer in a piece of code that has a null pointer check immediately above it. That won't make sense, unless you know about the above behavior. This case with the uninitialized memory is a bit less well known, but as the godbolt link illustrates, the C++ compiler is definitely aware that it can do whatever it wants after the read from uninitialized memory.

[DavidXanatos]

the sample is not correctly representing the situation in question

#include <cstdint>
#include <iostream>

int main()
{
    char buff[1024];
    int i = 5;

    std::cout << i << std::endl;

    if (buff[5]) {
        i = 6;
    }

    std::cout << i << std::endl;

    return 0;
}

if (buff[5]) is always true for a local array

the correct scenario would be

#include <cstdint>
#include <iostream>

void test(char* buff, int& i)
{
    if(buff){
        if (buff[5]) {
            i = 6;
        }
    }
}

int main()
{
    char buff[1024];
    int i = 5;

    std::cout << i << std::endl;

    test(buff, i);

    std::cout << i << std::endl;

    return 0;
}

this gives us

test(char*, int&):
        test    rdi, rdi
        je      .LBB0_3
        cmp     byte ptr [rdi + 5], 0
        je      .LBB0_3
        mov     dword ptr [rsi], 6
.LBB0_3:
        ret

main:
        ...

and here we see that the compile keeps the if(buff){ even with optimization

there are two statements to be made about a pointer argument like cbSize,
first is if the pointer is valid or not
second is if its valid if the value it points to is initialized or not
and the first does not depand on the second.

No meter what we do with the de-referenced pointer later the compiler can not assume from it any statement about whether the pointer itself is valid, especially when there is an explicit check in the code.

[gcp]

if (buff[5]) is always true for a local array
...
No meter what we do with the de-referenced pointer later the compiler can not assume from it any statement about whether the pointer itself is valid
...
especially when there is an explicit check in the code.

I'm going to disengage from this conversation, because I don't think we're getting anywhere 😞

[DavidXanatos]

if (buff[5]) is always true for a local array
...
No meter what we do with the de-referenced pointer later the compiler can not assume from it any statement about whether the pointer itself is valid
...
especially when there is an explicit check in the code.

I'm going to disengage from this conversation, because I don't think we're getting anywhere 😞

I'm sorry you are dissapointed but this coding style is all over sandboxie, if one would start fixing formal issues which work just fine that would take for ever and ever.
Sandboxie is also not intended to work on any other platform that what windows works on, and whats good enough for windows is also fine for us, see my comment 3878f30#r147389239 about how the ASCII wrapper GetRawInputDeviceInfoA is implemented, they don't even check if the pointer is not NULL.

[isaak654]

So if there was a pull request that would solve these formal problems in one fell swoop, would you be willing to consider it? Could we leave a little window open on this (in a separate issue)?

[DavidXanatos]

Well such a pull request would replace many quick mov mov instruction pairs with a full blown function call to memcpy which would execute at least a couple dozens instructions each time so we would add a non insignificant additional CPU load only to adhere to the official standard, it would not improve anything practically.
Best case the compiler obtimizes memcpy away and puts a mov or two in its place.
Leaving us with the same final machine code we already head to begin with.

[bastik-1001]

Maybe working code is working code, but if a patch is fixing an issue and formally correct, why mess with it again to get the same result, just to have it share how other parts are written?

Due to my lack of understanding if something is safe, even if done informal or if it needs to be avoided at all cost, I can't make statements about this. In general, however, it has to be implemented with security by design, (privacy by design), code quality over it being functional, working with defense in depth, instead of assuming it can't be triggered anyway.

[gabrielesvelto]

FYI use of undefined behavior in code has long been known to be a source of instability and security issues even though it may appear to temporarily work. See this old paper for several practical examples including what's been discussed here.

[CaliSunUSA]

Is this one related to the issue I have? I am assuming that in this issue gamepad is connected thorough USB.
In my issue, my WIFI is also connected through USB and my entire WIFI drops permanently after using Firefox inside Sandboxie.
#4278