ZIP: Fix infinite loop + significant code cleanup

An infinite loop may occur when scanning some malformed ZIP files.

I introduced this issue in 96c00b6d80
with this line:

```c
// decrement coff by 1 to account for the increment at the end of the loop
coff -= 1;
```

The problem is that the function may return 0, which should
indicate that there are no more files. The result was that
`coff` would stay the same and the loop would repeat.

This issue is in 1.5 development and affects the 1.5.0 beta but
does not affect any production versions.

Fixes: https://github.com/Cisco-Talos/clamav/issues/1534

Special thanks to Sophie0x2E for an initial fix, proposed in
https://github.com/Cisco-Talos/clamav/pull/1539
In review, I was uncomfortable with other existing code and
decided to to a more significant overhaul of the error handling
in the ZIP module.

In addition to cleanup, this commit has some functional changes:

- When parsing a central directory file header inside of
  `parse_central_directory_file_header()`, it will now fail out if the
  "extra length" or "comment length" fields would exceced the length of
  the archive. That doesn't mean the associated local file header won't
  be parsed later, but it won't use the central directory file header
  to find it. Instead, the ZIP module will have to find the local file
  header by searching for extra records not listed in the central directory.

  This change was mostly to tidy up complex error handling.

- Add two FTM new signatures to identify split ZIP archives.

  This signature identifies the first segment (first file) in a split or
  spanned ZIP archive. It may also be found on a single-segment "split"
  archive, depending on the ZIP archiver.
  ```
  0:0:504b0708504b0304:ZIP (First segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP
  ```

  Practically speaking, this new signature makes it so ClamAV identifies
  the file as a ZIP right away without having to rely on SFX_ZIP detection.
  Extraction is then handled by the ZIP `cli_unzip` function rather than
  extracting each with `cli_unzip_single` which handles SFX_ZIP entries.

  Note: ClamAV isn't capable of finding additional files on disk to support
  handling the additional segments. So it doesn't make any difference with
  handling those other files.

  This signature is for single-segment split/spanned archives, depending
  on the ZIP archiver.
  ```
  0:0:504b0303504b0304:ZIP (Single-segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP
  ```
  Like the first one, this also means we won't rely on SFX_ZIP detection
  and will treat this files as regular ZIPs.

- Added a test file to verify that ClamAV can extract a single-file
  "split" ZIP.

- Added a clamscan test with test files to verify that scanning a split
  archive across two segments correctly extracts the properly formed zip
  file entries. Sadly, we can't join the segments to extract everything.

libclamav/filetypes_int.h

@@ -324,5 +324,7 @@ static const char *ftypes_int[] = {
     "1:0:08??12??6f6e6e782d636166666532:ONNX AI Model File:CL_TYPE_ANY:CL_TYPE_AI_MODEL:220",
     // tflite model detection
     "0:4:54464c33:TensorFlow Lite Model File:CL_TYPE_ANY:CL_TYPE_AI_MODEL:220",
+    "0:0:504b0708504b0304:ZIP (First segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP",
+    "0:0:504b0303504b0304:ZIP (Single-segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP",
     NULL};
 #endif

libclamav/unzip.h

@@ -19,6 +19,47 @@
  *  MA 02110-1301, USA.
  */
 
+/*
+ * General Structure for PKZIP files:
+ * +---------------------------------------------------------------+
+ * | Local file header 1                                           |
+ * +---------------------------------------------------------------+
+ * | File data 1                                                   |
+ * +---------------------------------------------------------------+
+ * | Data descriptor 1 (optional)                                  |
+ * +---------------------------------------------------------------+
+ * | Local file header 2                                           |
+ * +---------------------------------------------------------------+
+ * | File data 2                                                   |
+ * +---------------------------------------------------------------+
+ * | Data descriptor 2 (optional)                                  |
+ * +---------------------------------------------------------------+
+ * | ...                                                           |
+ * +---------------------------------------------------------------+
+ * | Local file header N                                           |
+ * +---------------------------------------------------------------+
+ * | File data N                                                   |
+ * +---------------------------------------------------------------+
+ * | Data descriptor N (optional)                                  |
+ * +---------------------------------------------------------------+
+ * | Archive Decryption Header (optional, v6.2+)                   |
+ * +---------------------------------------------------------------+
+ * | Archive Extra Data Record (optional, v6.2+)                   |
+ * +---------------------------------------------------------------+
+ * | Central directory                                             |
+ * +---------------------------------------------------------------+
+ *
+ * This and additional diagrams from courtesy of:
+ * https://users.cs.jmu.edu/buchhofp/forensics/formats/pkzip.html
+ *
+ * See also:
+ * https://www.pkware.com/documents/casestudies/APPNOTE.TXT
+ *
+ * And see also: unzip.h
+ *
+ * Note the diagrams and current implemementation do not implement all features.
+ */
+
 #ifndef __UNZIP_H
 #define __UNZIP_H
 
