From 68f3399cfd784f95927da7e90c72be952fde284a Mon Sep 17 00:00:00 2001 From: Dylan T Date: Tue, 4 Jan 2022 20:39:02 +0000 Subject: [PATCH 1/5] Merge pull request from GHSA-p62j-hrxm-xcxf This checks the following things: - Validity of UTF-8 encoding of title, author, and page content - Maximum soft and hard lengths of title, author, and page content (soft limits may be bypassed by uncancelling PlayerEditBookEvent; hard limits may not be bypassed) - Maximum number of pages. Books with more than 50 pages may still be edited, but may not have new pages added. --- src/pocketmine/Player.php | 51 +++++++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/src/pocketmine/Player.php b/src/pocketmine/Player.php index 7721b4013..c4bcd651d 100644 --- a/src/pocketmine/Player.php +++ b/src/pocketmine/Player.php @@ -210,6 +210,7 @@ use function json_encode; use function json_last_error_msg; use function lcg_value; use function max; +use function mb_strlen; use function microtime; use function min; use function preg_match; @@ -217,6 +218,7 @@ use function round; use function spl_object_hash; use function sprintf; use function sqrt; +use function str_repeat; use function strlen; use function strpos; use function strtolower; @@ -3265,6 +3267,24 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ return true; } + /** + * @throws \UnexpectedValueException + */ + private function checkBookText(string $string, string $fieldName, int $softLimit, int $hardLimit, bool &$cancel) : string{ + if(strlen($string) > $hardLimit){ + throw new \UnexpectedValueException(sprintf("Book %s must be at most %d bytes, but have %d bytes", $fieldName, $hardLimit, strlen($string))); + } + + $result = TextFormat::clean($string, false); + //strlen() is O(1), mb_strlen() is O(n) + if(strlen($result) > $softLimit * 4 || mb_strlen($result, 'UTF-8') > $softLimit){ + $cancel = true; + $this->server->getLogger()->debug(sprintf("Cancelled book edit by %s due to %s exceeded soft limit of %d chars", $this->getName(), $fieldName, $softLimit)); + } + + return $result; + } + public function handleBookEdit(BookEditPacket $packet) : bool{ /** @var WritableBook $oldBook */ $oldBook = $this->inventory->getItem($packet->inventorySlot); @@ -3274,10 +3294,11 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $newBook = clone $oldBook; $modifiedPages = []; - + $cancel = false; switch($packet->type){ case BookEditPacket::TYPE_REPLACE_PAGE: - $newBook->setPageText($packet->pageNumber, $packet->text); + $text = self::checkBookText($packet->text, "page text", 256, 0x7fff, $cancel); + $newBook->setPageText($packet->pageNumber, $text); $modifiedPages[] = $packet->pageNumber; break; case BookEditPacket::TYPE_ADD_PAGE: @@ -3286,7 +3307,8 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ //TODO: the client can send insert-before actions on trailing client-side pages which cause odd behaviour on the server return false; } - $newBook->insertPage($packet->pageNumber, $packet->text); + $text = self::checkBookText($packet->text, "page text", 256, 0x7fff, $cancel); + $newBook->insertPage($packet->pageNumber, $text); $modifiedPages[] = $packet->pageNumber; break; case BookEditPacket::TYPE_DELETE_PAGE: @@ -3305,17 +3327,36 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $modifiedPages = [$packet->pageNumber, $packet->secondaryPageNumber]; break; case BookEditPacket::TYPE_SIGN_BOOK: + $title = self::checkBookText($packet->title, "title", 16, 0x7fff, $cancel); + //this one doesn't have a limit in vanilla, so we have to improvise + $author = self::checkBookText($packet->author, "author", 256, 0x7fff, $cancel); + /** @var WrittenBook $newBook */ $newBook = Item::get(Item::WRITTEN_BOOK, 0, 1, $newBook->getNamedTag()); - $newBook->setAuthor($packet->author); - $newBook->setTitle($packet->title); + $newBook->setAuthor($author); + $newBook->setTitle($title); $newBook->setGeneration(WrittenBook::GENERATION_ORIGINAL); break; default: return false; } + /* + * Plugins may have created books with more than 50 pages; we allow plugins to do this, but not players. + * Don't allow the page count to grow past 50, but allow deleting, swapping or altering text of existing pages. + */ + $oldPageCount = count($oldBook->getPages()); + $newPageCount = count($newBook->getPages()); + if(($newPageCount > $oldPageCount && $newPageCount > 50)){ + $this->server->getLogger()->debug("Cancelled book edit by " . $this->getName() . " due to adding too many pages (new page count would be $newPageCount)"); + $cancel = true; + } + $event = new PlayerEditBookEvent($this, $oldBook, $newBook, $packet->type, $modifiedPages); + if($cancel){ + $event->setCancelled(); + } + $event->call(); if($event->isCancelled()){ return true; From 958a9dbf0fe3131ab60319c5a939f5dfbfe5dfbb Mon Sep 17 00:00:00 2001 From: Dylan T Date: Tue, 4 Jan 2022 20:40:55 +0000 Subject: [PATCH 2/5] Merge pull request from GHSA-c6fg-99pr-25m9 * Skin: impose length limits on skinID, geometryName and geometryData fields * Skin: remove extra newline --- src/pocketmine/entity/Skin.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/pocketmine/entity/Skin.php b/src/pocketmine/entity/Skin.php index 98d1fdc4e..4effa390b 100644 --- a/src/pocketmine/entity/Skin.php +++ b/src/pocketmine/entity/Skin.php @@ -28,6 +28,7 @@ use function implode; use function in_array; use function json_encode; use function strlen; +use const INT32_MAX; class Skin{ public const ACCEPTED_SKIN_SIZES = [ @@ -67,10 +68,20 @@ class Skin{ } } + private static function checkLength(string $string, string $name, int $maxLength) : void{ + if(strlen($string) > $maxLength){ + throw new InvalidSkinException("$name must be at most $maxLength bytes, but have " . strlen($string) . " bytes"); + } + } + /** * @throws InvalidSkinException */ public function validate() : void{ + self::checkLength($this->skinId, "Skin ID", 32767); + self::checkLength($this->geometryName, "Geometry name", 32767); + self::checkLength($this->geometryData, "Geometry data", INT32_MAX); + if($this->skinId === ""){ throw new InvalidSkinException("Skin ID must not be empty"); } From 8c4b8a90422566c262bb96c545fca9365f3a4226 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 4 Jan 2022 20:44:10 +0000 Subject: [PATCH 3/5] CS --- src/pocketmine/Player.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pocketmine/Player.php b/src/pocketmine/Player.php index c4bcd651d..1bf11b1d6 100644 --- a/src/pocketmine/Player.php +++ b/src/pocketmine/Player.php @@ -218,7 +218,6 @@ use function round; use function spl_object_hash; use function sprintf; use function sqrt; -use function str_repeat; use function strlen; use function strpos; use function strtolower; From a4af1609eaf42eff5effe04b08f2c1402d408f3d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 4 Jan 2022 20:47:31 +0000 Subject: [PATCH 4/5] Release 3.26.5 --- changelogs/3.26.md | 4 ++++ src/pocketmine/VersionInfo.php | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/changelogs/3.26.md b/changelogs/3.26.md index 35b840a98..102e68aad 100644 --- a/changelogs/3.26.md +++ b/changelogs/3.26.md @@ -26,3 +26,7 @@ Plugin developers should **only** update their required API to this version if y - Fixed skins appearing black when using RTX resource packs. - Fixed chunks containing furnaces in old worlds (pre-2017) being discarded as corrupted. - This was caused by a strict corruption check detecting bad data created by a bug in PocketMine-MP that was fixed in 2017. + +# 3.26.5 +- Fixed several denial-of-service attack vectors related to writable book text length and encoding. +- Fixed several denial-of-service attack vectors related to skin data field lengths. diff --git a/src/pocketmine/VersionInfo.php b/src/pocketmine/VersionInfo.php index ae075617b..8effa492f 100644 --- a/src/pocketmine/VersionInfo.php +++ b/src/pocketmine/VersionInfo.php @@ -34,5 +34,5 @@ const _VERSION_INFO_INCLUDED = true; const NAME = "PocketMine-MP"; const BASE_VERSION = "3.26.5"; -const IS_DEVELOPMENT_BUILD = true; +const IS_DEVELOPMENT_BUILD = false; const BUILD_CHANNEL = "pm3"; From e8893dd91f340b48e2072ebbba8d16a4cbd1ea82 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 4 Jan 2022 20:47:31 +0000 Subject: [PATCH 5/5] 3.26.6 is next --- src/pocketmine/VersionInfo.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pocketmine/VersionInfo.php b/src/pocketmine/VersionInfo.php index 8effa492f..d2c0036be 100644 --- a/src/pocketmine/VersionInfo.php +++ b/src/pocketmine/VersionInfo.php @@ -33,6 +33,6 @@ if(defined('pocketmine\_VERSION_INFO_INCLUDED')){ const _VERSION_INFO_INCLUDED = true; const NAME = "PocketMine-MP"; -const BASE_VERSION = "3.26.5"; -const IS_DEVELOPMENT_BUILD = false; +const BASE_VERSION = "3.26.6"; +const IS_DEVELOPMENT_BUILD = true; const BUILD_CHANNEL = "pm3";