Skip to content

Commit 254f201

Browse files
kesyogkesyog
committed
SQUASHME: Fix a bunch of compiler errors
Using `manticore::Result` everywhere introduced a lot of compiler errors, even with the added `From` implementations added in `debug_from!`. Most of these errors are places where `Err` is directly returned. This commit fixes most of these issues using the `check!` and `fail!` macros. #### TODO * Figure out an appropriate `Result` boundary between the x509 modules and the `untrusted` crate. That crate leaks in a `core::result::Result` type. * Fix remaining few compiler errors * Double-check changes, especially wherever the `check!` was added, to make sure that logical errors weren't introduced. * Fix unit tests if necessary #### Future PR(s) * Incrementally add error messages to the `fail!` macros * Incrementally replace usage of try operator with the `fail!` macro so that error messages can be introduced.
1 parent c177da2 commit 254f201

File tree

26 files changed

+188
-335
lines changed

26 files changed

+188
-335
lines changed

‎src/cert/chain.rs

+8-15
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,7 @@ impl<'cert, const LEN: usize> SimpleChain<'cert, LEN> {
6767
ciphers: &mut impl sig::Ciphers,
6868
signer: Option<&'cert mut dyn sig::Sign>,
6969
) -> Result<Self, Error> {
70-
if raw_chain.len() > LEN {
71-
return Err(Error::ChainTooLong);
72-
}
70+
check!(raw_chain.len() <= LEN, Error::ChainTooLong);
7371

7472
let mut chain = ArrayVec::new();
7573
for (i, &raw_cert) in raw_chain.iter().enumerate() {
@@ -78,26 +76,21 @@ impl<'cert, const LEN: usize> SimpleChain<'cert, LEN> {
7876
let cert = Cert::parse(raw_cert, format, key, ciphers)?;
7977

8078
let prev = prev.unwrap_or(&cert);
81-
if prev.subject() != cert.issuer() {
82-
return Err(Error::BadChainLink);
83-
}
84-
if !prev.supports_cert_signing() {
85-
return Err(Error::BadChainLink);
86-
}
79+
check!(prev.subject() == cert.issuer(), Error::BadChainLink);
80+
check!(prev.supports_cert_signing(), Error::BadChainLink);
8781

8882
// None is also ok; it means the format (e.g. CWT) does not support
8983
// a CA bit.
90-
if prev.is_ca_cert() == Some(false) {
91-
return Err(Error::BadChainLink);
92-
}
84+
check!(prev.is_ca_cert() != Some(false), Error::BadChainLink);
9385

9486
// raw_chain.len() - i is the number of certificates that follow
9587
// `cert`; the path length constraint for `prev` is the number of
9688
// certs that follow it, except the leaf; these numbers are the
9789
// same.
98-
if !prev.is_within_path_len_constraint(raw_chain.len() - i) {
99-
return Err(Error::BadChainLink);
100-
}
90+
check!(
91+
prev.is_within_path_len_constraint(raw_chain.len() - i),
92+
Error::BadChainLink
93+
);
10194

10295
chain.push(cert);
10396
}

‎src/cert/cwt/cbor/mod.rs

+16-23
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,7 @@ impl<'i, 'r> Item<'i, 'r> {
159159

160160
24 => {
161161
let n = buf.read_byte()?;
162-
if n < 24 {
163-
return Err(Error::BadEncoding);
164-
}
162+
check!(n >= 24, Error::BadEncoding);
165163

166164
n as u32
167165
}
@@ -171,9 +169,7 @@ impl<'i, 'r> Item<'i, 'r> {
171169
let bytes: [u8; 2] = slice.try_into().unwrap();
172170

173171
let n = u16::from_be_bytes(bytes);
174-
if n < u8::MAX as u16 {
175-
return Err(Error::BadEncoding);
176-
}
172+
check!(n >= u8::MAX as u16, Error::BadEncoding);
177173

178174
n as u32
179175
}
@@ -183,17 +179,15 @@ impl<'i, 'r> Item<'i, 'r> {
183179
let bytes: [u8; 4] = slice.try_into().unwrap();
184180

185181
let n = u32::from_be_bytes(bytes);
186-
if n < u16::MAX as u32 {
187-
return Err(Error::BadEncoding);
188-
}
182+
check!(n >= u16::MAX as u32, Error::BadEncoding);
189183

190184
n
191185
}
192186

193187
// 27 is 64-bit integers, which we don't handle currently;
194188
// 28-30 are reserved;
195189
// 31 is indefinite-length encoding, which is banned.
196-
_ => return Err(Error::BadEncoding),
190+
_ => return Err(fail!(Error::BadEncoding)),
197191
};
198192

199193
match initial >> 5 {
@@ -208,7 +202,7 @@ impl<'i, 'r> Item<'i, 'r> {
208202
buf.read_bytes(argument as usize)?.as_slice_less_safe();
209203
Ok(Item::Scalar(Scalar::Utf8(
210204
core::str::from_utf8(bytes)
211-
.map_err(|_| Error::BadEncoding)?,
205+
.map_err(|_| fail!(Error::BadEncoding))?,
212206
)))
213207
}
214208
4 => Ok(Item::Array(Array { buf, len: argument })),
@@ -218,57 +212,58 @@ impl<'i, 'r> Item<'i, 'r> {
218212
prev_key: None,
219213
current_key: None,
220214
})),
221-
_ => Err(Error::BadEncoding),
215+
_ => Err(fail!(Error::BadEncoding)),
222216
}
223217
}
224218

