Refactor Error and Result usage for pkcs5 crate #26

dwhjames · 2021-09-15T00:55:20Z

introduce Error enum with one and only existing error value
introduce specialized Result type for crate
rewrite references to der crate Error & Result types

dwhjames · 2021-09-15T00:58:12Z

@tarcieri, admittedly I’m just trying to do easy/accessible things, with learning being the primary motivation. I hope this is actually useful work, and not just busy work. It it did seem that the one error value was actually rolling up a variety of distinct error categories, so I thought I’d rewrite the one and only error, before moving on to separating out the cases.

tarcieri · 2021-09-15T01:08:27Z

It'd make sense if Error had more than one variant. Perhaps you can eliminate some of the map_errs that are discarding error values with From conversions?

Note that the only variants I think should be added are ones which distinguish encoding-related errors from actual cryptographic errors. I don't think you should try to categorize the various kinds of cryptographic errors or propagate information about it. That has the potential for leakage.

dwhjames · 2021-09-15T02:36:00Z

Here were my ideas:

‘encrypt/decrypt not supported for PBES1’

formats/pkcs5/src/lib.rs

Lines 89 to 90 in a391e5b

Self::Pbes2(params) => params.decrypt_in_place(password, buffer),

_ => Err(Error::Crypto),
and three other locations

‘invalid scrypt params’

formats/pkcs5/src/pbes2/kdf.rs

Lines 364 to 365 in a391e5b

    
           block_size: params.r().try_into().map_err(|_| Error::Crypto)?, 
        
           parallelization: params.p().try_into().map_err(|_| Error::Crypto)?,

formats/pkcs5/src/pbes2/kdf.rs

Lines 429 to 438 in a391e5b

    
           if 1 << log_n != n { 
        
               return Err(Error::Crypto); 
        
           } 
        
           scrypt::Params::new( 
        
               log_n, 
        
               params.block_size.into(), 
        
               params.parallelization.into(), 
        
           ) 
        
           .map_err(|_| Error::Crypto)

‘invalid block cipher params’

formats/pkcs5/src/pbes2/encryption.rs

Lines 40 to 41 in a391e5b

let cipher = Aes128Cbc::new_from_slices(encryption_key.as_slice(), iv)

.map_err(|_| Error::Crypto)?;
and several other locations
one could argue that these could be panic’s at this point and that there could be validation at an earlier point

‘no buffer remaining for block cipher padding during encryption’

formats/pkcs5/src/pbes2/encryption.rs

Line 42 in a391e5b

cipher.encrypt(buffer, pos).map_err(|_| Error::Crypto)
and several other locations

‘decryption error - invalid ciphertext size or malformed padding’

formats/pkcs5/src/pbes2/encryption.rs

Line 97 in a391e5b

cipher.decrypt(buffer).map_err(|_| Error::Crypto)
and several other locations

‘encryption not supported for cipher’

formats/pkcs5/src/pbes2/encryption.rs

Line 61 in a391e5b

EncryptionScheme::DesCbc { .. } => Err(Error::Crypto),

‘key derivation with Sha1 not enabled’

formats/pkcs5/src/pbes2/encryption.rs

Line 130 in a391e5b

Pbkdf2Prf::HmacWithSha1 => return Err(Error::Crypto),

‘scrypt derivation failure’

formats/pkcs5/src/pbes2/encryption.rs

Lines 187 to 194 in a391e5b

    
           let mut buffer = [0u8; MAX_KEY_LEN]; 
        
           scrypt( 
        
               password, 
        
               params.salt, 
        
               &params.try_into()?, 
        
               &mut buffer[..length], 
        
           ) 
        
           .map_err(|_| Error::Crypto)?;

this also seems like a library bug thus panic, rather than a plausible runtime error — entirely prevented as long as the constants in the crate are set appropriately

jklong · 2021-09-15T12:03:49Z

1. ‘decryption error - invalid ciphertext size or malformed padding’


* https://github.com/RustCrypto/formats/blob/a391e5b0995ae7aa9224972749d4186625b6ec5c/pkcs5/src/pbes2/encryption.rs#L97

* and several other locations

I'd be careful with this one in particular. Once you start identifying information around padding you're introducing the possibility of a padding oracle. That's a large potential foot-gun if the library user isn't careful about that information leaking.

tarcieri · 2021-09-15T12:04:03Z

Going through these one-by-one:

‘encrypt/decrypt not supported for PBES1’

I think it's fine to have a variant that says PBES1 is unsupported. This will effectively be a perpetual WONTFIX.

‘invalid scrypt params’

‘invalid block cipher params’

I would really like to avoid overspecializing variants for specific algorithms, especially if they are variants that are only available via conditional compilation.

How about something like: AlgorithmParametersInvalid { oid: ObjectIdentifier }?

If you'd really like to break the cases above down, perhaps it could be KdfParametersInvalid { oid } versus CipherParametersInvalid { oid }.

‘no buffer remaining for block cipher padding during encryption’

An error case for the output buffer being too small sounds fine. I would avoid mentioning anything about padding: the real problem is that the buffer is too small.

‘decryption error - invalid ciphertext size or malformed padding’

This is the kind of error I'd personally prefer to continue to keep an opaque Crypto error. If you really want to split out errors specifically related to decryption, something like Error::DecryptFailed would be OK.

‘encryption not supported for cipher’

‘key derivation with Sha1 not enabled’

These are effectively unsupported algorithm errors. As I mentioned earlier, I'd prefer not to get too off in the weeds with algorithm-specific error variants.

Something like UnsupportedAlgorithm { oid: ObjectIdentifier } seems fine to me though.

