apkverify: add tests for invalid zip format
Bug: n/a
Test: atest --test-mapping .
Change-Id: I286d497dee2ae65b2e0c5f6e48f17122543dc7a3
diff --git a/apkverify/Android.bp b/apkverify/Android.bp
index d2dbf41..df1cac6 100644
--- a/apkverify/Android.bp
+++ b/apkverify/Android.bp
@@ -4,7 +4,6 @@
rust_defaults {
name: "libapkverify.defaults",
- host_supported: true,
crate_name: "apkverify",
srcs: ["src/lib.rs"],
prefer_rlib: true,
@@ -33,12 +32,14 @@
rust_test {
name: "libapkverify.integration_test",
- host_supported: true,
crate_name: "apkverify_test",
srcs: ["tests/*_test.rs"],
prefer_rlib: true,
edition: "2018",
test_suites: ["general-tests"],
- rustlibs: ["libapkverify"],
+ rustlibs: [
+ "libapkverify",
+ "libzip",
+ ],
data: ["tests/data/*"],
}
diff --git a/apkverify/src/lib.rs b/apkverify/src/lib.rs
index 9930099..869431e 100644
--- a/apkverify/src/lib.rs
+++ b/apkverify/src/lib.rs
@@ -18,6 +18,7 @@
mod bytes_ext;
mod sigutil;
+mod testing;
mod v3;
mod ziputil;
diff --git a/apkverify/src/testing.rs b/apkverify/src/testing.rs
new file mode 100644
index 0000000..777afb8
--- /dev/null
+++ b/apkverify/src/testing.rs
@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+//! A collection of utilities for testing
+
+/// Asserts if `haystack.contains(needed)`
+#[macro_export]
+macro_rules! assert_contains {
+ ($haystack:expr,$needle:expr $(,)?) => {
+ match (&$haystack, &$needle) {
+ (haystack_value, needle_value) => {
+ assert!(
+ haystack_value.contains(needle_value),
+ "{} is not found in {}",
+ needle_value,
+ haystack_value
+ );
+ }
+ }
+ };
+}
diff --git a/apkverify/src/ziputil.rs b/apkverify/src/ziputil.rs
index dbf5131..bfb1c01 100644
--- a/apkverify/src/ziputil.rs
+++ b/apkverify/src/ziputil.rs
@@ -22,8 +22,10 @@
use zip::ZipArchive;
const EOCD_MIN_SIZE: usize = 22;
+const EOCD_CENTRAL_DIRECTORY_SIZE_FIELD_OFFSET: usize = 12;
const EOCD_CENTRAL_DIRECTORY_OFFSET_FIELD_OFFSET: usize = 16;
const EOCD_MAGIC: u32 = 0x06054b50;
+const ZIP64_MARK: u32 = 0xffffffff;
#[derive(Debug, PartialEq)]
pub struct ZipSections {
@@ -44,30 +46,39 @@
// retrieve reader back
reader = archive.into_inner();
// the current position should point EOCD offset
- let eocd_offset = reader.seek(SeekFrom::Current(0))?;
+ let eocd_offset = reader.seek(SeekFrom::Current(0))? as u32;
let mut eocd = vec![0u8; eocd_size as usize];
reader.read_exact(&mut eocd)?;
if (&eocd[0..]).get_u32_le() != EOCD_MAGIC {
bail!("Invalid ZIP: ZipArchive::new() should point EOCD after reading.");
}
- let central_directory_offset = get_central_directory_offset(&eocd)?;
- let central_directory_size = eocd_offset as u32 - central_directory_offset;
+ let (central_directory_size, central_directory_offset) = get_central_directory(&eocd)?;
+ if central_directory_offset == ZIP64_MARK || central_directory_size == ZIP64_MARK {
+ bail!("Unsupported ZIP: ZIP64 is not supported.");
+ }
+ if central_directory_offset + central_directory_size != eocd_offset {
+ bail!("Invalid ZIP: EOCD should follow CD with no extra data or overlap.");
+ }
+
Ok((
reader,
ZipSections {
central_directory_offset,
central_directory_size,
- eocd_offset: eocd_offset as u32,
+ eocd_offset,
eocd_size: eocd_size as u32,
},
))
}
-fn get_central_directory_offset(buf: &[u8]) -> Result<u32> {
+fn get_central_directory(buf: &[u8]) -> Result<(u32, u32)> {
if buf.len() < EOCD_MIN_SIZE {
bail!("Invalid EOCD size: {}", buf.len());
}
- Ok((&buf[EOCD_CENTRAL_DIRECTORY_OFFSET_FIELD_OFFSET..]).get_u32_le())
+ let mut buf = &buf[EOCD_CENTRAL_DIRECTORY_SIZE_FIELD_OFFSET..];
+ let size = buf.get_u32_le();
+ let offset = buf.get_u32_le();
+ Ok((size, offset))
}
/// Update EOCD's central_directory_offset field.
@@ -78,3 +89,42 @@
(&mut buf[EOCD_CENTRAL_DIRECTORY_OFFSET_FIELD_OFFSET..]).put_u32_le(value);
Ok(())
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::assert_contains;
+ use std::io::{Cursor, Write};
+ use zip::{write::FileOptions, ZipWriter};
+
+ fn create_test_zip() -> Cursor<Vec<u8>> {
+ let mut writer = ZipWriter::new(Cursor::new(Vec::new()));
+ writer.start_file("testfile", FileOptions::default()).unwrap();
+ writer.write_all(b"testcontent").unwrap();
+ writer.finish().unwrap()
+ }
+
+ #[test]
+ fn test_zip_sections() {
+ let (cursor, sections) = zip_sections(create_test_zip()).unwrap();
+ assert_eq!(sections.eocd_offset, (cursor.get_ref().len() - EOCD_MIN_SIZE) as u32);
+ }
+
+ #[test]
+ fn test_reject_if_extra_data_between_cd_and_eocd() {
+ // prepare normal zip
+ let buf = create_test_zip().into_inner();
+
+ // insert garbage between CD and EOCD.
+ // by the way, to mock zip-rs, use CD as garbage. This is implementation detail of zip-rs,
+ // which reads CD at (eocd_offset - cd_size) instead of at cd_offset from EOCD.
+ let (pre_eocd, eocd) = buf.split_at(buf.len() - EOCD_MIN_SIZE);
+ let (_, cd_offset) = get_central_directory(eocd).unwrap();
+ let cd = &pre_eocd[cd_offset as usize..];
+
+ // ZipArchive::new() succeeds, but we should reject
+ let res = zip_sections(Cursor::new([pre_eocd, cd, eocd].concat()));
+ assert!(res.is_err());
+ assert_contains!(res.err().unwrap().to_string(), "Invalid ZIP: offset should be 0");
+ }
+}
diff --git a/apkverify/tests/apkverify_test.rs b/apkverify/tests/apkverify_test.rs
index cad5ef2..3366524 100644
--- a/apkverify/tests/apkverify_test.rs
+++ b/apkverify/tests/apkverify_test.rs
@@ -14,22 +14,8 @@
* limitations under the License.
*/
-use apkverify::verify;
-
-macro_rules! assert_contains {
- ($haystack:expr,$needle:expr $(,)?) => {
- match (&$haystack, &$needle) {
- (haystack_value, needle_value) => {
- assert!(
- haystack_value.contains(needle_value),
- "{} is not found in {}",
- needle_value,
- haystack_value
- );
- }
- }
- };
-}
+use apkverify::{assert_contains, verify};
+use std::matches;
#[test]
fn test_verify_v3() {
@@ -49,3 +35,14 @@
assert!(res.is_err());
assert_contains!(res.err().unwrap().to_string(), "Public key mismatch");
}
+
+#[test]
+fn test_verify_truncated_cd() {
+ use zip::result::ZipError;
+ let res = verify("tests/data/v2-only-truncated-cd.apk");
+ // TODO(jooyung): consider making a helper for err assertion
+ assert!(matches!(
+ res.err().unwrap().root_cause().downcast_ref::<ZipError>().unwrap(),
+ ZipError::InvalidArchive(_),
+ ));
+}
diff --git a/apkverify/tests/data/README.md b/apkverify/tests/data/README.md
index 953ecdb..7556921 100644
--- a/apkverify/tests/data/README.md
+++ b/apkverify/tests/data/README.md
@@ -11,6 +11,4 @@
Number of signers: 1
```
-Some test APKs are copied from tools/apksig/src/test/resources/com/android/apksig/.
-- v3-only-cert-and-public-key-mismatch.apk
-- v3-only-with-rsa-pkcs1-sha512-8192-digest-mismatch.apk
+APK files are copied from tools/apksig/src/test/resources/com/android/apksig/.
diff --git a/apkverify/tests/data/v2-only-truncated-cd.apk b/apkverify/tests/data/v2-only-truncated-cd.apk
new file mode 100644
index 0000000..d2e3e8d
--- /dev/null
+++ b/apkverify/tests/data/v2-only-truncated-cd.apk
Binary files differ