Skip to content

Commit

Permalink
1.14.10
Browse files Browse the repository at this point in the history
  • Loading branch information
@DavidXanatos
DavidXanatos committed Sep 29, 2024
1 parent fbafb1f commit 3878f30
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).

### Fixed
- fixed issue with sandbox path entry combo boxes
- fixed Proxy for GetRawInputDeviceInfoW() causes a buffer overflow [#4267](https://github.com/sandboxie-plus/Sandboxie/issues/4267) (thanks marti4d)



Expand Down
18 changes: 6 additions & 12 deletions Sandboxie/core/dll/guimisc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1350,16 +1350,11 @@ _FX LONG Gui_GetRawInputDeviceInfo_impl(
req->hasData = !!pData;
req->hasSize = !!pcbSize;

if (pcbSize) {
// *pcbSize is allowed to be uninitialized if pData == nullptr.
// It would be UB to access it as a UINT, so it's important that
// we simply copy the bytes without interpretation.
memcpy(&req->cbSize, pcbSize, sizeof(*pcbSize));
}

if (lenData) {
if (lenData)
memcpy(reqData, pData, lenData);
}

if (pcbSize)
req->cbSize = *pcbSize;

This comment has been minimized.

Sign in to view

Copy link
@marti4d

marti4d Sep 30, 2024

Contributor

@DavidXanatos -- Umm... Did you miss the long rant/comment I had here about how *pcbSize is allowed to be uninitialized according to MSDN, and therefore reading it is potentially Undefined Behavior in C? Why did you re-add incorrect code to your program after I went through the trouble to correct it? ¯\_(ツ)_/¯ 🤦‍♂️

This comment has been minimized.

Sign in to view

Copy link
@DavidXanatos

DavidXanatos Sep 30, 2024

Author Member

reading it with memcpy(&rpl->cbSize, pcbSize, sizeof(*pcbSize));
is equivalent to req->cbSize = *pcbSize;
in booth cases 4 bytes / sizeof(ULONG) of memory pointed to by pcbSize is read and write into rpl->cbSize

This comment has been minimized.

Sign in to view

Copy link
@marti4d

marti4d Sep 30, 2024

Contributor

No, these two things are not the same in C. In C, it is Undefined Behavior to read an uninitialized int value even if all you're doing is copying it to another uninitialized int value -- Simply the act of having an expression that evaluates the value of the variable is UB.

The exception to that (according to the C Standard) is when you access the value as a bunch of unsigned char. If you do that, you are simply accessing the bytes of the underlying memory without interpretation, and therefore it is allowed by the standard.

Since that is what memcpy() does, it is actually fine to memcpy() a piece of uninitialized memory from one location to another.

This comment has been minimized.

Sign in to view

Copy link
@marti4d

marti4d Sep 30, 2024

Contributor

In other words, if a piece of memory may be uninitialized, you may only ever read it through an unsigned char* or it's UB.

This comment has been minimized.

Sign in to view

Copy link
@gcp

gcp Sep 30, 2024 •
edited
Loading

To understand why this is important, the compilers' optimizer may legally assume that you will never do UB, so it will consider it safe to optimize away all branches of this code that execute that UB snippet.

In practice, that means that it could decide to remove all the branches where pcbSize is not 0, breaking this entirely.

And maybe it's not doing it now, but any random code change in the neighborhood risks doing that.

This comment has been minimized.

Sign in to view

Copy link
@DavidXanatos

DavidXanatos Sep 30, 2024

Author Member

Why would it assume that if I do if (pcbSize) req->cbSize = *pcbSize;
then I'm telling the compiler that pcbSize may be NULL or a valid value.
So the compiler should not optimize the null check away, the code clearly states that the value may be NULL.
And assigning a UINT to an other UINT does not require any interpretation its just a bulk copy so de don't have to care which value the source UINT has, as any value it may have is a valid UINT value.
Would we use a double or something ok there are special values with special meanings. But any value an UINT can have is valid so it may be from our view point uninitialized but any state it may be in will be valid, even if its a random one.

This comment has been minimized.

Sign in to view

Copy link
@marti4d

marti4d Sep 30, 2024

Contributor

assigning a UINT to an other UINT does not require any interpretation its just a bulk copy

No, that's what I've been trying to say to you -- This statement is untrue. Any time you read from any variable there is interpretation. That is simply how the language is specified. This statement is not a bulk copy -- It says "first interpret the bytes at this location as a valid int and read it" and then "write the thing you just read as a valid int at this other location".

This comment has been minimized.

Sign in to view

Copy link
@marti4d

marti4d Sep 30, 2024

Contributor

any value an UINT can have is valid

First of all, this is not true on every platform. Yes -- It is always true on every platform that Windows runs on, but regardless it doesn't matter -- The C Language says that a program that does this is not a valid program.

It's no different that you de-referencing a NULL pointer -- The C Language says that you are simply not allowed to do that.

This comment has been minimized.

Sign in to view

Copy link
@DavidXanatos

DavidXanatos Sep 30, 2024

Author Member

Any time you read from any variable there is interpretation. That is simply how the language is specified.

that may be written so but its not what is being done by any compiler I know, just assigning just copies it and doesn't care what its copying.

It is always true on every platform that Windows runs on

and that's all that we want it to work on

The C Language says that a program that does this is not a valid program.

I'm quite sure if you would go with the rule book through the source code you would find a few thousand rule violations.

see user32.dll

18008b760  int64_t GetRawInputDeviceInfoA(int64_t hDevice, int32_t uiCommand, int64_t pData, int32_t* pcbSize)
18008b760  {
18008b76b      pData_1 = pData;
18008b780      int64_t rax;
18008b780      if (uiCommand != RIDI_DEVICENAME)
18008b77a      {
18008b87c          rax = NtUserGetRawInputDeviceInfo(hDevice, uiCommand, pData, pcbSize);
18008b87c      }
18008b786      else
18008b786      {
18008b786          arg_10 = 0;
18008b78d          if (pData != 0)
18008b78a          {
18008b7b4              int32_t rax_1 = *(uint32_t*)pcbSize;
18008b7b7              arg_10 = rax_1;
18008b7c0              int64_t rax_2;
18008b7c0              uint64_t rcx_2;
18008b7c0              if (rax_1 < 0x7fffffff)
18008b7bb              {
18008b7eb                  rax_2 = RtlAllocateHeap(data_1800b3020, 8, ((uint64_t)(rax_1 * 2)));
18008b7f7                  char* rdi_1 = rax_2;
18008b7fd                  if (rax_2 == 0)
18008b7fa                  {
18008b7ff                      rcx_2 = ((uint64_t)((int32_t)(rax_2 + 0xe)));
18008b7ff                  }
18008b814                  else
18008b814                  {
18008b814                      int32_t rax_3 = NtUserGetRawInputDeviceInfo(hDevice, 0x20000007, rdi_1, &arg_10);
18008b820                      int32_t rsi_1 = rax_3;
18008b825                      if (rax_3 != 0xffffffff)
18008b822                      {
18008b85e                          rsi_1 = WCSToMBEx(0, rdi_1, rsi_1, &pData_1, *(uint32_t*)pcbSize, 0);
18008b84e                      }
18008b836                      else if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
18008b833                      {
18008b83e                          *(uint32_t*)pcbSize = (arg_10 * 2);
18008b83c                      }
18008b86c                      RtlFreeHeap(data_1800b3020, 0, rdi_1);
18008b878                      rax = ((uint64_t)rsi_1);
18008b878                  }
18008b878              }
18008b7c2              else
18008b7c2              {
18008b7c2                  rcx_2 = 0x57;
18008b7c2              }
18008b7fd              if ((rax_1 >= 0x7fffffff || (rax_1 < 0x7fffffff && rax_2 == 0)))
18008b7fa              {
18008b7c7                  RtlSetLastWin32Error(rcx_2);
18008b7d3                  rax = 0xffffffff;
18008b7d3              }
18008b7bb          }
18008b793          else
18008b793          {
18008b793              rax = NtUserGetRawInputDeviceInfo();
18008b7a1              if (rax == 0)
18008b79f              {
18008b7ad                  *(uint32_t*)pcbSize = (arg_10 * 2);
18008b7ab              }
18008b79f          }
18008b79f      }
18008b897      return rax;
18008b897  }

they also do ´´´int32_t rax_1 = (uint32_t)pcbSize;´´´ without even checking if pcbSize is not null

if its good enough for windows its surely good enough for sandboxie as well

rpl = Gui_CallProxy(req, reqSize, sizeof(*rpl));

Expand All @@ -1371,9 +1366,8 @@ _FX LONG Gui_GetRawInputDeviceInfo_impl(
ULONG error = rpl->error;
ULONG retval = rpl->retval;

if (pcbSize) {
memcpy(pcbSize, &rpl->cbSize, sizeof(rpl->cbSize));
}
if (pcbSize)
*pcbSize = rpl->cbSize;

if (lenData) {
LPVOID rplData = (BYTE*)rpl + sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_RPL);
Expand Down
13 changes: 7 additions & 6 deletions Sandboxie/core/svc/GuiServer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3545,21 +3545,22 @@ ULONG GuiServer::GetRawInputDeviceInfoSlave(SlaveArgs *args)
SetLastError(ERROR_SUCCESS);
if (req->unicode) {
rpl->retval = GetRawInputDeviceInfoW((HANDLE)req->hDevice, req->uiCommand, reqData, pcbSize);
} else {
}
else {
rpl->retval = GetRawInputDeviceInfoA((HANDLE)req->hDevice, req->uiCommand, reqData, pcbSize);
}
rpl->error = GetLastError();

if (pcbSize) {
// It's possible that (*pcbSize) could still be uninitialized.
// It would be UB to access it as a UINT.
memcpy(&rpl->cbSize, pcbSize, sizeof(*pcbSize));
}
if (pcbSize)
rpl->cbSize = *pcbSize;

if (lenData) {
rpl->hasData = TRUE;

This comment has been minimized.

Sign in to view

Copy link
@marti4d

marti4d Sep 30, 2024

Contributor

And why did you re-add this member? AFAICT it doesn't do anything and it's unused...

This comment has been minimized.

Sign in to view

Copy link
@DavidXanatos

DavidXanatos Sep 30, 2024

Author Member

its not used but may be used in future, it does not ham us to keep it
LPVOID rplData = (BYTE*)rpl + sizeof(GUI_GET_RAW_INPUT_DEVICE_INFO_RPL);
memcpy(rplData, reqData, lenData);
}
else
rpl->hasData = FALSE;

args->rpl_len = args->req_len;

Expand Down
2 changes: 2 additions & 0 deletions Sandboxie/core/svc/GuiWire.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -703,8 +703,10 @@ struct tagGUI_GET_RAW_INPUT_DEVICE_INFO_REQ

struct tagGUI_GET_RAW_INPUT_DEVICE_INFO_RPL
{
ULONG status;
ULONG error;
ULONG retval;
BOOLEAN hasData;
UINT cbSize;
};

Expand Down

0 comments on commit 3878f30

Please sign in to comment.