From 3ed57ce49a6b5ea49ced6d9622af04cf35120d79 Mon Sep 17 00:00:00 2001 From: Dylan T Date: Tue, 4 Jan 2022 20:39:02 +0000 Subject: [PATCH] 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/item/WritableBookPage.php | 17 +++++- src/item/WrittenBook.php | 9 +++ .../mcpe/handler/InGamePacketHandler.php | 56 +++++++++++++++++-- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/src/item/WritableBookPage.php b/src/item/WritableBookPage.php index 62ebbcf50..95d5a6258 100644 --- a/src/item/WritableBookPage.php +++ b/src/item/WritableBookPage.php @@ -23,17 +23,32 @@ declare(strict_types=1); namespace pocketmine\item; +use pocketmine\utils\Limits; use pocketmine\utils\Utils; +use function sprintf; +use function strlen; class WritableBookPage{ + public const PAGE_LENGTH_HARD_LIMIT_BYTES = Limits::INT16_MAX; + public const PHOTO_NAME_LENGTH_HARD_LIMIT_BYTES = Limits::INT16_MAX; /** @var string */ private $text; /** @var string */ private $photoName; + /** + * @throws \InvalidArgumentException + */ + private static function checkLength(string $string, string $name, int $maxLength) : void{ + if(strlen($string) > $maxLength){ + throw new \InvalidArgumentException(sprintf("$name must be at most %d bytes, but have %d bytes", $maxLength, strlen($string))); + } + } + public function __construct(string $text, string $photoName = ""){ - //TODO: data validation + self::checkLength($text, "Text", self::PAGE_LENGTH_HARD_LIMIT_BYTES); + self::checkLength($photoName, "Photo name", self::PHOTO_NAME_LENGTH_HARD_LIMIT_BYTES); Utils::checkUTF8($text); $this->text = $text; $this->photoName = $photoName; diff --git a/src/item/WrittenBook.php b/src/item/WrittenBook.php index f356b1cd6..88352cdd1 100644 --- a/src/item/WrittenBook.php +++ b/src/item/WrittenBook.php @@ -24,7 +24,10 @@ declare(strict_types=1); namespace pocketmine\item; use pocketmine\nbt\tag\CompoundTag; +use pocketmine\utils\Limits; use pocketmine\utils\Utils; +use function sprintf; +use function strlen; class WrittenBook extends WritableBookBase{ @@ -85,6 +88,9 @@ class WrittenBook extends WritableBookBase{ * @return $this */ public function setAuthor(string $authorName) : self{ + if(strlen($authorName) > Limits::INT16_MAX){ + throw new \InvalidArgumentException(sprintf("Author must be at most %d bytes, but have %d bytes", Limits::INT16_MAX, strlen($authorName))); + } Utils::checkUTF8($authorName); $this->author = $authorName; return $this; @@ -103,6 +109,9 @@ class WrittenBook extends WritableBookBase{ * @return $this */ public function setTitle(string $title) : self{ + if(strlen($title) > Limits::INT16_MAX){ + throw new \InvalidArgumentException(sprintf("Title must be at most %d bytes, but have %d bytes", Limits::INT16_MAX, strlen($title))); + } Utils::checkUTF8($title); $this->title = $title; return $this; diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 04e1c847b..cfa80738c 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -37,6 +37,7 @@ use pocketmine\inventory\transaction\TransactionException; use pocketmine\inventory\transaction\TransactionValidationException; use pocketmine\item\VanillaItems; use pocketmine\item\WritableBook; +use pocketmine\item\WritableBookPage; use pocketmine\item\WrittenBook; use pocketmine\math\Vector3; use pocketmine\nbt\tag\CompoundTag; @@ -96,6 +97,8 @@ use pocketmine\network\mcpe\protocol\types\PlayerAction; use pocketmine\network\PacketHandlingException; use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; +use pocketmine\utils\Limits; +use pocketmine\utils\TextFormat; use function array_push; use function base64_encode; use function count; @@ -107,8 +110,10 @@ use function json_decode; use function json_encode; use function json_last_error_msg; use function max; +use function mb_strlen; use function microtime; use function preg_match; +use function sprintf; use function strlen; use function strpos; use function substr; @@ -711,6 +716,24 @@ class InGamePacketHandler extends PacketHandler{ return false; //TODO } + /** + * @throws PacketHandlingException + */ + private function checkBookText(string $string, string $fieldName, int $softLimit, int $hardLimit, bool &$cancel) : string{ + if(strlen($string) > $hardLimit){ + throw new PacketHandlingException(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->session->getLogger()->debug("Cancelled book edit due to $fieldName exceeded soft limit of $softLimit chars"); + } + + return $result; + } + public function handleBookEdit(BookEditPacket $packet) : bool{ //TODO: break this up into book API things $oldBook = $this->player->getInventory()->getItem($packet->inventorySlot); @@ -720,10 +743,11 @@ class InGamePacketHandler extends PacketHandler{ $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, WritableBookPage::PAGE_LENGTH_HARD_LIMIT_BYTES, $cancel); + $newBook->setPageText($packet->pageNumber, $text); $modifiedPages[] = $packet->pageNumber; break; case BookEditPacket::TYPE_ADD_PAGE: @@ -732,7 +756,8 @@ class InGamePacketHandler extends PacketHandler{ //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, WritableBookPage::PAGE_LENGTH_HARD_LIMIT_BYTES, $cancel); + $newBook->insertPage($packet->pageNumber, $text); $modifiedPages[] = $packet->pageNumber; break; case BookEditPacket::TYPE_DELETE_PAGE: @@ -751,11 +776,14 @@ class InGamePacketHandler extends PacketHandler{ $modifiedPages = [$packet->pageNumber, $packet->secondaryPageNumber]; break; case BookEditPacket::TYPE_SIGN_BOOK: - /** @var WrittenBook $newBook */ + $title = self::checkBookText($packet->title, "title", 16, Limits::INT16_MAX, $cancel); + //this one doesn't have a limit in vanilla, so we have to improvise + $author = self::checkBookText($packet->author, "author", 256, Limits::INT16_MAX, $cancel); + $newBook = VanillaItems::WRITTEN_BOOK() ->setPages($oldBook->getPages()) - ->setAuthor($packet->author) - ->setTitle($packet->title) + ->setAuthor($author) + ->setTitle($title) ->setGeneration(WrittenBook::GENERATION_ORIGINAL); break; default: @@ -771,7 +799,23 @@ class InGamePacketHandler extends PacketHandler{ BookEditPacket::TYPE_SIGN_BOOK => PlayerEditBookEvent::ACTION_SIGN_BOOK, default => throw new AssumptionFailedError("We already filtered unknown types in the switch above") }; + + /* + * 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->session->getLogger()->debug("Cancelled book edit due to adding too many pages (new page count would be $newPageCount)"); + $cancel = true; + } + $event = new PlayerEditBookEvent($this->player, $oldBook, $newBook, $action, $modifiedPages); + if($cancel){ + $event->cancel(); + } + $event->call(); if($event->isCancelled()){ return true;