Verify ZIP symlink data only after its bounds check in PureLib

src/AppInstallerCLITests/Archive.cpp

@@ -4,10 +4,90 @@
 #include "TestCommon.h"
 #include <winget/Archive.h>
 
+// Defined in PureLib, which is compiled into AppInstallerCommonCore.
+int pure_zip(const uint8_t* buffer, const uint64_t size, const uint64_t flags);
+const char* pure_error_code(const int error);
+
 using namespace AppInstaller::Archive;
 using namespace TestCommon;
 
 constexpr std::string_view s_ZipFile = "TestZip.zip";
+constexpr std::string_view s_SymlinkName = "link";
+constexpr std::string_view s_SymlinkTarget = "target.txt";
+
+namespace
+{
+    void AppendUInt16(std::vector<uint8_t>& zip, uint16_t value)
+    {
+        zip.push_back(static_cast<uint8_t>(value));
+        zip.push_back(static_cast<uint8_t>(value >> 8));
+    }
+
+    void AppendUInt32(std::vector<uint8_t>& zip, uint32_t value)
+    {
+        AppendUInt16(zip, static_cast<uint16_t>(value));
+        AppendUInt16(zip, static_cast<uint16_t>(value >> 16));
+    }
+
+    void AppendString(std::vector<uint8_t>& zip, std::string_view value)
+    {
+        zip.insert(zip.end(), value.begin(), value.end());
+    }
+
+    // Fields shared by the local file header and the central directory header.
+    void AppendEntryFields(std::vector<uint8_t>& zip, uint32_t dataSize)
+    {
+        AppendUInt16(zip, 20); // Version needed
+        AppendUInt16(zip, 0); // Flags
+        AppendUInt16(zip, 0); // Stored
+        AppendUInt16(zip, 0); // Time
+        AppendUInt16(zip, 0x5C21); // Date
+        AppendUInt32(zip, 0x00A4BA49); // CRC-32 of s_SymlinkTarget
+        AppendUInt32(zip, dataSize); // Compressed size
+        AppendUInt32(zip, dataSize); // Uncompressed size
+        AppendUInt16(zip, static_cast<uint16_t>(s_SymlinkName.size()));
+        AppendUInt16(zip, 0); // Extra length
+    }
+
+    // Creates a ZIP with a stored Unix symlink whose headers declare dataSize bytes of target data.
+    std::vector<uint8_t> CreateSymlinkZip(uint32_t dataSize)
+    {
+        std::vector<uint8_t> zip;
+
+        AppendUInt32(zip, 0x04034B50); // Local file header
+        AppendEntryFields(zip, dataSize);
+        AppendString(zip, s_SymlinkName);
+        AppendString(zip, s_SymlinkTarget);
+
+        uint32_t centralDirectoryOffset = static_cast<uint32_t>(zip.size());
+        AppendUInt32(zip, 0x02014B50); // Central directory header
+        AppendUInt16(zip, 0x0314); // Made by Unix
+        AppendEntryFields(zip, dataSize);
+        AppendUInt16(zip, 0); // Comment length
+        AppendUInt16(zip, 0); // Disk
+        AppendUInt16(zip, 0); // Internal attributes
+        AppendUInt32(zip, 0xA1FF0000); // Unix symlink mode
+        AppendUInt32(zip, 0); // Local header offset
+        AppendString(zip, s_SymlinkName);
+
+        uint32_t centralDirectorySize = static_cast<uint32_t>(zip.size()) - centralDirectoryOffset;
+        AppendUInt32(zip, 0x06054B50); // End of central directory
+        AppendUInt16(zip, 0); // Disk
+        AppendUInt16(zip, 0); // Central directory disk
+        AppendUInt16(zip, 1); // Entries on disk
+        AppendUInt16(zip, 1); // Total entries
+        AppendUInt32(zip, centralDirectorySize);
+        AppendUInt32(zip, centralDirectoryOffset);
+        AppendUInt16(zip, 0); // Comment length
+
+        return zip;
+    }
+
+    std::string_view GetScanResult(const std::vector<uint8_t>& zip)
+    {
+        return pure_error_code(pure_zip(zip.data(), zip.size(), 0));
+    }
+}
 
 TEST_CASE("Extract_ZipArchive", "[archive]")
 {
@@ -32,3 +112,20 @@ TEST_CASE("Scan_ZipArchive", "[archive]")
     bool result = ScanZipFile(testZipPath);
     REQUIRE(result);
 }
+
+TEST_CASE("Scan_ZipArchive_Symlink", "[archive]")
+{
+    auto zip = CreateSymlinkZip(static_cast<uint32_t>(s_SymlinkTarget.size()));
+    REQUIRE(GetScanResult(zip) == "PURE_E_OK");
+}
+
+TEST_CASE("Scan_ZipArchive_SymlinkDataPastEnd", "[archive]")
+{
+    auto zip = CreateSymlinkZip(static_cast<uint32_t>(s_SymlinkTarget.size()));
+
+    // Declare one byte more than the buffer holds after the 30-byte local file header and name.
+    auto dataSize = zip.size() - 30 - s_SymlinkName.size() + 1;
+    zip = CreateSymlinkZip(static_cast<uint32_t>(dataSize));
+
+    REQUIRE(GetScanResult(zip) == "PURE_E_ZIP_LFH_DATA_OVERFLOW");
+}

src/PureLib/pure/pure.h

@@ -1,3 +1,5 @@
+// This file has been modified for WinGet. See src/PureLib/readme.md for the local changes.
+
 // pure.h - C99 interface:
 
 #ifndef PURE_H
@@ -1658,10 +1660,6 @@ int pure_zip_meta(
       int error = pure_zip_diff_cdh_lfh(&cdh, &lfh);
       if (error) return error;
     }
-    {
-      int error = pure_zip_verify_symlink(&cdh, &lfh, buffer);
-      if (error) return error;
-    }
     assert(lfh.length >= PURE_ZIP_LFH_MIN);
     lfh_offset += lfh.length;
     // File Data (compressed or uncompressed):
@@ -1670,6 +1668,11 @@ int pure_zip_meta(
     }
     lfh_offset += cdh.compressed_size;
     if (lfh_offset > size) return PURE_E_ZIP_LFH_DATA_OVERFLOW;
+    // Verify symlink data only after checking that it is within the buffer:
+    {
+      int error = pure_zip_verify_symlink(&cdh, &lfh, buffer);
+      if (error) return error;
+    }
     // Data Descriptor Record (optional):
     if (lfh.general_purpose_bit_flag & (1 << 3)) {
       {

src/PureLib/readme.md

@@ -7,6 +7,13 @@ It is created using git subtree command:
 
 The PureLib.vcxitems is created to make pure code compiled as part of the WinGet solution.
 
+#### Local changes
+
+Keep these changes to pure.h when updating the subtree:
+
+- It includes `zlib.h` instead of `zlib/zlib.h`, since zlib comes from vcpkg.
+- `pure_zip_meta` verifies symlink data only after checking that the data is within the buffer.
+
 #### Steps used to create the VS project files.
 
 1. VS Create project from existing code wizard to create a Shared Items template.