Skip to content

Commit 11b6fc1

Browse files
committed
Prevent overflow errors, and add a correctness comment
1 parent 0f3d708 commit 11b6fc1

4 files changed

Lines changed: 19 additions & 3 deletions

File tree

api/src/sign/mod.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,11 @@ pub fn gather_signature_data(
7878
return Err(SignaturesError::Empty.into());
7979
}
8080

81-
let signature_bytes = HEADER_SIZE + keys.len() * SIGNATURE_LENGTH;
81+
let signature_bytes = keys
82+
.len()
83+
.checked_mul(SIGNATURE_LENGTH)
84+
.and_then(|s| s.checked_add(HEADER_SIZE))
85+
.ok_or(SignaturesError::TooManyKeys)?;
8286
if signature_bytes > BUF_LIMIT {
8387
return Err(SignaturesError::TooManyKeys.into());
8488
}

api/src/sign/zip.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,11 @@ where
4141
if keys.len() > SignatureCountLeInt::MAX as usize {
4242
return Err(Error::TooManyKeys.into());
4343
}
44-
let signature_bytes = SIGNATURE_LENGTH * keys.len() + HEADER_SIZE;
44+
let signature_bytes = keys
45+
.len()
46+
.checked_mul(SIGNATURE_LENGTH)
47+
.and_then(|s| s.checked_add(HEADER_SIZE))
48+
.ok_or(Error::TooManyKeys)?;
4549
if signature_bytes > BUF_LIMIT {
4650
return Err(Error::TooManyKeys.into());
4751
}

api/src/verify/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,9 @@ where
105105
if signature_count == 0 {
106106
return Err(SignaturesError::Empty.into());
107107
}
108-
let signature_bytes = signature_count * SIGNATURE_LENGTH;
108+
let signature_bytes = signature_count
109+
.checked_mul(SIGNATURE_LENGTH)
110+
.ok_or(SignaturesError::MagicHeader)?;
109111
if signature_bytes > BUF_LIMIT {
110112
return Err(SignaturesError::MagicHeader.into());
111113
}

api/src/verify_unsign_tar.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,12 @@ where
9898
if data[..GZIP_START.len()] != *GZIP_START {
9999
return Err(TarReadSignaturesError::Gzip);
100100
}
101+
// `base64` already ensures that no `NUL` was contained in the input. A `NUL` would mean that
102+
// the signature data was broken / contained inside another `DEFLATE` block. I don't think
103+
// forging a `.tar.gz` file this way could be used for an attack, anyway, because an empty
104+
// `DEFLATE` block cannot contain data. Being "empty" and "containing data" at the same time …
105+
// But without any `NUL` check, `zipsign-api` could possibly say "this file is fine", when it's
106+
// actually broken.
101107
let Ok(data) = BASE64_STANDARD.decode(&data[GZIP_START.len()..]) else {
102108
return Err(TarReadSignaturesError::Base64);
103109
};

0 commit comments

Comments
 (0)