fixing incompatible protocol handling, do not explode immediately on bad clientdata, login encode/decode is now symmetrical

src/network/mcpe/handler/LoginPacketHandler.php

@@ -28,12 +28,17 @@ use pocketmine\event\player\PlayerPreLoginEvent;
 use pocketmine\network\BadPacketException;
 use pocketmine\network\mcpe\auth\ProcessLoginTask;
 use pocketmine\network\mcpe\convert\SkinAdapterSingleton;
+use pocketmine\network\mcpe\JwtException;
+use pocketmine\network\mcpe\JwtUtils;
 use pocketmine\network\mcpe\NetworkSession;
 use pocketmine\network\mcpe\protocol\LoginPacket;
 use pocketmine\network\mcpe\protocol\PlayStatusPacket;
 use pocketmine\network\mcpe\protocol\ProtocolInfo;
+use pocketmine\network\mcpe\protocol\types\login\AuthenticationData;
+use pocketmine\network\mcpe\protocol\types\login\ClientData;
 use pocketmine\network\mcpe\protocol\types\login\ClientDataPersonaPieceTintColor;
 use pocketmine\network\mcpe\protocol\types\login\ClientDataPersonaSkinPiece;
+use pocketmine\network\mcpe\protocol\types\login\JwtChain;
 use pocketmine\network\mcpe\protocol\types\PersonaPieceTintColor;
 use pocketmine\network\mcpe\protocol\types\PersonaSkinPiece;
 use pocketmine\network\mcpe\protocol\types\SkinAnimation;
@@ -45,6 +50,7 @@ use pocketmine\Server;
 use pocketmine\uuid\UUID;
 use function array_map;
 use function base64_decode;
