From 921ad28a30fc9f0716c32e4c85e68f747b79a14a Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Thu, 30 Dec 2021 19:37:06 +0000 Subject: [PATCH 1/2] Fix fuzzy test on too large command Device might return I/O error instead of FAIL message if command send to device is too large. Since maximum command size in fastboto protocol is 64 bytes. The device might only try to read 64 bytes for the bulk transfer, sending data more than that might result in USB I/O error. Do proper handler for that in fuzzy_fastboot and reset the USB if I/O error encounter during the Comman.dTooLarge test Test: adb reboot fastboot fuzzy_fastboot --gtest_filter=Fuzz.CommandTooLarge fuzzy_fastboot --gtest_filter=Fuzz.BadCommandTooLarge Bug: 212628476 Change-Id: I3612e131de02435ee3ed7d18f2b2d20b50ae6c3f --- fastboot/fuzzy_fastboot/main.cpp | 39 +++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/fastboot/fuzzy_fastboot/main.cpp b/fastboot/fuzzy_fastboot/main.cpp index b6beaf93a..8593adc87 100644 --- a/fastboot/fuzzy_fastboot/main.cpp +++ b/fastboot/fuzzy_fastboot/main.cpp @@ -890,28 +890,51 @@ TEST_F(Fuzz, GetVarAllSpam) { TEST_F(Fuzz, BadCommandTooLarge) { std::string s = RandomString(FB_COMMAND_SZ + 1, rand_legal); - EXPECT_EQ(fb->RawCommand(s), DEVICE_FAIL) + RetCode ret = fb->RawCommand(s); + EXPECT_TRUE(ret == DEVICE_FAIL || ret == IO_ERROR) << "Device did not respond with failure after sending length " << s.size() << " string of random ASCII chars"; + if (ret == IO_ERROR) EXPECT_EQ(transport->Reset(), 0) << "USB reset failed"; std::string s1 = RandomString(1000, rand_legal); - EXPECT_EQ(fb->RawCommand(s1), DEVICE_FAIL) + ret = fb->RawCommand(s1); + EXPECT_TRUE(ret == DEVICE_FAIL || ret == IO_ERROR) << "Device did not respond with failure after sending length " << s1.size() << " string of random ASCII chars"; + if (ret == IO_ERROR) EXPECT_EQ(transport->Reset(), 0) << "USB reset failed"; std::string s2 = RandomString(1000, rand_illegal); - EXPECT_EQ(fb->RawCommand(s2), DEVICE_FAIL) - << "Device did not respond with failure after sending length " << s1.size() + ret = fb->RawCommand(s2); + EXPECT_TRUE(ret == DEVICE_FAIL || ret == IO_ERROR) + << "Device did not respond with failure after sending length " << s2.size() << " string of random non-ASCII chars"; + if (ret == IO_ERROR) EXPECT_EQ(transport->Reset(), 0) << "USB reset failed"; std::string s3 = RandomString(1000, rand_char); - EXPECT_EQ(fb->RawCommand(s3), DEVICE_FAIL) - << "Device did not respond with failure after sending length " << s1.size() + ret = fb->RawCommand(s3); + EXPECT_TRUE(ret == DEVICE_FAIL || ret == IO_ERROR) + << "Device did not respond with failure after sending length " << s3.size() << " string of random chars"; + if (ret == IO_ERROR) EXPECT_EQ(transport->Reset(), 0) << "USB reset failed"; + + std::string s4 = RandomString(10 * 1024 * 1024, rand_legal); + ret = fb->RawCommand(s); + EXPECT_TRUE(ret == DEVICE_FAIL || ret == IO_ERROR) + << "Device did not respond with failure after sending length " << s4.size() + << " string of random ASCII chars "; + if (ret == IO_ERROR) EXPECT_EQ(transport->Reset(), 0) << "USB reset failed"; + + ASSERT_TRUE(UsbStillAvailible()) << USB_PORT_GONE; + std::string resp; + EXPECT_EQ(fb->GetVar("product", &resp), SUCCESS) + << "Device is unresponsive to getvar command"; } TEST_F(Fuzz, CommandTooLarge) { for (const std::string& s : CMDS) { std::string rs = RandomString(1000, rand_char); - EXPECT_EQ(fb->RawCommand(s + rs), DEVICE_FAIL) - << "Device did not respond with failure after '" << s + rs << "'"; + RetCode ret; + ret = fb->RawCommand(s + rs); + EXPECT_TRUE(ret == DEVICE_FAIL || ret == IO_ERROR) + << "Device did not respond with failure " << ret << "after '" << s + rs << "'"; + if (ret == IO_ERROR) EXPECT_EQ(transport->Reset(), 0) << "USB reset failed"; ASSERT_TRUE(UsbStillAvailible()) << USB_PORT_GONE; std::string resp; EXPECT_EQ(fb->GetVar("product", &resp), SUCCESS) From 3724bbcbe99cbe6d3697d19e5d1658838da3a179 Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Thu, 30 Dec 2021 20:08:04 +0000 Subject: [PATCH 2/2] Fix userspace fastboot with fuzzy test Add more checking for fastboot to detect malformed requests. Such as checking no control characters in the command send from host. Make sure the download command length is eight bytes. And report FAIL if download length is zero. Test: adb reboot fastboot fuzzy_fastboot --gtest_filter=Fuzz.DownloadInvalid1 fuzzy_fastboot --gtest_filter=Fuzz.DownloadInvalid2 fuzzy_fastboot --gtest_filter=Fuzz.DownloadInvalid7 fuzzy_fastboot --gtest_filter=Fuzz.DownloadInvalid8 Bug: 212628476 Change-Id: I750174205377395b5328923fb00462d078f3310d --- fastboot/device/commands.cpp | 8 ++++++++ fastboot/device/fastboot_device.cpp | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/fastboot/device/commands.cpp b/fastboot/device/commands.cpp index 4042531e5..b9f6c973c 100644 --- a/fastboot/device/commands.cpp +++ b/fastboot/device/commands.cpp @@ -268,10 +268,18 @@ bool DownloadHandler(FastbootDevice* device, const std::vector& arg } // arg[0] is the command name, arg[1] contains size of data to be downloaded + // which should always be 8 bytes + if (args[1].length() != 8) { + return device->WriteStatus(FastbootResult::FAIL, + "Invalid size (length of size != 8)"); + } unsigned int size; if (!android::base::ParseUint("0x" + args[1], &size, kMaxDownloadSizeDefault)) { return device->WriteStatus(FastbootResult::FAIL, "Invalid size"); } + if (size == 0) { + return device->WriteStatus(FastbootResult::FAIL, "Invalid size (0)"); + } device->download_data().resize(size); if (!device->WriteStatus(FastbootResult::DATA, android::base::StringPrintf("%08x", size))) { return false; diff --git a/fastboot/device/fastboot_device.cpp b/fastboot/device/fastboot_device.cpp index e6a834e59..ae225debc 100644 --- a/fastboot/device/fastboot_device.cpp +++ b/fastboot/device/fastboot_device.cpp @@ -186,6 +186,11 @@ void FastbootDevice::ExecuteCommands() { PLOG(ERROR) << "Couldn't read command"; return; } + if (std::count_if(command, command + bytes_read, iscntrl) != 0) { + WriteStatus(FastbootResult::FAIL, + "Command contains control character"); + continue; + } command[bytes_read] = '\0'; LOG(INFO) << "Fastboot command: " << command;