CVE-2021-30045: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: SerenityOS 2021-03-27 contains a buffer overflow vulnerability in the EndOfCentralDirectory::read() function.
CWE description: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 9.1
Published: 2021-04-06T13:15Z
Repository: https://github.com/SerenityOS/serenity
Language: C
Files changed: Userland/Libraries/LibArchive/Zip.h
Commit date: 2021-03-27 16:28:48 +0100
Commit message: LibArchive: Make bounds checks stricter in the Zip parser

We now also check we have enough space in the incoming buffer for the
various signatures and optional (length specified) fields. This helps
prevents a possible heap overflow read.

Diff: diff --git a/Userland/Libraries/LibArchive/Zip.h b/Userland/Libraries/LibArchive/Zip.h
index ac835b8bfa..66a25acf72 100644
--- a/Userland/Libraries/LibArchive/Zip.h
+++ b/Userland/Libraries/LibArchive/Zip.h
@@ -50,15 +50,17 @@ struct [[gnu::packed]] EndOfCentralDirectory {
     const u8* comment;
 
     bool read(ReadonlyBytes buffer)
     {
         auto fields_size = sizeof(EndOfCentralDirectory) - sizeof(u8*);
-        if (buffer.size() < fields_size)
+        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size)
             return false;
         if (memcmp(buffer.data(), end_of_central_directory_signature, sizeof(end_of_central_directory_signature)) != 0)
             return false;
         memcpy(reinterpret_cast<void*>(&disk_number), buffer.data() + sizeof(end_of_central_directory_signature), fields_size);
+        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size + comment_length)
+            return false;
         comment = buffer.data() + sizeof(end_of_central_directory_signature) + fields_size;
         return true;
     }
 
     void write(OutputStream& stream) const
@@ -99,15 +101,17 @@ struct [[gnu::packed]] CentralDirectoryRecord {
     const u8* comment;
 
     bool read(ReadonlyBytes buffer)
     {
         auto fields_size = sizeof(CentralDirectoryRecord) - (sizeof(u8*) * 3);
-        if (buffer.size() < fields_size)
+        if (buffer.size() < sizeof(central_directory_record_signature) + fields_size)
             return false;
         if (memcmp(buffer.data(), central_directory_record_signature, sizeof(central_directory_record_signature)) != 0)
             return false;
         memcpy(reinterpret_cast<void*>(&made_by_version), buffer.data() + sizeof(central_directory_record_signature), fields_size);
+        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size + comment_length + name_length + extra_data_length)
+            return false;
         name = buffer.data() + sizeof(central_directory_record_signature) + fields_size;
         extra_data = name + name_length;
         comment = extra_data + extra_data_length;
         return true;
     }
@@ -163,15 +167,17 @@ struct [[gnu::packed]] LocalFileHeader {
     const u8* compressed_data;
 
     bool read(ReadonlyBytes buffer)
     {
         auto fields_size = sizeof(LocalFileHeader) - (sizeof(u8*) * 3);
-        if (buffer.size() < fields_size)
+        if (buffer.size() < sizeof(local_file_header_signature) + fields_size)
             return false;
         if (memcmp(buffer.data(), local_file_header_signature, sizeof(local_file_header_signature)) != 0)
             return false;
         memcpy(reinterpret_cast<void*>(&minimum_version), buffer.data() + sizeof(local_file_header_signature), fields_size);
+        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size + name_length + extra_data_length + compressed_size)
+            return false;
         name = buffer.data() + sizeof(local_file_header_signature) + fields_size;
         extra_data = name + name_length;
         compressed_data = extra_data + extra_data_length;
         return true;
     }

Vulnerable code:         if (buffer.size() < fields_size)
        if (buffer.size() < fields_size)
        if (buffer.size() < fields_size)
Fixed code:         if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size)
        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size + comment_length)
            return false;
        if (buffer.size() < sizeof(central_directory_record_signature) + fields_size)
        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size + comment_length + name_length + extra_data_length)
            return false;
        if (buffer.size() < sizeof(local_file_header_signature) + fields_size)
        if (buffer.size() < sizeof(end_of_central_directory_signature) + fields_size + name_length + extra_data_length + compressed_size)
            return false;