‘scrypt derivation failure’
this also seems like a library bug thus panic, rather than a plausible runtime error — entirely prevented as long as the constants in the crate are set appropriately

Across the RustCrypto projects we generally try to minimize any potential runtime panics, and especially in low-level libraries like this we are aiming for panic-free code. The reason is we'd like to make these libraries usable in contexts where panics are unacceptable, like OS kernels or bare metal embedded device programming.

That's not a hard rule, or an easily enforceable one (see previous link for some discussion on that), but we would like to ensure that there is a baseline level of functionality which does not require a panic handler.

tarcieri · 2021-09-15T12:06:44Z

I'd be careful with this one in particular. Once you start identifying information around padding you're introducing the possibility of a padding oracle. That's a large potential foot-gun if the library user isn't careful about that information leaking.

Yep, exactly, avoiding this sort of leakage is exactly why we've largely kept the error types opaque.

dwhjames · 2021-09-15T16:21:23Z

@jklong @tarcieri thanks for the great feedback.

I’ll come back with something concrete.

tarcieri

Looks good! I'll hold off on merging this until we're ready to make breaking changes

tarcieri · 2021-10-06T18:38:44Z

@dwhjames can you rebase? If so, I can merge after that

- introduce Error enum with new error cases - introduce specialized Result type for crate - rewrite references to der crate Error & Result types - some collateral refactoring as well

dwhjames · 2021-10-09T18:56:29Z

@tarcieri, 👍, conflicts resolved.

tarcieri · 2021-10-09T19:00:09Z

pkcs5/src/pbes2/encryption.rs

+    let algo_params_invalid_error = Error::AlgorithmParametersInvalid {
+        oid: params.encryption.oid(),
+    };


Rather than eagerly calculating this, you could make it a function on Parameters, e.g. Parameters::invalid_error.

see d30f931

tarcieri · 2021-10-09T19:00:59Z

pkcs5/src/pbes2/encryption.rs

+    let algo_params_invalid_error = Error::AlgorithmParametersInvalid {
+        oid: params.encryption.oid(),
+    };


Ditto here re: https://github.com/RustCrypto/formats/pull/26/files#r725524106

see d30f931

tarcieri · 2021-10-09T19:07:33Z

pkcs5/src/pbes2/kdf.rs


 impl<'a> Pbkdf2Params<'a> {
    /// Initialize PBKDF2-SHA256 with the given iteration count and salt
-    pub fn hmac_with_sha256(iteration_count: u16, salt: &'a [u8]) -> Result<Self, CryptoError> {


It would probably be good to leave this fallible.

I'm not sure why u16 was used for the iteration count but it unnecessarily constrains the number of iterations. Using 100,000+ should be supported.

However, if this type is changed to a u32, it would also be good to set a sanity limit on the number of iterations.

RFC8018 says the sanity limit is implementation defined:

https://datatracker.ietf.org/doc/html/rfc8018#appendix-A.2

iterationCount specifies the iteration count. The maximum iteration count allowed depends on the implementation. It is expected that implementation profiles may further constrain the bounds.

see d30f931

sanity limit chosen as 100 million

tarcieri · 2021-10-09T19:08:12Z

pkcs5/src/pbes2/kdf.rs


 impl<'a> ScryptParams<'a> {
+    #[cfg(feature = "scrypt")]
+    const INVALID_ERR: Error = Error::AlgorithmParametersInvalid { oid: SCRYPT_OID };


This can also be replaced with a function on params.

Edit: actually, since it's private I guess this is fine. You could also use this approach on the other params structs.

see d30f931

same added for Pbkdf2Params

see RustCrypto#26

tarcieri · 2021-10-09T20:58:28Z

Thank you!

dwhjames force-pushed the pkcs5-error branch 2 times, most recently from ec05443 to a391e5b Compare September 15, 2021 01:02

dwhjames force-pushed the pkcs5-error branch from 8c950bb to 38ca864 Compare September 16, 2021 07:06

tarcieri approved these changes Sep 16, 2021

View reviewed changes

Refactor Error and Result usage for pkcs5 crate

554c8cb

- introduce Error enum with new error cases - introduce specialized Result type for crate - rewrite references to der crate Error & Result types - some collateral refactoring as well

dwhjames force-pushed the pkcs5-error branch from 38ca864 to 554c8cb Compare October 9, 2021 18:49

tarcieri reviewed Oct 9, 2021

View reviewed changes

address code review comments

d30f931

see RustCrypto#26

tarcieri merged commit 65fbe57 into RustCrypto:master Oct 9, 2021

dwhjames deleted the pkcs5-error branch October 9, 2021 21:25

tarcieri mentioned this pull request Nov 15, 2021

pkcs5 v0.4.0 #224

Merged

Refactor Error and Result usage for pkcs5 crate #26

Refactor Error and Result usage for pkcs5 crate #26

Uh oh!

Conversation

dwhjames commented Sep 15, 2021

Uh oh!

dwhjames commented Sep 15, 2021

Uh oh!

tarcieri commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwhjames commented Sep 15, 2021

Uh oh!

jklong commented Sep 15, 2021

Uh oh!

tarcieri commented Sep 15, 2021

Uh oh!

tarcieri commented Sep 15, 2021

Uh oh!

dwhjames commented Sep 15, 2021

Uh oh!

tarcieri left a comment

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Oct 6, 2021

Uh oh!

dwhjames commented Oct 9, 2021

Uh oh!

tarcieri Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

dwhjames Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

tarcieri Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

dwhjames Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

tarcieri Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

dwhjames Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

tarcieri Oct 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwhjames Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Oct 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tarcieri commented Sep 15, 2021 •

edited

Loading

tarcieri Oct 9, 2021 •

edited

Loading