+use function is_array;
 
 /**
  * Handles the initial login phase of the session. This handler is used as the initial state.
@@ -94,12 +100,15 @@ class LoginPacketHandler extends PacketHandler{
 			return true;
 		}
 
-		if(!Player::isValidUserName($packet->extraData->displayName)){
+		$extraData = $this->fetchAuthData($packet->chainDataJwt);
+
+		if(!Player::isValidUserName($extraData->displayName)){
 			$this->session->disconnect("disconnectionScreen.invalidName");
 
 			return true;
 		}
 
+		$clientData = $this->parseClientData($packet->clientDataJwt);
 		$safeB64Decode = static function(string $base64, string $context) : string{
 			$result = base64_decode($base64, true);
 			if($result === false){
@@ -108,7 +117,6 @@ class LoginPacketHandler extends PacketHandler{
 			return $result;
 		};
 		try{
-			$clientData = $packet->clientData; //this serves no purpose except readability
 			/** @var SkinAnimation[] $animations */
 			$animations = [];
 			foreach($clientData->AnimatedImageData as $k => $animation){
@@ -154,17 +162,17 @@ class LoginPacketHandler extends PacketHandler{
 		}
 
 		try{
-			$uuid = UUID::fromString($packet->extraData->identity);
+			$uuid = UUID::fromString($extraData->identity);
 		}catch(\InvalidArgumentException $e){
 			throw BadPacketException::wrap($e, "Failed to parse login UUID");
 		}
 		($this->playerInfoConsumer)(new PlayerInfo(
-			$packet->extraData->displayName,
+			$extraData->displayName,
 			$uuid,
 			$skin,
-			$packet->clientData->LanguageCode,
+			$clientData->LanguageCode,
-			$packet->extraData->XUID,
+			$extraData->XUID,
-			(array) $packet->clientData
+			(array) $clientData
 		));
 
 		$ev = new PlayerPreLoginEvent(
@@ -194,6 +202,67 @@ class LoginPacketHandler extends PacketHandler{
 		return true;
 	}
 
+	/**
+	 * @throws BadPacketException
+	 */
+	protected function fetchAuthData(JwtChain $chain) : AuthenticationData{
+		/** @var AuthenticationData|null $extraData */
+		$extraData = null;
+		foreach($chain->chain as $k => $chain){
+			//validate every chain element
+			try{
+				[, $claims, ] = JwtUtils::parse($chain);
+			}catch(JwtException $e){
+				throw BadPacketException::wrap($e);
+			}
+			if(isset($claims["extraData"])){
+				if($extraData !== null){
+					throw new BadPacketException("Found 'extraData' more than once in chainData");
+				}
+
+				if(!is_array($claims["extraData"])){
+					throw new BadPacketException("'extraData' key should be an array");
+				}
+				$mapper = new \JsonMapper;
+				$mapper->bEnforceMapType = false; //TODO: we don't really need this as an array, but right now we don't have enough models
+				$mapper->bExceptionOnMissingData = true;
+				$mapper->bExceptionOnUndefinedProperty = true;
+				try{
+					/** @var AuthenticationData $extraData */
+					$extraData = $mapper->map($claims['extraData'], new AuthenticationData);
+				}catch(\JsonMapper_Exception $e){
+					throw BadPacketException::wrap($e);
+				}
+			}
+		}
+		if($extraData === null){
+			throw new BadPacketException("'extraData' not found in chain data");
+		}
+		return $extraData;
+	}
+
+	/**
+	 * @throws BadPacketException
+	 */
+	protected function parseClientData(string $clientDataJwt) : ClientData{
+		try{
+			[, $clientDataClaims, ] = JwtUtils::parse($clientDataJwt);
+		}catch(JwtException $e){
+			throw BadPacketException::wrap($e);
+		}
+
+		$mapper = new \JsonMapper;
+		$mapper->bEnforceMapType = false; //TODO: we don't really need this as an array, but right now we don't have enough models
+		$mapper->bExceptionOnMissingData = true;
+		$mapper->bExceptionOnUndefinedProperty = true;
+		try{
+			$clientData = $mapper->map($clientDataClaims, new ClientData);
+		}catch(\JsonMapper_Exception $e){
+			throw BadPacketException::wrap($e);
+		}
+		return $clientData;
+	}
+
 	/**
 	 * TODO: This is separated for the purposes of allowing plugins (like Specter) to hack it and bypass authentication.
 	 * In the future this won't be necessary.

src/network/mcpe/protocol/LoginPacket.php

@@ -25,18 +25,14 @@ namespace pocketmine\network\mcpe\protocol;
 
 #include <rules/DataPacket.h>
 
-use pocketmine\network\mcpe\JwtException;
-use pocketmine\network\mcpe\JwtUtils;
 use pocketmine\network\mcpe\protocol\serializer\NetworkBinaryStream;
-use pocketmine\network\mcpe\protocol\types\login\AuthenticationData;
-use pocketmine\network\mcpe\protocol\types\login\ClientData;
 use pocketmine\network\mcpe\protocol\types\login\JwtChain;
-use pocketmine\utils\BinaryDataException;
 use pocketmine\utils\BinaryStream;
-use function is_array;
 use function is_object;
 use function json_decode;
+use function json_encode;
 use function json_last_error_msg;
+use function strlen;
 
 class LoginPacket extends DataPacket implements ServerboundPacket{
 	public const NETWORK_ID = ProtocolInfo::LOGIN_PACKET;
@@ -48,12 +44,8 @@ class LoginPacket extends DataPacket implements ServerboundPacket{
 
 	/** @var JwtChain */
 	public $chainDataJwt;
-	/** @var AuthenticationData|null extraData index of whichever JWT has it */
-	public $extraData = null;
 	/** @var string */
 	public $clientDataJwt;
-	/** @var ClientData decoded payload of the clientData JWT */
-	public $clientData;
 
 	public function canBeSentBeforeLogin() : bool{
 		return true;
@@ -61,17 +53,13 @@ class LoginPacket extends DataPacket implements ServerboundPacket{
 
 	protected function decodePayload(NetworkBinaryStream $in) : void{
 		$this->protocol = $in->getInt();
-		$this->decodeConnectionRequest($in);
+		$this->decodeConnectionRequest($in->getString());
 	}
 
-	/**
+	protected function decodeConnectionRequest(string $binary) : void{
-	 * @throws PacketDecodeException
+		$connRequestReader = new BinaryStream($binary);
-	 * @throws BinaryDataException
-	 */
-	protected function decodeConnectionRequest(NetworkBinaryStream $in) : void{
-		$buffer = new BinaryStream($in->getString());
 
-		$chainDataJson = json_decode($buffer->get($buffer->getLInt()));
+		$chainDataJson = json_decode($connRequestReader->get($connRequestReader->getLInt()));
 		if(!is_object($chainDataJson)){
 			throw new PacketDecodeException("Failed decoding chain data JSON: " . json_last_error_msg());
 		}
@@ -85,57 +73,28 @@ class LoginPacket extends DataPacket implements ServerboundPacket{
 		}
 
 		$this->chainDataJwt = $chainData;
-
+		$this->clientDataJwt = $connRequestReader->get($connRequestReader->getLInt());
-		foreach($this->chainDataJwt->chain as $k => $chain){
-			//validate every chain element
-			try{
-				[, $claims, ] = JwtUtils::parse($chain);
-			}catch(JwtException $e){
-				throw new PacketDecodeException($e->getMessage(), 0, $e);
-			}
-			if(isset($claims["extraData"])){
-				if(!is_array($claims["extraData"])){
-					throw new PacketDecodeException("'extraData' key should be an array");
-				}
-				if($this->extraData !== null){
-					throw new PacketDecodeException("Found 'extraData' more than once in chainData");
-				}
-
-				$mapper = new \JsonMapper;
-				$mapper->bEnforceMapType = false; //TODO: we don't really need this as an array, but right now we don't have enough models
-				$mapper->bExceptionOnMissingData = true;
-				$mapper->bExceptionOnUndefinedProperty = true;
-				try{
-					$this->extraData = $mapper->map($claims['extraData'], new AuthenticationData);
-				}catch(\JsonMapper_Exception $e){
-					throw PacketDecodeException::wrap($e);
-				}
-			}
-		}
-		if($this->extraData === null){
-			throw new PacketDecodeException("'extraData' not found in chain data");
-		}
-
-		$this->clientDataJwt = $buffer->get($buffer->getLInt());
-		try{
-			[, $clientData, ] = JwtUtils::parse($this->clientDataJwt);
-		}catch(JwtException $e){
-			throw new PacketDecodeException($e->getMessage(), 0, $e);
-		}
-
-		$mapper = new \JsonMapper;
-		$mapper->bEnforceMapType = false; //TODO: we don't really need this as an array, but right now we don't have enough models
-		$mapper->bExceptionOnMissingData = true;
-		$mapper->bExceptionOnUndefinedProperty = true;
-		try{
-			$this->clientData = $mapper->map($clientData, new ClientData);
-		}catch(\JsonMapper_Exception $e){
-			throw PacketDecodeException::wrap($e);
-		}
 	}
 
 	protected function encodePayload(NetworkBinaryStream $out) : void{
-		//TODO
+		$out->putInt($this->protocol);
+		$out->putString($this->encodeConnectionRequest());
+	}
+
+	protected function encodeConnectionRequest() : string{
+		$connRequestWriter = new BinaryStream();
+
+		$chainDataJson = json_encode($this->chainDataJwt);
+		if($chainDataJson === false){
+			throw new \InvalidStateException("Failed to encode chain data JSON: " . json_last_error_msg());
+		}
+		$connRequestWriter->putLInt(strlen($chainDataJson));
+		$connRequestWriter->put($chainDataJson);
+
+		$connRequestWriter->putLInt(strlen($this->clientDataJwt));
+		$connRequestWriter->put($this->clientDataJwt);
+
+		return $connRequestWriter->getBuffer();
 	}
 
 	public function handle(PacketHandlerInterface $handler) : bool{