Fix exploitable HLE problems reported by hthh #4447

Merged
merged 5 commits into from Nov 24, 2016

Conversation

4 participants
@JosJuice
Contributor

JosJuice commented Nov 12, 2016

The problem that the third commit fixes wasn't actually reported by hthh. I saw it when working on the second commit and thought it looked suspicious. The other four problems were reported by hthh.


This change is Reviewable

JosJuice added some commits Nov 12, 2016

Correct bounds checking for /dev/sdio/slot0
The bounds checks in IOCtl were using 0x200 as the size of
m_Registers, which is more than the actual size, 0x200 / 4.

This commit turns m_Registers into an std::array to allow
for a correct and obvious way of getting its size.
HLE_OS: Implement %n in GetStringVA
%n writes to a pointer that's provided as a parameter.
We didn't have a custom implementation of this before,
meaning that %n would trigger a write to the host
memory instead of the emulated memory!
@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Nov 12, 2016

Member

Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/Common/FileUtil.cpp, line 11 at r1 (raw file):

#include <fcntl.h>
#include <limits.h>
#include <stdlib.h>

<cstdlib>


Source/Core/Common/FileUtil.cpp, line 724 at r1 (raw file):

  char absolute_path[MAX_PATH + 1];
  char* result = realpath(path.c_str(), absolute_path);
  return result ? std::string(result) : "";