@@ -28,11 +69,26 @@
 
 #include "others.h"
 
-// Callback is the same as cli_magic_scan_desc
+/**
+ * @brief Callback function type for handling extracted files.
+ *
+ * The `unzip_single_internal` function lets you specify a callback function to handle the extracted file.
+ * Other functions like `cli_unzip` and `cli_unzip_single` use `zip_scan_cb` as the default callback and
+ * thus just scan the file.
+ *
+ * Note: The callback function must match the signature of `cli_magic_scan_desc`.
+ */
 typedef cl_error_t (*zip_cb)(int fd, const char *filepath, cli_ctx *ctx, const char *name, uint32_t attributes);
 #define zip_scan_cb cli_magic_scan_desc
 
 #define MAX_ZIP_REQUESTS 10
+
+/**
+ * @brief Structure to hold zip file search requests.
+ *
+ * This structure is used to hold multiple file names that we want to search for in a zip archive.
+ * It is used by the `unzip_search` function.
+ */
 struct zip_requests {
     const char *names[MAX_ZIP_REQUESTS];
     size_t namelens[MAX_ZIP_REQUESTS];
@@ -42,16 +98,91 @@ struct zip_requests {
     int found, match;
 };
 
-cl_error_t cli_unzip(cli_ctx *);
-cl_error_t unzip_single_internal(cli_ctx *, off_t, zip_cb);
-cl_error_t cli_unzip_single(cli_ctx *, off_t);
+/**
+ * @brief Unzip a zip file.
+ *
+ * Scan each extracted file.
+ *
+ * @param ctx                   The scan context containing the file map and other scan parameters.
+ * @return cl_error_t           Returns CL_SUCCESS on success, or an error code on failure.
+ */
+cl_error_t cli_unzip(cli_ctx *ctx);
 
-cl_error_t unzip_search_add(struct zip_requests *, const char *, size_t);
-cl_error_t unzip_search(cli_ctx *, fmap_t *, struct zip_requests *);
-cl_error_t unzip_search_single(cli_ctx *, const char *, size_t, uint32_t *);
+/**
+ * @brief Unzip a single file from a zip archive.
+ *
+ * Scan the file after extracting it.
+ *
+ * @param ctx                   The scan context containing the file map and other scan parameters.
+ * @param local_header_offset   The offset of the local file header in the zip archive.
+ * @return cl_error_t           Returns CL_SUCCESS on success, or an error code on failure.
+ */
+cl_error_t cli_unzip_single(cli_ctx *ctx, off_t local_header_offset);
+
+/**
+ * @brief Unzip a single file from a zip archive.
+ *
+ * Different from `cli_unzip_single`, this function allows for a custom callback to be used after extraction.
+ * In other words, it can be used to extract a file without scanning it immediately.
+ * This is useful for cases where you want to handle the file differently.
+ *
+ * @param ctx                   The scan context containing the file map and other scan parameters.
+ * @param local_header_offset   The offset of the local file header in the zip archive.
+ * @param zcb                   The callback function to invoke after extraction. See `zip_scan_cb`.
+ * @return cl_error_t           Returns CL_SUCCESS on success, or an error code on failure.
+ */
+cl_error_t unzip_single_internal(cli_ctx *ctx, off_t local_header_offset, zip_cb zcb);
+
+/**
+ * @brief Add a file a bundle of files to search for in a zip archive.
+ *
+ * @param requests              The `zip_requests` structure to modify.
+ * @param name                  The name of the file to add.
+ * @param nlen                  The length of the file name.
+ * @return cl_error_t           Returns CL_SUCCESS on success, or an error code on failure.
+ */
+cl_error_t unzip_search_add(struct zip_requests *requests, const char *name, size_t nlen);
+
+/**
+ * @brief Search for files in a zip archive.
+ *
+ * This function searches for one or more files in a zip archive and scans them.
+ *
+ * Disclaimer: As compared with `cli_unzip`, this function depends on the central directory header.
+ *             It will not work correctly if the zip archive does not have a central directory header
+ *             or the file you're looking for is not listed in the central directory.
+ *
+ * @param ctx                   The scan context containing the file map and other scan parameters.
+ * @param map                   The file map to search in.
+ * @param requests              The `zip_requests` structure containing the files to search for.
+ * @return cl_error_t           Returns CL_SUCCESS if nothing was found.
+ *                              Returns CL_VIRUS if a match was found.
+ *                              Returns a CL_E* error code on failure.
+ */
+cl_error_t unzip_search(cli_ctx *ctx, fmap_t *map, struct zip_requests *requests);
+
+/**
+ * @brief Search for a single file in a zip archive.
+ *
+ * This function searches for a single file in a zip archive.
+ *
+ * Disclaimer: As compared with `cli_unzip`, this function depends on the central directory header.
+ *             It will not work correctly if the zip archive does not have a central directory header
+ *             or the file you're looking for is not listed in the central directory.
+ *
+ * @param ctx                   The scan context containing the file map and other scan parameters.
+ * @param name                  The name of the file to search for.
+ * @param nlen                  The length of the file name.
+ * @param loff                  The offset of the file in the zip archive.
+ * @return cl_error_t           Returns CL_SUCCESS if nothing was found.
+ *                              Returns CL_VIRUS if a match was found.
+ *                              Returns a CL_E* error code on failure.
+ */
+cl_error_t unzip_search_single(cli_ctx *ctx, const char *name, size_t nlen, uint32_t *loff);
 
 // clang-format off
 #ifdef UNZIP_PRIVATE
+
 #define F_ENCR  (1<<0)
 #define F_ALGO1 (1<<1)
 #define F_ALGO2 (1<<2)
@@ -96,22 +227,34 @@ enum ALGO {
 };
 
 /*
- * Local File Header format:
+ * Local File Header Structure:
+ *
+ *  0x0 0x1 0x2 0x3 0x4 0x5 0x6 0x7 0x8 0x9 0xa 0xb 0xc 0xd 0xe 0xf
+ * +---------------+-------+-------+-------+-------+-------+-------+
+ * | P K 0x03 0x04 | Vers  | Flags | Compr |Mod Tm |Mod Dt | CRC 32|
+ * +-------+-------+-------+-------+-------+-------+-------+-------+
+ * | CRC 32| Compr Size    | Uncompr Size  |FName L|Extra L|       |
+ * +-------+---------------+---------------+-------+-------+       +
+ * | File Name (variable)                                          |
+ * +---------------------------------------------------------------+
+ * | Extra field (variable)                                        |
+ * +---------------------------------------------------------------+
  */
-/* struct LH { */
-/*   uint32_t magic; */
-/*   uint16_t version; */
-/*   uint16_t flags; */
-/*   uint16_t method; */
-/*   uint32_t mtime; */
-/*   uint32_t crc32; */
-/*   uint32_t csize; */
-/*   uint32_t usize; */
-/*   uint16_t flen; */
-/*   uint16_t elen; */
-/*   char fname[flen] */
-/*   char extra[elen] */
-/* } __attribute__((packed)); */
+
+// struct LH {
+//   uint32_t magic;
+//   uint16_t version;
+//   uint16_t flags;
+//   uint16_t method;
+//   uint32_t mtime;
+//   uint32_t crc32;
+//   uint32_t csize;
+//   uint32_t usize;
+//   uint16_t flen;
+//   uint16_t elen;
+//   char fname[flen]
+//   char extra[elen]
+// } __attribute__((packed));
 
 /*
  * Local File Header convenience macros:
@@ -131,32 +274,79 @@ enum ALGO {
 // clang-format on
 
 /*
- * Central Directory File Header format:
+ * Central Directory Structure:
+ *
+ * +---------------------------------------------------------------+
+ * | Central directory file header 1                               |
+ * +---------------------------------------------------------------+
+ * | Central directory file header 2                               |
+ * +---------------------------------------------------------------+
+ * | ...                                                           |
+ * +---------------------------------------------------------------+
+ * | Central directory file header N                               |
+ * +---------------------------------------------------------------+
+ * | Digital Signature                                             |
+ * +---------------------------------------------------------------+
+ * | Data descriptor N (optional)                                  |
+ * +---------------------------------------------------------------+
+ * | Zip64 end of central directory Record                         |
+ * +---------------------------------------------------------------+
+ * | Zip64 end of central directory locator                        |
+ * +---------------------------------------------------------------+
+ * | End of central directory record                               |
+ * +---------------------------------------------------------------+
+ *
+ * Central Directory File Header structure:
+ *
+ *  0x0 0x1 0x2 0x3 0x4 0x5 0x6 0x7 0x8 0x9 0xa 0xb 0xc 0xd 0xe 0xf
+ * +---------------+-------+-------+-------+-------+-------+-------+
+ * | P K 0x01 0x02 | Vers  |Vers Nd| Flags | Compr |Mod Tm |Mod Dt |
+ * +---------------+-------+-------+-------+-------+-------+-------+
+ * | CRC 32        | Compr Size    | Uncompr Size  |FName L|Extra L|
+ * +---------------+-------+-------+-------+-------+-------+-------+
+ * |F Com L|D #strt|IntAttr|Ext Attributes |Offset L Header|       |
+ * +---------------+-----------------------+---------------+       +
+ * | File Name (variable)                                          |
+ * +---------------------------------------------------------------+
+ * | Extra field (variable)                                        |
+ * +---------------------------------------------------------------+
+ * | File comment (variable)                                       |
+ * +---------------------------------------------------------------+
+ *
+ * End of central directory record structure:
+ *
+ *  0x0 0x1 0x2 0x3 0x4 0x5 0x6 0x7 0x8 0x9 0xa 0xb 0xc 0xd 0xe 0xf
+ * +---------------+-------+-------+-------+-------+---------------+
+ * | P K 0x05 0x06 |Disk # |Dsk#cd |DskEnts|T.Entrs|Central Dir Sz |
+ * +---------------+-------+-------+-------+-------+---------------+
+ * | Offset of CD  |CommLen| ZIP file comment (variable)           |
+ * +---------------+-------+-------+-------+-------+-------+-------+
  */
-/* struct CH { */
-/*   uint32_t magic; */
-/*   uint16_t vermade; */
-/*   uint16_t verneed; */
-/*   uint16_t flags; */
-/*   uint16_t method; */
-/*   uint32_t mtime; */
-/*   uint32_t crc32; */
-/*   uint32_t csize; */
-/*   uint32_t usize; */
-/*   uint16_t flen; */
-/*   uint16_t elen; */
-/*   uint16_t clen; */
-/*   uint16_t dsk; */
-/*   uint16_t iattrib; */
-/*   uint32_t eattrib; */
-/*   uint32_t off; */
-/*   char fname[flen] */
-/*   char extra[elen] */
-/*   char comment[clen] */
-/* } __attribute__((packed)); */
+
+// struct CH {
+//   uint32_t magic;
+//   uint16_t vermade;
+//   uint16_t verneed;
+//   uint16_t flags;
+//   uint16_t method;
+//   uint32_t mtime;
+//   uint32_t crc32;
+//   uint32_t csize;
+//   uint32_t usize;
+//   uint16_t flen;
+//   uint16_t elen;
+//   uint16_t clen;
+//   uint16_t dsk;
+//   uint16_t iattrib;
+//   uint32_t eattrib;
+//   uint32_t off;
+//   char fname[flen]
+//   char extra[elen]
+//   char comment[clen]
+// } __attribute__((packed));
 
 /*
- * Central Directory File Header convenience macro's:
+ * Central Directory File Header convenience macro's.
  */
 // clang-format off
 #define CENTRAL_HEADER_magic        ((uint32_t)cli_readint32((uint8_t *)(central_header)+0))
@@ -177,8 +367,10 @@ enum ALGO {
 #define CENTRAL_HEADER_off          ((uint32_t)cli_readint32((uint8_t *)(central_header)+42))
 // clang-format on
 
-#define SIZEOF_CENTRAL_HEADER 46
-#define SIZEOF_ENCRYPTION_HEADER 12
+#define SIZEOF_CENTRAL_HEADER 46    // Excluding variable size fields
+#define SIZEOF_ENCRYPTION_HEADER 12 // Excluding variable size fields
+#define SIZEOF_END_OF_CENTRAL 22    // Excluding variable size fields
+
 #endif /* UNZIP_PRIVATE */
 
 #endif /* __UNZIP_H */

unit_tests/check_clamav.c

@@ -536,7 +536,7 @@ END_TEST
 static char **testfiles     = NULL;
 static unsigned testfiles_n = 0;
 
-static const int expected_testfiles = 52;
+static const int expected_testfiles = 53;
 
 static unsigned skip_files(void)
 {

unit_tests/clamscan/assorted_test.py

@@ -282,3 +282,80 @@ class TC(testcase.TestCase):
         expected_results.append('Scanned files: 3')
         expected_results.append('Infected files: 0')
         self.verify_output(output.out, expected=expected_results)
+
+    def test_split_zip(self):
+        self.step_name('Test scanning a split zip archive containing 4 identical logo files.')
+
+        # For context, the zip utility won't make splits smaller than 64k.
+        # I used a folder with 4 copies of the same logo.png file, and then used the zip utility to create a split zip archive.
+        # The split zip archive segments are "logos.z01" and "logos.zip".
+        #
+        # The logos.z01 file is the first segment, and it contains the first 64k of the zip archive.
+        # This includes "logo.2.png", "logo.1.png", and a malformed portion of "logo.4.png" files.
+        # The first part has the identifying magic at the start, so we recognize it as a zip archive.
+        #
+        # The logos.zip file is the second segment, and it contains the remaining 36k of the zip archive.
+        # This includes a malformed portion of "logo.4.png" and "logo.3.png" and the zip archive's central directory.
+        # The second part does not have the identifying magic at the start, so we discover "logo.3.png" through ZIP_SFX
+        # embedded file type recognition.
+
+        (TC.path_tmp / 'logo.png.ldb').write_text(
+            "logo.png;Engine:150-255,Target:0;0;fuzzy_img#af2ad01ed42993c7#0\n"
+        )
+
+        first_file = TC.path_source / 'unit_tests' / 'input' / 'other_scanfiles' / 'zip' / 'logos.z01'
+        second_file = TC.path_source / 'unit_tests' / 'input' / 'other_scanfiles' / 'zip' / 'logos.zip'
+
+        # Scan the first segment of the split zip archive.
+        command = '{valgrind} {valgrind_args} {clamscan} -d {path_db} {testfiles} --allmatch --gen-json --debug'.format(
+            valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, clamscan=TC.clamscan,
+            path_db=TC.path_tmp / 'logo.png.ldb',
+            testfiles=first_file,
+        )
+        output = self.execute_command(command)
+
+        assert output.ec == 1  # virus
+
+        expected_stdout = [
+            'logos.z01: logo.png.UNOFFICIAL FOUND',
+        ]
+        self.verify_output(output.out, expected=expected_stdout)
+
+        expected_stderr = [
+            '"FileName":"logo.2.png",',
+            '"FileName":"logo.1.png",',
+        ]
+        # The "logo.4.png" file is split between this segment and the next, so it can't be extracted.
+        # The "logo.3.png" file is not in this segment, so it won't be reported either.
+        unexpected_stdout = [
+            '"FileName":"logo.3.png",',
+            '"FileName":"logo.4.png",',
+        ]
+        self.verify_output(output.err, expected=expected_stderr)
+
+        # Scan the second segment of the split zip archive.
+        command = '{valgrind} {valgrind_args} {clamscan} -d {path_db} {testfiles} --allmatch --gen-json --debug'.format(
+            valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, clamscan=TC.clamscan,
+            path_db=TC.path_tmp / 'logo.png.ldb',
+            testfiles=second_file,
+        )
+        output = self.execute_command(command)
+
+        assert output.ec == 1  # virus
+
+        expected_stdout = [
+            'logos.zip: logo.png.UNOFFICIAL FOUND',
+        ]
+        self.verify_output(output.out, expected=expected_stdout)
+
+        expected_stderr = [
+            '"FileName":"logo.3.png",',
+        ]
+        # The "logo.4.png" file is split between this segment and the first, so it can't be extracted.
+        # The "logo.2.png" and "logo.1.png" files are not in this segment, so they won't be reported either.
+        unexpected_stdout = [
+            '"FileName":"logo.4.png",',
+            '"FileName":"logo.2.png",',
+            '"FileName":"logo.1.png",',
+        ]
+        self.verify_output(output.err, expected=expected_stderr)

unit_tests/input/CMakeLists.txt

@@ -57,6 +57,7 @@ set(ENCRYPTED_TESTFILES
     clamav_hdb_scanfiles/clam.exe.2007.one
     clamav_hdb_scanfiles/clam.exe.2010.one
     clamav_hdb_scanfiles/clam.exe.webapp-export.one
+    clamav_hdb_scanfiles/clam.split.oneseg.zip
     signing/sign/signing-test.key
 )