Scan uploaded levels for ACE #1002
Conversation
|
Using PHP's SPL |
There was a problem hiding this comment.
I believe this line is unnecessary: if ($obj[1] == 1817) {
Regardless of the object, the itemID property will still work with the array.
Moreover, this fix doesn't account for the level re-upload tool. As this fix is right now, it will only be "safe" assuming other servers don't have an ACE level
There was a problem hiding this comment.
tried to test this with a proof of concept level, but the code wasn't functional as-is. i did have to make some changes to make the base code work and found a few bugs with the implementation.
unfortunately, those other bugs meant the poc level passed through the check just fine...
in general, error handling needs to be significantly improved. i'm not sure how you tested the code but the current implementation definitely needs more work before it could be called secure.
| <?php | ||
|
|
||
| class LevelParser { | ||
| public $data = []; |
There was a problem hiding this comment.
tbh i don't see the point of making this a single member class. this data member is basically useless anyways (unless you were planning on extending the functionality of this class?). just make validate static and pass the level string as its param
There was a problem hiding this comment.
Yes in the future functionality could be expanded on validating other parts of the level request. However for now I have implemented your suggestions
incl/lib/LevelParser.php
Outdated
| $obj = self::map($objs[$i], ','); | ||
| // Check for pickup trigger exploit | ||
| if ($obj[1] == 1817) { | ||
| $id = intval($obj[80]); |
There was a problem hiding this comment.
i would recommend checking the color id property of color triggers as well, but it's probably not necessary. mostly just crashes
incl/lib/LevelParser.php
Outdated
| */ | ||
| public function validate() { | ||
| // Ocular Miracle ain't happening | ||
| $max_level_size = 50 * 1024 * 1024; // 50 MB Max uncompressed size |
There was a problem hiding this comment.
keep in mind that any level over this size could bypass the check entirely. it wouldn't be impossible to add padding in ways that inflate the size of the level while keeping it working on desktop platforms...
|
Fixed various things. I think this also necessitates a server scan utility for existing levels. |
|
does this support levelstrings compressed with both gzdeflate (H4sIAAA header in b64) and gzcompress (eJ header in b64), as well as uncompressed levelstrings? |
Yes, zlib_decode() works on both deflate and gzip data. When the level string is uncompressed, the b64 and decompression don't run. |
Correction, it works as described but I haven't added the magic for deflate so it doesn't run. I'll fix this when I get home. |
| if (preg_match('/[^\x20-\x7e]/', $data)) return false; | ||
| $objs = explode(';', $data); | ||
| // Skip level header | ||
| for ($i = 1; $i < count($objs); $i++) { | ||
| $obj = self::map($objs[$i], ','); | ||
| // Clamp groups based on version | ||
| if (array_key_exists(80, $obj)) { | ||
| $id = intval($obj[80]); | ||
| if ($id > ($version > 21 ? self::MAX_GROUPS_22 : self::MAX_GROUPS_21) || $id < 0) return false; |
There was a problem hiding this comment.
allowing all ascii characters with preg_match('/[^\x20-\x7e]/', $data)) is a bad idea imo. the reason is the $id = intval($obj[80]);
In the client, this data is parsed using atoi which has different behaviour to intval
atoi does not support scientific notation whereas intval does
C
PHP
Would be a simple fix if it wasn't for the fact that a larger exponent results in the method erroring to 0
This problem allows you to use values in the client while also tricking this code into believing its safe
There was a problem hiding this comment.
Yes that's right.
It's also possible to insert other invalid values while conforming to object format. This PR is primarily concerned with ACE protection not validating all level data (though this is worth looking into more).
The check for non-ASCII characters is simply a heuristic for detecting malformed level data (such as attempting to upload arbitrary binary files or catching possible failure of the data decompression) and is not intended as a catch-all solution for level validation.
|
Hello! I'm creator of this core's fork, could i have your permission to add this PR to my repo? https://github.com/MegaSa1nt/GMDprivateServer |
Added a LevelParser class which scans uploads for invalid things. I'd like someone with access to a PoC level to test on a local installation. And also sanity check my late-night code.