From fdeccc1674e01a4241a8ada93cfe56bb4f00b5e9 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 9 Feb 2022 11:36:19 +0100 Subject: [PATCH] Fix transmission handling of maps with LUA scripts The message handler wrongly only counted the map size as received not also the (just received) lua size and hence did never finish. Also add buffer overflow protection to the map-receive code and test both issues. Fixes #1494 --- libs/s25main/network/GameClient.cpp | 14 ++-- tests/s25Main/network/testGameClient.cpp | 87 +++++++++++++++++++++--- 2 files changed, 86 insertions(+), 15 deletions(-) diff --git a/libs/s25main/network/GameClient.cpp b/libs/s25main/network/GameClient.cpp index 7c0f3fd54..84e351c08 100644 --- a/libs/s25main/network/GameClient.cpp +++ b/libs/s25main/network/GameClient.cpp @@ -933,18 +933,22 @@ bool GameClient::OnGameMessage(const GameMessage_Map_Data& msg) return true; LOG.writeToFile("<<< NMS_MAP_DATA(%u)\n") % msg.data.size(); - if(msg.isMapData) - std::copy(msg.data.begin(), msg.data.end(), mapinfo.mapData.data.begin() + msg.offset); - else - std::copy(msg.data.begin(), msg.data.end(), mapinfo.luaData.data.begin() + msg.offset); + std::vector& targetData = (msg.isMapData) ? mapinfo.mapData.data : mapinfo.luaData.data; + if(msg.data.size() > targetData.size() || msg.offset > targetData.size() - msg.data.size()) + { + OnError(ClientError::MapTransmission); + return true; + } + std::copy(msg.data.begin(), msg.data.end(), targetData.begin() + msg.offset); uint32_t totalSize = mapinfo.mapData.data.size(); uint32_t receivedSize = msg.offset + msg.data.size(); if(!mapinfo.luaFilepath.empty()) { totalSize += mapinfo.luaData.data.size(); + // Assumes lua data comes after the map data if(!msg.isMapData) - receivedSize = mapinfo.mapData.data.size(); + receivedSize += mapinfo.mapData.data.size(); } if(ci) ci->CI_MapPartReceived(receivedSize, totalSize); diff --git a/tests/s25Main/network/testGameClient.cpp b/tests/s25Main/network/testGameClient.cpp index 738ad3438..78d314fbe 100644 --- a/tests/s25Main/network/testGameClient.cpp +++ b/tests/s25Main/network/testGameClient.cpp @@ -12,6 +12,7 @@ #include "network/GameMessages.h" #include "gameTypes/GameTypesOutput.h" #include "test/testConfig.h" +#include "rttr/test/LogAccessor.hpp" #include "rttr/test/random.hpp" #include "s25util/boostTestHelpers.h" #include "s25util/tmpFile.h" @@ -19,6 +20,7 @@ #include #include #include +#include #include namespace bfs = boost::filesystem; @@ -48,7 +50,8 @@ MOCK_BASE_CLASS(MockClientInterface, ClientInterface) } // namespace -BOOST_AUTO_TEST_CASE(ClientFollowsConnectProtocol) +static constexpr std::array usesLuaScriptValues{false, true}; +BOOST_DATA_TEST_CASE(ClientFollowsConnectProtocol, usesLuaScriptValues, usesLuaScript) { GameClient client; GameMessageInterface& clientMsgInterface = client; @@ -87,35 +90,60 @@ BOOST_AUTO_TEST_CASE(ClientFollowsConnectProtocol) } const boost::filesystem::path testMapPath = rttr::test::rttrBaseDir / "tests" / "testData" / "maps" / "LuaFunctions.SWD"; + const boost::filesystem::path testLuaPath = boost::filesystem::path(testMapPath).replace_extension("lua"); MapInfo mapInfo; mapInfo.mapData.CompressFromFile(testMapPath, &mapInfo.mapChecksum); + if(usesLuaScript) + mapInfo.luaData.CompressFromFile(testLuaPath, &mapInfo.luaChecksum); TmpFolder tmpUserdata(rttr::test::rttrTestDataDirOut); RTTRCONFIG.overridePathMapping("USERDATA", tmpUserdata); bfs::create_directories(RTTRCONFIG.ExpandPath(s25::folders::mapsPlayed)); + const auto mapDataSize = mapInfo.mapData.data.size(); + const auto luaDataSize = (usesLuaScript) ? mapInfo.luaData.data.size() : 0u; { MOCK_EXPECT(callbacks.CI_NextConnectState).with(ConnectState::ReceiveMap).once(); clientMsgInterface.OnGameMessage(GameMessage_Map_Info(testMapPath.filename().string(), MapType::OldMap, - mapInfo.mapData.uncompressedLength, - mapInfo.mapData.data.size(), 0, 0)); + mapInfo.mapData.uncompressedLength, mapDataSize, + mapInfo.luaData.uncompressedLength, luaDataSize)); const auto msg = boost::dynamic_pointer_cast(client.GetMainPlayer().sendQueue.pop()); BOOST_TEST_REQUIRE(msg); BOOST_TEST(!msg->requestInfo); } { - BOOST_TEST_REQUIRE(mapInfo.mapData.data.size() > 10u); // Should be or we can't chunk the map - MOCK_EXPECT(callbacks.CI_MapPartReceived).with(10u, mapInfo.mapData.data.size()).once(); + const auto totalSize = mapDataSize + luaDataSize; + BOOST_TEST_REQUIRE(mapDataSize > 10u); // Should be or we can't chunk the map + if(usesLuaScript) + BOOST_TEST_REQUIRE(luaDataSize > 5u); // Should be or we can't chunk the lua data + // First part of map + MOCK_EXPECT(callbacks.CI_MapPartReceived).with(10u, totalSize).once(); clientMsgInterface.OnGameMessage(GameMessage_Map_Data(true, 0, mapInfo.mapData.data.data(), 10)); BOOST_TEST(client.GetMainPlayer().sendQueue.empty()); - - MOCK_EXPECT(callbacks.CI_MapPartReceived).with(mapInfo.mapData.data.size(), mapInfo.mapData.data.size()).once(); - MOCK_EXPECT(callbacks.CI_NextConnectState).with(ConnectState::VerifyMap).once(); + // Remaining part of map + MOCK_EXPECT(callbacks.CI_MapPartReceived).with(mapDataSize, totalSize).once(); + if(!usesLuaScript) + MOCK_EXPECT(callbacks.CI_NextConnectState).with(ConnectState::VerifyMap).once(); clientMsgInterface.OnGameMessage( - GameMessage_Map_Data(true, 10, mapInfo.mapData.data.data() + 10, mapInfo.mapData.data.size() - 10)); + GameMessage_Map_Data(true, 10, mapInfo.mapData.data.data() + 10, mapDataSize - 10)); + if(usesLuaScript) + { + // First part of lua + MOCK_EXPECT(callbacks.CI_MapPartReceived).with(mapDataSize + 5u, totalSize).once(); + clientMsgInterface.OnGameMessage(GameMessage_Map_Data(false, 0, mapInfo.luaData.data.data(), 5)); + BOOST_TEST(client.GetMainPlayer().sendQueue.empty()); + // Remaining part of lua -> Transmission done + MOCK_EXPECT(callbacks.CI_MapPartReceived).with(totalSize, totalSize).once(); + MOCK_EXPECT(callbacks.CI_NextConnectState).with(ConnectState::VerifyMap).once(); + clientMsgInterface.OnGameMessage( + GameMessage_Map_Data(false, 5, mapInfo.luaData.data.data() + 5, luaDataSize - 5)); + } + const auto msg = boost::dynamic_pointer_cast(client.GetMainPlayer().sendQueue.pop()); BOOST_TEST_REQUIRE(msg); BOOST_TEST(msg->mapChecksum == mapInfo.mapChecksum); - BOOST_TEST(msg->luaChecksum == 0u); + BOOST_TEST(msg->luaChecksum == mapInfo.luaChecksum); BOOST_TEST(bfs::exists(RTTRCONFIG.ExpandPath(s25::folders::mapsPlayed) / testMapPath.filename())); + if(usesLuaScript) + BOOST_TEST(bfs::exists(RTTRCONFIG.ExpandPath(s25::folders::mapsPlayed) / testLuaPath.filename())); } { MOCK_EXPECT(callbacks.CI_NextConnectState).with(ConnectState::QueryServerName).once(); @@ -141,3 +169,42 @@ BOOST_AUTO_TEST_CASE(ClientFollowsConnectProtocol) BOOST_TEST_REQUIRE(client.GetState() == ClientState::Config); } } + +BOOST_AUTO_TEST_CASE(ClientDetectsMapBufferOverflow) +{ + rttr::test::LogAccessor _suppressLogOutput; + GameClient client; + GameMessageInterface& clientMsgInterface = client; + MockClientInterface callbacks; + client.SetInterface(&callbacks); + TestServer server; + const auto serverPort = server.tryListen(); + BOOST_TEST_REQUIRE(serverPort >= 0); + const auto pw = rttr::test::randString(10); + const auto serverType = rttr::test::randomEnum(); + mock::sequence s; + MOCK_EXPECT(callbacks.CI_NextConnectState).in(s).with(ConnectState::Initiated).once(); + MOCK_EXPECT(callbacks.CI_NextConnectState).in(s).with(ConnectState::VerifyServer).once(); + MOCK_EXPECT(callbacks.CI_NextConnectState).in(s).with(ConnectState::QueryPw).once(); + MOCK_EXPECT(callbacks.CI_NextConnectState).in(s).with(ConnectState::QueryMapInfo).once(); + MOCK_EXPECT(callbacks.CI_NextConnectState).in(s).with(ConnectState::ReceiveMap).once(); + + BOOST_TEST_REQUIRE(client.Connect("localhost", pw, serverType, serverPort, false, false)); + clientMsgInterface.OnGameMessage(GameMessage_Player_Id(1)); + clientMsgInterface.OnGameMessage(GameMessage_Server_TypeOK(GameMessage_Server_TypeOK::StatusCode::Ok, "")); + clientMsgInterface.OnGameMessage(GameMessage_Server_Password("true")); + + const auto mapDataSize = rttr::test::randomValue(10u, 100u); + std::vector mapData(mapDataSize); + clientMsgInterface.OnGameMessage(GameMessage_Map_Info("testMap.swd", MapType::OldMap, 500u, mapDataSize, 0, 0)); + // First part of map + MOCK_EXPECT(callbacks.CI_MapPartReceived).in(s).with(10u, mapDataSize).once(); + clientMsgInterface.OnGameMessage(GameMessage_Map_Data(true, 0, mapData.data(), 10)); + BOOST_TEST_REQUIRE(mock::verify()); + BOOST_TEST(client.GetState() == ClientState::Connect); + + // Remaining part of map but to big/wrong offset + MOCK_EXPECT(callbacks.CI_Error).with(ClientError::MapTransmission).once(); + clientMsgInterface.OnGameMessage(GameMessage_Map_Data(true, 10, mapData.data(), mapDataSize - 9)); + BOOST_TEST(client.GetState() == ClientState::Stopped); +}