225219
/// Folds this item into an [`Int`].
226220
pub fn into_int(self) -> Result<Int, Error> {
227221
match self {
228222
Item::Scalar(Scalar::Int(i)) => Ok(i),
229-
_ => Err(Error::BadEncoding),
223+
_ => Err(fail!(Error::BadEncoding)),
230224
}
231225
}
232226

233227
/// Folds this item into a UTF-8 string.
234228
pub fn into_utf8(self) -> Result<&'i str, Error> {
235229
match self {
236230
Item::Scalar(Scalar::Utf8(b)) => Ok(b),
237-
_ => Err(Error::BadEncoding),
231+
_ => Err(fail!(Error::BadEncoding)),
238232
}
239233
}
240234

241235
/// Folds this item into a byte string.
242236
pub fn into_bytes(self) -> Result<&'i [u8], Error> {
243237
match self {
244238
Item::Scalar(Scalar::Bytes(b)) => Ok(b),
245-
_ => Err(Error::BadEncoding),
239+
_ => Err(fail!(Error::BadEncoding)),
246240
}
247241
}
248242

249243
/// Folds this item into a byte string and recurses into it for reading
250244
/// more CBOR.
251245
pub fn read_all<R>(
252246
self,
253-
f: impl FnOnce(&mut untrusted::Reader<'i>) -> Result<R, Error>,
247+
f: impl FnOnce(&mut untrusted::Reader<'i>) -> core::result::Result<R, Error>,
254248
) -> Result<R, Error> {
255249
untrusted::Input::from(self.into_bytes()?)
256250
.read_all(Error::BadEncoding, f)
251+
.map_err(|e| fail!(e))
257252
}
258253

259254
/// Folds this item into an [`Array`].
260255
pub fn into_array(self) -> Result<Array<'i, 'r>, Error> {
261256
match self {
262257
Item::Array(a) => Ok(a),
263-
_ => Err(Error::BadEncoding),
258+
_ => Err(fail!(Error::BadEncoding)),
264259
}
265260
}
266261

267262
/// Folds this item into a [`Map`].
268263
pub fn into_map(self) -> Result<Map<'i, 'r>, Error> {
269264
match self {
270265
Item::Map(m) => Ok(m),
271-
_ => Err(Error::BadEncoding),
266+
_ => Err(fail!(Error::BadEncoding)),
272267
}
273268
}
274269

@@ -338,15 +333,13 @@ impl<'i, 'r> Map<'i, 'r> {
338333
let k = match Item::parse(&mut *self.buf) {
339334
Ok(Item::Scalar(i @ Scalar::Int { .. })) => i,
340335
Ok(Item::Scalar(s @ Scalar::Utf8(..))) => s,
341-
_ => return Err(Error::BadEncoding),
336+
_ => return Err(fail!(Error::BadEncoding)),
342337
};
343338

344339
// Map key encodings must be in lexicographic order, and duplicate keys
345340
// are not permitted.
346341
if let Some(prev) = self.prev_key {
347-
if prev >= k {
348-
return Err(Error::BadEncoding);
349-
}
342+
check!(prev < k, Error::BadEncoding);
350343
}
351344

352345
self.current_key = Some(k);
@@ -427,7 +420,7 @@ impl<'i, 'r> MapWalker<'i, 'r> {
427420
&mut self,
428421
key: impl Into<Scalar<'i>>,
429422
) -> Result<Item<'i, '_>, Error> {
430-
self.get(key)?.ok_or(Error::BadEncoding)
423+
self.get(key)?.ok_or_else(|| fail!(Error::BadEncoding))
431424
}
432425

433426
/// Runs `body` on remaining key-value pair in the map until completion or parse

‎src/cert/cwt/mod.rs

+8-16
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,7 @@ pub fn parse<'cert>(
107107
})?;
108108

109109
let key = key.unwrap_or(&subject_key);
110-
if !key.is_params_for(cose.algo) {
111-
return Err(Error::WrongAlgorithm);
112-
}
110+
check!(key.is_params_for(cose.algo), Error::WrongAlgorithm);
113111

114112
let verifier = ciphers
115113
.verifier(cose.algo, key)
@@ -129,7 +127,7 @@ pub fn parse<'cert>(
129127
],
130128
cose.signature,
131129
)
132-
.map_err(|_| Error::BadSignature)?;
130+
.map_err(|_| fail!(Error::BadSignature))?;
133131
let _ = (verifier, cose.signature, cose.protected_bytes);
134132