std::string() doesn't need to be specified here.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 42 at r1 (raw file):

  if (absolute_root_path.empty() || absolute_full_path.empty())
  {
    PanicAlert("Couldn't get an absolute path. The root directory will be returned. "

Common/MsgHandler.h should be included, since this is where PanicAlert is defined.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 46 at r1 (raw file):

    return root_path;
  }
  else if (path_wii.empty() || path_wii[0] != '/' ||

else isn't necessary here. Same for the else below as well.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 49 at r1 (raw file):

           absolute_full_path.compare(0, absolute_root_path.size(), absolute_root_path) != 0)
  {
    WARN_LOG(WII_IPC_FILEIO,

Common/Logging/Log.h should be included.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_net.cpp, line 853 at r1 (raw file):

    if (BufferOutSize < 2 + sizeof(sa.sa_data))
      WARN_LOG(WII_IPC_NET, "IOCTL_SO_GETSOCKNAME output buffer is too small. Truncating");

Common/Logging/Log.h needs to be included.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_net.cpp, line 860 at r1 (raw file):

      Memory::Write_U8(sa.sa_family & 0xFF, BufferOut + 1);
    Memory::CopyToEmu(BufferOut + 2, &sa.sa_data,
                      std::min<size_t>(sizeof(sa.sa_data), BufferOutSize - 2));

<cstddef> needs to be included for size_t.


Comments from Reviewable

Member

lioncash commented Nov 12, 2016

Review status: 0 of 7 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/Common/FileUtil.cpp, line 11 at r1 (raw file):

#include <fcntl.h>
#include <limits.h>
#include <stdlib.h>

<cstdlib>


Source/Core/Common/FileUtil.cpp, line 724 at r1 (raw file):

  char absolute_path[MAX_PATH + 1];
  char* result = realpath(path.c_str(), absolute_path);
  return result ? std::string(result) : "";

std::string() doesn't need to be specified here.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 42 at r1 (raw file):

  if (absolute_root_path.empty() || absolute_full_path.empty())
  {
    PanicAlert("Couldn't get an absolute path. The root directory will be returned. "

Common/MsgHandler.h should be included, since this is where PanicAlert is defined.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 46 at r1 (raw file):

    return root_path;
  }
  else if (path_wii.empty() || path_wii[0] != '/' ||

else isn't necessary here. Same for the else below as well.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 49 at r1 (raw file):

           absolute_full_path.compare(0, absolute_root_path.size(), absolute_root_path) != 0)
  {
    WARN_LOG(WII_IPC_FILEIO,

Common/Logging/Log.h should be included.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_net.cpp, line 853 at r1 (raw file):

    if (BufferOutSize < 2 + sizeof(sa.sa_data))
      WARN_LOG(WII_IPC_NET, "IOCTL_SO_GETSOCKNAME output buffer is too small. Truncating");

Common/Logging/Log.h needs to be included.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_net.cpp, line 860 at r1 (raw file):

      Memory::Write_U8(sa.sa_family & 0xFF, BufferOut + 1);
    Memory::CopyToEmu(BufferOut + 2, &sa.sa_data,
                      std::min<size_t>(sizeof(sa.sa_data), BufferOutSize - 2));

<cstddef> needs to be included for size_t.


Comments from Reviewable

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Nov 12, 2016

Contributor

@lioncash All done.

Contributor

JosJuice commented Nov 12, 2016

@lioncash All done.

Avoid buffer over-reads in /dev/net/ip/top
Also fixes the less serious problem of buffer overflows
in emulated memory when BufferOutSize is less than 2.
@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Nov 13, 2016

Member

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 42 at r2 (raw file):

  if (absolute_root_path.empty() || absolute_full_path.empty())
  {
    PanicAlert("Couldn't get an absolute path. The root directory will be returned. "

A semicolon, rather than a period, would be slightly better here since the two independent clauses are closely related. e.g. 'Couldn't get an absolute path; the root directory will be returned.'


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 43 at r2 (raw file):

  {
    PanicAlert("Couldn't get an absolute path. The root directory will be returned. "
               "This will most likely will lead to failures.");

superfluous second 'will'


Comments from Reviewable

Member

lioncash commented Nov 13, 2016

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 42 at r2 (raw file):

  if (absolute_root_path.empty() || absolute_full_path.empty())
  {
    PanicAlert("Couldn't get an absolute path. The root directory will be returned. "

A semicolon, rather than a period, would be slightly better here since the two independent clauses are closely related. e.g. 'Couldn't get an absolute path; the root directory will be returned.'


Source/Core/Core/IPC_HLE/WII_IPC_HLE_Device_FileIO.cpp, line 43 at r2 (raw file):

  {
    PanicAlert("Couldn't get an absolute path. The root directory will be returned. "
               "This will most likely will lead to failures.");

superfluous second 'will'


Comments from Reviewable

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Nov 13, 2016

Contributor

@lioncash Done. I also added "IOS HLE: " to the beginning of the message, to make it easier to know where things went wrong.

Contributor

JosJuice commented Nov 13, 2016

@lioncash Done. I also added "IOS HLE: " to the beginning of the message, to make it easier to know where things went wrong.

@BhaaLseN

This comment has been minimized.

Show comment
Hide comment
@BhaaLseN

BhaaLseN Nov 19, 2016

Member

LGTM

Member

BhaaLseN commented Nov 19, 2016

LGTM

@sepalani

This comment has been minimized.

Show comment
Hide comment
@sepalani

sepalani Nov 21, 2016

Contributor

LBTM.

The way varargs are handled is way too messy/buggy.

The width and precision specifier aren't processed the right way. If someone use a "*" character to specify them as an argument, first, it will take the wrong one as a width/precision, then take on the host stack the one that should be processed. It might crash the user's computer if too many characters have to be written, even if the game/homebrew is legit.

If Dolphin is compiled with printf family functions supporting the POSIX's parameter field extension (which is the case on Linux with GCC, not Windows) an user can also use this notation to format the nth argument:

printf("21 equals %2$d\n", 42, 21);

Same issue as above, it isn't sanitized properly and will leak the host's stack.

Windows & Linux printf family functions are used to format the string but it also means that both implementations need to be supported to prevent that kind of platform-specific issue.

Contributor

sepalani commented Nov 21, 2016

LBTM.

The way varargs are handled is way too messy/buggy.

The width and precision specifier aren't processed the right way. If someone use a "*" character to specify them as an argument, first, it will take the wrong one as a width/precision, then take on the host stack the one that should be processed. It might crash the user's computer if too many characters have to be written, even if the game/homebrew is legit.

If Dolphin is compiled with printf family functions supporting the POSIX's parameter field extension (which is the case on Linux with GCC, not Windows) an user can also use this notation to format the nth argument:

printf("21 equals %2$d\n", 42, 21);

Same issue as above, it isn't sanitized properly and will leak the host's stack.

Windows & Linux printf family functions are used to format the string but it also means that both implementations need to be supported to prevent that kind of platform-specific issue.

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Nov 21, 2016

Contributor

I did notice that GetStringVA's approach is fragile, but I couldn't think of any cases where it would fail (other than the %n one I fixed), so thank you for writing about some problematic cases.

I don't think I will be able to make a proper fix for it any time soon. Would you prefer for this PR to be merged without the %n commit, or for this PR to be committed as-is despite the problems because the %n commit at least fixes the write to host memory? (Or are there problems with commits other than the %n one?)

Contributor

JosJuice commented Nov 21, 2016

I did notice that GetStringVA's approach is fragile, but I couldn't think of any cases where it would fail (other than the %n one I fixed), so thank you for writing about some problematic cases.

I don't think I will be able to make a proper fix for it any time soon. Would you prefer for this PR to be merged without the %n commit, or for this PR to be committed as-is despite the problems because the %n commit at least fixes the write to host memory? (Or are there problems with commits other than the %n one?)

@BhaaLseN

This comment has been minimized.

Show comment
Hide comment
@BhaaLseN

BhaaLseN Nov 21, 2016

Member

Not sure about that, I believe the %n fix is good enough as-is here, his notes are for something else (unless he specifically meant %n with "varargs").
As I understood it, he mostly meant modifiers like %03d (for at-least-3-digit zero-padded output) or %.5f (for floats with up to 5 fractional digits)...and wonky stuff like positional parameters printf("%2$d %1$d", 1, 2) or dynamic width parameters printf("%*s", 5, "test") vs. printf("%5s", "test")

While I'm not certain if positional parameters can even occur in DOLs (certainly not official game ones I suppose), he does have a point (since Homebrew might; depending on the compiler libogc etc. ship with and its capabilities). But I think that could be a follow-up PR just aswell, which may or may not include a rewrite of that logic to clean it up/make it less fragile (which is IMO a little out-of-scope for this current one).

Member

BhaaLseN commented Nov 21, 2016

Not sure about that, I believe the %n fix is good enough as-is here, his notes are for something else (unless he specifically meant %n with "varargs").
As I understood it, he mostly meant modifiers like %03d (for at-least-3-digit zero-padded output) or %.5f (for floats with up to 5 fractional digits)...and wonky stuff like positional parameters printf("%2$d %1$d", 1, 2) or dynamic width parameters printf("%*s", 5, "test") vs. printf("%5s", "test")

While I'm not certain if positional parameters can even occur in DOLs (certainly not official game ones I suppose), he does have a point (since Homebrew might; depending on the compiler libogc etc. ship with and its capabilities). But I think that could be a follow-up PR just aswell, which may or may not include a rewrite of that logic to clean it up/make it less fragile (which is IMO a little out-of-scope for this current one).

@sepalani

This comment has been minimized.

Show comment
Hide comment
@sepalani

sepalani Nov 21, 2016

Contributor

@JosJuice
As @BhaaLseN said, these issues aren't directly related to your PR so you might want to merge it as-is. Nonetheless, these particular cases are reading directly the host's stack and might be a concern at some point.

Contributor

sepalani commented Nov 21, 2016

@JosJuice
As @BhaaLseN said, these issues aren't directly related to your PR so you might want to merge it as-is. Nonetheless, these particular cases are reading directly the host's stack and might be a concern at some point.

@JosJuice JosJuice merged commit 9bfea4a into dolphin-emu:master Nov 24, 2016

10 of 11 checks passed

code-review/reviewable 7 files, 2 discussions left (JosJuice, lioncash)
Details
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd Build succeeded on builder pr-freebsd
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@JosJuice JosJuice deleted the JosJuice:hthh-hle-issues branch Nov 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment