Merge "zip_archive: validate data descriptor contents." am: b001cc5b1c (08ffb1b9) · Commits · e / os / android_system_core

libziparchive/zip_archive.cc

+44 −16

Original line number	Original line	Diff line number	Diff line
	@@ -48,6 +48,10 @@

	using android::base::get_unaligned;		using android::base::get_unaligned;

			// Used to turn on crc checks - verify that the content CRC matches the values
			// specified in the local file header and the central directory.
			static const bool kCrcChecksEnabled = false;

	// This is for windows. If we don't open a file in binary mode, weird		// This is for windows. If we don't open a file in binary mode, weird
	// things will happen.		// things will happen.
	#ifndef O_BINARY		#ifndef O_BINARY
	@@ -496,8 +500,7 @@ void CloseArchive(ZipArchiveHandle handle) {
	delete archive;		delete archive;
	}		}

	static int32_t UpdateEntryFromDataDescriptor(MappedZipFile& mapped_zip,		static int32_t ValidateDataDescriptor(MappedZipFile& mapped_zip, ZipEntry* entry) {
	ZipEntry *entry) {
	uint8_t ddBuf[sizeof(DataDescriptor) + sizeof(DataDescriptor::kOptSignature)];		uint8_t ddBuf[sizeof(DataDescriptor) + sizeof(DataDescriptor::kOptSignature)];
	if (!mapped_zip.ReadData(ddBuf, sizeof(ddBuf))) {		if (!mapped_zip.ReadData(ddBuf, sizeof(ddBuf))) {
	return kIoError;		return kIoError;
	@@ -507,9 +510,17 @@ static int32_t UpdateEntryFromDataDescriptor(MappedZipFile& mapped_zip,
	const uint16_t offset = (ddSignature == DataDescriptor::kOptSignature) ? 4 : 0;		const uint16_t offset = (ddSignature == DataDescriptor::kOptSignature) ? 4 : 0;
	const DataDescriptor* descriptor = reinterpret_cast<const DataDescriptor*>(ddBuf + offset);		const DataDescriptor* descriptor = reinterpret_cast<const DataDescriptor*>(ddBuf + offset);

	entry->crc32 = descriptor->crc32;		// Validate that the values in the data descriptor match those in the central
	entry->compressed_length = descriptor->compressed_size;		// directory.
	entry->uncompressed_length = descriptor->uncompressed_size;		if (entry->compressed_length != descriptor->compressed_size \|\|
			entry->uncompressed_length != descriptor->uncompressed_size \|\|
			entry->crc32 != descriptor->crc32) {
			ALOGW("Zip: size/crc32 mismatch. expected {%" PRIu32 ", %" PRIu32 ", %" PRIx32
			"}, was {%" PRIu32 ", %" PRIu32 ", %" PRIx32 "}",
			entry->compressed_length, entry->uncompressed_length, entry->crc32,
			descriptor->compressed_size, descriptor->uncompressed_size, descriptor->crc32);
			return kInconsistentInformation;
			}

	return 0;		return 0;
	}		}
	@@ -577,12 +588,22 @@ static int32_t FindEntry(const ZipArchive* archive, const int ent,
	// Paranoia: Match the values specified in the local file header		// Paranoia: Match the values specified in the local file header
	// to those specified in the central directory.		// to those specified in the central directory.

	// Verify that the central directory and local file header have the same general purpose bit		// Warn if central directory and local file header don't agree on the use
	// flags set.		// of a trailing Data Descriptor. The reference implementation is inconsistent
	if (lfh->gpb_flags != cdr->gpb_flags) {		// and appears to use the LFH value during extraction (unzip) but the CD value
	ALOGW("Zip: gpb flag mismatch. expected {%04" PRIx16 "}, was {%04" PRIx16 "}",		// while displayng information about archives (zipinfo). The spec remains
			// silent on this inconsistency as well.
			//
			// For now, always use the version from the LFH but make sure that the values
			// specified in the central directory match those in the data descriptor.
			//
			// NOTE: It's also worth noting that unzip does warn about inconsistencies in
			// bit 11 (EFS: The language encoding flag, marking that filename and comment are
			// encoded using UTF-8). This implementation does not check for the presence of
			// that flag and always enforces that entry names are valid UTF-8.
			if ((lfh->gpb_flags & kGPBDDFlagMask) != (cdr->gpb_flags & kGPBDDFlagMask)) {
			ALOGW("Zip: gpb flag mismatch at bit 3. expected {%04" PRIx16 "}, was {%04" PRIx16 "}",
	cdr->gpb_flags, lfh->gpb_flags);		cdr->gpb_flags, lfh->gpb_flags);
	return kInconsistentInformation;
	}		}

	// If there is no trailing data descriptor, verify that the central directory and local file		// If there is no trailing data descriptor, verify that the central directory and local file
	@@ -945,6 +966,7 @@ static int32_t InflateEntryToWriter(MappedZipFile& mapped_zip, const ZipEntry* e

	const uint32_t uncompressed_length = entry->uncompressed_length;		const uint32_t uncompressed_length = entry->uncompressed_length;

			uint64_t crc = 0;
	uint32_t compressed_length = entry->compressed_length;		uint32_t compressed_length = entry->compressed_length;
	do {		do {
	/* read as much as we can */		/* read as much as we can */
	@@ -977,6 +999,8 @@ static int32_t InflateEntryToWriter(MappedZipFile& mapped_zip, const ZipEntry* e
	if (!writer->Append(&write_buf[0], write_size)) {		if (!writer->Append(&write_buf[0], write_size)) {
	// The file might have declared a bogus length.		// The file might have declared a bogus length.
	return kInconsistentInformation;		return kInconsistentInformation;
			} else {
			crc = crc32(crc, &write_buf[0], write_size);
	}		}

	zstream.next_out = &write_buf[0];		zstream.next_out = &write_buf[0];
	@@ -986,8 +1010,13 @@ static int32_t InflateEntryToWriter(MappedZipFile& mapped_zip, const ZipEntry* e

	assert(zerr == Z_STREAM_END); /* other errors should've been caught */		assert(zerr == Z_STREAM_END); /* other errors should've been caught */

	// stream.adler holds the crc32 value for such streams.		// NOTE: zstream.adler is always set to 0, because we're using the -MAX_WBITS
	*crc_out = zstream.adler;		// "feature" of zlib to tell it there won't be a zlib file header. zlib
			// doesn't bother calculating the checksum in that scenario. We just do
			// it ourselves above because there are no additional gains to be made by
			// having zlib calculate it for us, since they do it by calling crc32 in
			// the same manner that we have above.
			*crc_out = crc;

	if (zstream.total_out != uncompressed_length \|\| compressed_length != 0) {		if (zstream.total_out != uncompressed_length \|\| compressed_length != 0) {
	ALOGW("Zip: size mismatch on inflated file (%lu vs %" PRIu32 ")",		ALOGW("Zip: size mismatch on inflated file (%lu vs %" PRIu32 ")",
	@@ -1050,15 +1079,14 @@ int32_t ExtractToWriter(ZipArchiveHandle handle,
	}		}

	if (!return_value && entry->has_data_descriptor) {		if (!return_value && entry->has_data_descriptor) {
	return_value = UpdateEntryFromDataDescriptor(archive->mapped_zip, entry);		return_value = ValidateDataDescriptor(archive->mapped_zip, entry);
	if (return_value) {		if (return_value) {
	return return_value;		return return_value;
	}		}
	}		}

	// TODO: Fix this check by passing the right flags to inflate2 so that		// Validate that the CRC matches the calculated value.
	// it calculates the CRC for us.		if (kCrcChecksEnabled && (entry->crc32 != static_cast<uint32_t>(crc))) {
	if (entry->crc32 != crc && false) {
	ALOGW("Zip: crc mismatch: expected %" PRIu32 ", was %" PRIu64, entry->crc32, crc);		ALOGW("Zip: crc mismatch: expected %" PRIu32 ", was %" PRIu64, entry->crc32, crc);
	return kInconsistentInformation;		return kInconsistentInformation;
	}		}

libziparchive/zip_archive_test.cc

+90 −0

Original line number	Original line	Diff line number	Diff line
	@@ -642,6 +642,96 @@ TEST(ziparchive, StreamUncompressedBadCrc) {
	CloseArchive(handle);		CloseArchive(handle);
	}		}

			// Generated using the following Java program:
			// public static void main(String[] foo) throws Exception {
			// FileOutputStream fos = new
			// FileOutputStream("/tmp/data_descriptor.zip");
			// ZipOutputStream zos = new ZipOutputStream(fos);
			// ZipEntry ze = new ZipEntry("name");
			// ze.setMethod(ZipEntry.DEFLATED);
			// zos.putNextEntry(ze);
			// zos.write("abdcdefghijk".getBytes());
			// zos.closeEntry();
			// zos.close();
			// }
			//
			// cat /tmp/data_descriptor.zip \| xxd -i
			//
			static const std::vector<uint8_t> kDataDescriptorZipFile{
			0x50, 0x4b, 0x03, 0x04, 0x14, 0x00, 0x08, 0x08, 0x08, 0x00, 0x30, 0x59, 0xce, 0x4a, 0x00, 0x00,
			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x6e, 0x61,
			0x6d, 0x65, 0x4b, 0x4c, 0x4a, 0x49, 0x4e, 0x49, 0x4d, 0x4b, 0xcf, 0xc8, 0xcc, 0xca, 0x06, 0x00,
			//[sig---------------], [crc32---------------], [csize---------------], [size----------------]
			0x50, 0x4b, 0x07, 0x08, 0x3d, 0x4e, 0x0e, 0xf9, 0x0e, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x00, 0x00,
			0x50, 0x4b, 0x01, 0x02, 0x14, 0x00, 0x14, 0x00, 0x08, 0x08, 0x08, 0x00, 0x30, 0x59, 0xce, 0x4a,
			0x3d, 0x4e, 0x0e, 0xf9, 0x0e, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00,
			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x6e, 0x61,
			0x6d, 0x65, 0x50, 0x4b, 0x05, 0x06, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x32, 0x00,
			0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00};

			// The offsets of the data descriptor in this file, so we can mess with
			// them later in the test.
			static constexpr uint32_t kDataDescriptorOffset = 48;
			static constexpr uint32_t kCSizeOffset = kDataDescriptorOffset + 8;
			static constexpr uint32_t kSizeOffset = kCSizeOffset + 4;

			static void ExtractEntryToMemory(const std::vector<uint8_t>& zip_data,
			std::vector<uint8_t>* entry_out, int32_t* error_code_out) {
			TemporaryFile tmp_file;
			ASSERT_NE(-1, tmp_file.fd);
			ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, &zip_data[0], zip_data.size()));
			ZipArchiveHandle handle;
			ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "ExtractEntryToMemory", &handle));

			// This function expects a variant of kDataDescriptorZipFile, for look for
			// an entry whose name is "name" and whose size is 12 (contents =
			// "abdcdefghijk").
			ZipEntry entry;
			ZipString empty_name;
			SetZipString(&empty_name, "name");

			ASSERT_EQ(0, FindEntry(handle, empty_name, &entry));
			ASSERT_EQ(static_cast<uint32_t>(12), entry.uncompressed_length);

			entry_out->resize(12);
			(error_code_out) = ExtractToMemory(handle, &entry, &((entry_out)[0]), 12);

			CloseArchive(handle);
			}

			TEST(ziparchive, ValidDataDescriptors) {
			std::vector<uint8_t> entry;
			int32_t error_code = 0;
			ExtractEntryToMemory(kDataDescriptorZipFile, &entry, &error_code);

			ASSERT_EQ(0, error_code);
			ASSERT_EQ(12u, entry.size());
			ASSERT_EQ('a', entry[0]);
			ASSERT_EQ('k', entry[11]);
			}

			TEST(ziparchive, InvalidDataDescriptors) {
			std::vector<uint8_t> invalid_csize = kDataDescriptorZipFile;
			invalid_csize[kCSizeOffset] = 0xfe;

			std::vector<uint8_t> entry;
			int32_t error_code = 0;
			ExtractEntryToMemory(invalid_csize, &entry, &error_code);

			ASSERT_GT(0, error_code);
			ASSERT_STREQ("Inconsistent information", ErrorCodeString(error_code));

			std::vector<uint8_t> invalid_size = kDataDescriptorZipFile;
			invalid_csize[kSizeOffset] = 0xfe;

			error_code = 0;
			entry.clear();
			ExtractEntryToMemory(invalid_csize, &entry, &error_code);

			ASSERT_GT(0, error_code);
			ASSERT_STREQ("Inconsistent information", ErrorCodeString(error_code));
			}

	int main(int argc, char** argv) {		int main(int argc, char** argv) {
	::testing::InitGoogleTest(&argc, argv);		::testing::InitGoogleTest(&argc, argv);