135133
Ok(Cert {
@@ -160,7 +158,7 @@ fn parse_cose_key<'cert>(
160158
map.must_get(label::RSA_EXPONENT)?.into_bytes()?;
161159
sig::PublicKeyParams::Rsa { modulus, exponent }
162160
}
163-
_ => return Err(Error::UnknownAlgorithm),
161+
_ => return Err(fail!(Error::UnknownAlgorithm)),
164162
};
165163
Ok((algo, params))
166164
})
@@ -169,7 +167,7 @@ fn parse_cose_key<'cert>(
169167
fn parse_algo(v: Item) -> Result<sig::Algo, Error> {
170168
match v.into_int()? {
171169
label::RSA_PKCS1_SHA256 => Ok(sig::Algo::RsaPkcs1Sha256),
172-
_ => Err(Error::UnknownAlgorithm),
170+
_ => Err(fail!(Error::UnknownAlgorithm)),
173171
}
174172
}
175173

@@ -201,25 +199,21 @@ impl<'cert> Cose<'cert> {
201199
let algo = map
202200
.get(label::COSE_ALG)?
203201
.map(|v| {
204-
if !protected {
205-
return Err(Error::BadEncoding);
206-
}
202+
check!(protected, Error::BadEncoding);
207203
parse_algo(v)
208204
})
209205
.transpose()?;
210206

211207
map.get(label::COSE_CRIT)?
212208
.map(|v| -> Result<_, _> {
213-
if !protected {
214-
return Err(Error::BadEncoding);
215-
}
209+
check!(protected, Error::BadEncoding);
216210
v.into_array()?.with(|l| match l.into_int()? {
217211
// We don't handle any other labels, so CRIT can't
218212
// contain any others.
219213
label::COSE_ALG
220214
| label::COSE_CRIT
221215
| label::COSE_KID => Ok(()),
222-
_ => Err(Error::BadEncoding),
216+
_ => Err(fail!(Error::BadEncoding)),
223217
})
224218
})
225219
.transpose()?;
@@ -249,9 +243,7 @@ impl<'cert> Cose<'cert> {
249243
})?;
250244

251245
let Headers { kid: kid2, .. } = parse_headers(buf, false)?;
252-
if kid.is_some() && kid2.is_some() {
253-
return Err(Error::BadEncoding);
254-
}
246+
check!(kid.is_none() || kid2.is_none(), Error::BadEncoding);
255247

256248
let (payload, _) =
257249
buf.read_partial(|buf| Item::parse(buf)?.into_bytes())?;

‎src/crypto/ring/csrng.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use ring::rand::SecureRandom as _;
1010
use ring::rand::SystemRandom;
1111

1212
use crate::crypto::csrng;
13+
use crate::Result;
1314

1415
#[cfg(doc)]
1516
use crate::crypto;
@@ -36,6 +37,8 @@ impl Default for Csrng {
3637

3738
impl csrng::Csrng for Csrng {
3839
fn fill(&mut self, buf: &mut [u8]) -> Result<(), csrng::Error> {
39-
self.inner.fill(buf).map_err(|_| csrng::Error::Unspecified)
40+
self.inner
41+
.fill(buf)
42+
.map_err(|_| fail!(csrng::Error::Unspecified))
4043
}
4144
}

‎src/crypto/ring/ecdsa.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ impl sig::Verify for VerifyP256 {
6363
message.as_slice().into(),
6464
signature.into(),
6565
)
66-
.map_err(|_| sig::Error::Unspecified)
66+
.map_err(|_| fail!(sig::Error::Unspecified))
6767
}
6868
}
6969

