Skip to content

Commit

Permalink
Fix integer overflow in cheatsImportGSACodeFile length check.
Browse files Browse the repository at this point in the history
Although a length check is being performed on the imported GSA Codes file, `len` is both a signed int and attacker controlled.

With a specially crafted GSA Codes file, an attacker could specify a value for `len` that overflows the `int` type, rolling over into a negative number. By doing so, the attacker can bypass the conditional mentioned above.

The `fseek` length parameter is of type `size_t` which is an unsigned int, this will result in `len` being interpreted as a large unsigned int, allowing for a stack based buffed overflow in the desc char array.

By making `len` an unsigned integer, it will prevent the overflow. It ensures that the bounds check works as intended.
  • Loading branch information
fresh-eggs authored and denisfa committed Jan 17, 2020
1 parent 77c299c commit 6a8a9e6
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/gba/Cheats.cpp
Expand Up @@ -2047,7 +2047,7 @@ bool cheatsImportGSACodeFile(const char* name, int game, bool v3)
return false;
}

int len = 0;
uint32_t len = 0;
bool found = false;
int g = 0;
while (games > 0) {
Expand Down

1 comment on commit 6a8a9e6

@fresh-eggs
Copy link
Contributor Author

@fresh-eggs fresh-eggs commented on 6a8a9e6 Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a clarification, this isn't actually related to an overflow of the integer.

It is simply due to the attackers ability to abuse the type interpretations of len in both the signed conditional and unsigned count parameter in fread.

I had overflows on the brain and clearly it leaked out 😄. The PR has a better description:
#593

Please sign in to comment.