@@ -84,7 +84,7 @@ impl SignP256 {
8484
&ring::signature::ECDSA_P256_SHA256_ASN1_SIGNING,
8585
pkcs8,
8686
)
87-
.map_err(|_| sig::Error::Unspecified)?;
87+
.map_err(|_| fail!(sig::Error::Unspecified))?;
8888
Ok(Self { keypair })
8989
}
9090

@@ -99,7 +99,7 @@ impl SignP256 {
9999
&ring::signature::ECDSA_P256_SHA256_FIXED_SIGNING,
100100
pkcs8,
101101
)
102-
.map_err(|_| sig::Error::Unspecified)?;
102+
.map_err(|_| fail!(sig::Error::Unspecified))?;
103103
Ok(Self { keypair })
104104
}
105105
}
@@ -134,7 +134,7 @@ impl sig::Sign for SignP256 {
134134
let sig = self
135135
.keypair
136136
.sign(&rng, &message)
137-
.map_err(|_| sig::Error::Unspecified)?;
137+
.map_err(|_| fail!(sig::Error::Unspecified))?;
138138
let signature = signature
139139
.get_mut(..sig.as_ref().len())
140140
.ok_or(sig::Error::Unspecified)?;

‎src/crypto/ring/hash.rs

+21-19
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ impl hash::Engine for Engine {
7575

7676
fn write_raw(&mut self, data: &[u8]) -> Result<(), hash::Error> {
7777
match &mut self.inner {
78-
Inner::Idle => return Err(hash::Error::Idle),
78+
Inner::Idle => return Err(fail!(hash::Error::Idle)),
7979
Inner::Hash(c) => c.update(data),
8080
Inner::Hmac(c, _) => c.update(data),
8181
}
@@ -84,18 +84,20 @@ impl hash::Engine for Engine {
8484

8585
fn finish_raw(&mut self, out: &mut [u8]) -> Result<(), hash::Error> {
8686
match mem::replace(&mut self.inner, Inner::Idle) {
87-
Inner::Idle => return Err(hash::Error::Idle),
87+
Inner::Idle => return Err(fail!(hash::Error::Idle)),
8888
Inner::Hash(c) => {
89-
if out.len() != c.algorithm().output_len {
90-
return Err(hash::Error::WrongSize);
91-
}
89+
check!(
90+
out.len() == c.algorithm().output_len,
91+
hash::Error::WrongSize
92+
);
9293
let digest = c.finish();
9394
out.copy_from_slice(digest.as_ref());
9495
}
9596
Inner::Hmac(c, a) => {
96-
if out.len() != a.digest_algorithm().output_len {
97-
return Err(hash::Error::WrongSize);
98-
}
97+
check!(
98+
out.len() == a.digest_algorithm().output_len,
99+
hash::Error::WrongSize
100+
);
99101
let digest = c.sign();
100102
out.copy_from_slice(digest.as_ref());
101103
}
@@ -105,26 +107,26 @@ impl hash::Engine for Engine {
105107

106108
fn compare_raw(&mut self, expected: &[u8]) -> Result<(), hash::Error> {
107109
match mem::replace(&mut self.inner, Inner::Idle) {
108-
Inner::Idle => return Err(hash::Error::Idle),
110+
Inner::Idle => return Err(fail!(hash::Error::Idle)),
109111
Inner::Hash(c) => {
110-
if expected.len() != c.algorithm().output_len {
111-
return Err(hash::Error::WrongSize);
112-
}
112+
check!(
113+
expected.len() == c.algorithm().output_len,
114+
hash::Error::WrongSize
115+
);
113116
let digest = c.finish();
114-
if digest.as_ref() != expected {
115-
return Err(hash::Error::Unspecified);
116-
}
117+
check!(digest.as_ref() == expected, hash::Error::Unspecified);
117118
}
118119
Inner::Hmac(c, a) => {
119-
if expected.len() != a.digest_algorithm().output_len {
120-
return Err(hash::Error::WrongSize);
121-
}
120+
check!(
121+
expected.len() == a.digest_algorithm().output_len,
122+
hash::Error::WrongSize
123+
);
122124
let digest = c.sign();
123125
ring::constant_time::verify_slices_are_equal(
124126
digest.as_ref(),
125127
expected,
126128
)
127-
.map_err(|_| hash::Error::Unspecified)?;
129+
.map_err(|_| fail!(hash::Error::Unspecified))?;
128130
}
129131
}
130132
Ok(())

0 commit comments

Comments
 (0)