orion-rs / orion Goto Github PK

All generate() functions should panic on an error that is returned by fill_bytes() or similar. Generating "non-faulty" random bytes is so important to security, that handling an error arising from this should not be left to the user.

Furthermore, using rand_os also chooses to use a blocking interface in most cases and so this will not be relevant to handle.

Due to the way clear() is called in impl Drop, the intended clear function is not being used and defaults instead back to the default one for byte vectors. _.clear() looks as what type _ is and what function it implements, therefor the intended clear() function is not being called.

In regards to /u/protestor's feedback, there is missing documentation on the info parameter for the HKDF functions. It should also be considered to add descriptions for all other parameters for all other functions.

Ways to use Pin for types that hold secret data need to be explored, to avoid copies being left around. The ideal scenario is implementing Pin with all types that hold secret data, without any changes to the public API's.

The docs for cshake are not consistent with the other modules. Function parameters, exceptions and notes should all be in the module root, where the example is listed now. All functions in cshake have a short description in the first line which should be preserved. The documentation for hmac is a good reference here.

Documentation should show how users can safely compare slices and newtypes with each other. The documentation should also include a short note on security, where constant-time comparison is required.

rand and byteorder and not specified as they should. They are not specified with compatible no_std flags, meaning even if orion is advertised as being no_std, the dependencies brought into scope rely on std.

This will be fixed by introducing a default feature, mening that if no_std context is needed, this can be enabled with default-features = false. This also means that when no_std is used, access to the default API is lost, since this relies on std through OsRng.

Why does this function return a Result instead of a simple bool?
I'm not even sure if Result is usable if you want a cost-time execution.
The check for the same length is also not necessary, subtle does this already.

https://github.com/brycx/orion/blob/a6efb6f9094c71a17a9ce07fb8e288d5bd808dd2/src/util/mod.rs#L97

In Rust 1.32 new endianness conversion functions landed which can be used with both u8, u32 and u64. However, those functions take in a fixed-size array as parameter when using from_*_bytes(), and having functions passing with slices currently needs to make use of the TryInto trait, which is not stable. Once it's stabilized we can avoid the tmp value in the below example code of impl_load. This would be able to replace the byteorder dependency crate.

Here are some issues and recommendations:

util.rs

• There was a missing return which made compare_ct return the wrong answer when y is longer than x and x and y are identical up to the length of x. PR to fix it: #1

hkdf.rs

• The name "data" for the HKDF parameter is confusing (formally, it's called the Initial Key Material, so maybe just calling it key_material or something similar would be more informative).
• HKDF doesn't validate self.hmac before using it (it relies on the HMAC call later on panicing when it's a bad value). I would use an enum or something instead of an integer so that it's not possible to even express an incorrect value here.
• The maximum-length check in HKDF doesn't need to convert to f32, since self.hmac/8 will always be an integer.
• It would be a good idea to add unit tests that test HKDF's maximum-length check.
• The implementation of HKDF looks correct to me (and the test vector tests are nice!).

hmac.rs

• It looks correct to me (and it's awesome that the unit tests compare against test vectors).
• Again, it would be nice if sha2 were an enum or something instead of a usize so that there can't be invalid values.

default.rs

• hmac_validate would be more useful (and less error-prone) if its signature was: hmac_validate(expected_hmac: &[u8], message: &[u8], key: &[u8]). Currently, it just compares two &[u8]s, taking in the key and message would be more useful since it saves the caller from having to re-compute the HMAC for comparison.

other

• The doc comments used 10-byte keys, which might mislead people into thinking 10-byte keys are safe. PR to fix it: #2
• I'm still in the process of learning rust so I may have missed rust-specific problems.

Something that i would like to see in this library is an API for encrypting a stream of messages.
This is for example useful if a file is too large to be encrypted at once.

It is impotent for such an API that messages can not be modified , dropped or reordered. This means the complete stream needs to be authenticated.
Its also a requirement, that the same message, if encrypted repeatedly, produces different ciphertexts.

Libsodium provides such a functionality based on XChaCha20Poly1305.
https://download.libsodium.org/doc/secret-key_cryptography/secretstream

This functionality can also easily implemented for orion. (I already have a working, hazardous implementation for it )

The main problem that i see is that this algorithm isn't standardized.

If you want this functionality i can submit a pull request for this

A recent paper on the usability of Rust cryptography API's has a lot of suggestions on how to improve usability. In part suggesting that low-level API's be hidden under a module layer called "hazardous materials", much like cryptography.io does. Therefor orion's primitives should be moved to a module called hazardous. To make users more aware of implications from different choices, orion should for example also document recommended salt sizes, promote the use of the constant-time comparison function that orion provides, and generating salt, keys, etc with the gen_rand_key function.

Many functions that return a Result are tagged with #[must_use].
This seems unnecessary, because Result itself is already tagged with it https://doc.rust-lang.org/std/result/#results-must-be-used

As pointed out in issue #92 raised by @snsmac, both verify() and secure_cmp currently return Result<Ok(bool), UknownCryptoError>.

Returning a bool is however redundant, as the verification or comparison only will return Ok() if it was successfull and UknownCryptoError if not. If a user needed a bool to represent the result, this could be done with .is_ok()/.is_err().

Because of this, it seems it would make sense to change secure_cmp and all verify() interfaces in orion to return Result<(), UknownCryptoError>, dropping the needless bool.

Before doing so, these new interfaces should be tested in orion-dudect for constant-time execution.

To check that all newtypes (SecretKeys, Poly1305, etc.) implement the correct traits, new testing functions are needed.

For example fn test_traits_secret_key<Secret: Debug + Drop + PartialEq>() {} could be used to check that a secret key implements the needed traits in a test like so:

fn test_traits_secret_key<Secret: Debug + Drop + PartialEq>() {}

#[test]
fn test_implemented_traits() {
    test_traits_secret_key<hmac::SecretKey>();
}

StreamCipherTestRunner and AeadTestRunner cannot randomly generate secret keys and nonces. The tests to check that using different key/nonce pairs produce different ciphertexts, have to be done outside of the test runners (as is the case with StreamCipherTestRunner right now).

Preferably we want to pass secret keys and nonces to these test runners, such that they can be generated randomly. This will require modifications to both test runners and most likely also a trait for the secret keys and nonces.

~/src/orion$ cargo +nightly test --tests --no-default-features --features no_std
   Compiling orion v0.14.4 (/home/jr/src/orion)
error[E0425]: cannot find value `BLAKE2B_GENSIZE` in this scope
   --> src/hazardous/hash/blake2b.rs:108:51
    |
108 |     (SecretKey, test_secret_key, 1, BLAKE2B_KEYSIZE, BLAKE2B_GENSIZE)
    |                                                      ^^^^^^^^^^^^^^^ help: a constant with a similar name exists: `BLAKE2B_KEYSIZE`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.
error: could not compile `orion`.
warning: build failed, waiting for other jobs to finish...
error: build failed

This seems to be caused by this commit:
8849be1

The fuzzing target ring_compare.rs should be split up into separate fuzzing targets. The new targets should still compare with ring but be split up depending on what they fuzz, like: hmac_compare.rs for HMAC, etc.

A new fuzzing target is added with cargo +nightly fuzz add new_target.

It was possible to call the update() function after finalization for both hmac and cshake structs. This would yield incorrect results without the program throwing exceptions. This has been fixed in version 0.7.0 and orion now panics when update() is called without resetting the struct first.

It could be useful to specify a minimum required rust version and add an CI test to enforce this.
Note: zeroize 1.0 requires rustc 1.36 or newer

Currently the only password hashing algorithm supported is PBKDF2. This is a dated algorithm and better alternatives exist.

The high-level API in orion::pwhash offers key derivation using PBKDF2, but should preferably use a more modern algorithm.

Argon2i would be a very good replacement, which is also provided by other crypto libraries such as libsodium. I know @vlmutolo began some work on implementing this.

To provide Argon2i and keep unsafe code at a minimum, this should for starters not support threading. Alternatively we could provide scrypt, though a PBKDF2-SHA256 implementation is missing for this. I don't see an argument for providing scrypt rather than Argon2i however.

The public API for streaming structs and newtypes could be made more familiar to users that come from other Rust crypto libraries.

In continuation of trying to make the streaming API more consistent (#87) the general approach seems to be that all one-shot functions are also part of the struct (an example of this is the RustCrypto Digest trait). Maybe the current should be changed to:

• module::one_shot_function() -> module::Ctx::one_shot_function()
• module::verify() -> module::Ctx::verify()

For the newtypes API, to be more consitent with types throughout Rust, we chould change:

• get_length -> len

Changing the newtype API seems worthwhile, but it's debatable whether changing the straming struct API will make a noticable difference in usability.

Google's Wycheproof has added test vectors for:

• XChaCha20Poly1305
• HKDF-HMAC-SHA512
• HMAC-SHA512

These have to be added. The test vectors for ChaCha20Poly1305 have also been updated. Most of the logic for parsing the test vectors should already be present in the current tests for ChaCha20Poly1305. Those could probably also do with some refactoring.

HMAC already has this, but it would be nice to offer verification/comparison for PBKDF2 and HKDF outside of the default API.

Currently BLAKE2b always returns a Digest, even if it was used with a secret-key and represents a MAC. Digest does implement PartialEq in constant-time, like Tag in HMAC and Poly1305, but doesn't provide the function unprotected_as_bytes(), but instead as_bytes().

Perhaps BLAKE2b should return a Tag when used with a secret key, making the difference more clear. Maybe even move keyed-BALKE2b into the mac module instead.

Hi, sorry in advance if this is a dumb question. I'm using the default API for encrypt/decrypt. The docs mention that these operations return a Result.

However, looking at the implementation, one can see that these functions will panic on Error, as they're calling .unwrap() on the xchacha20poly1305::{encrypt,decrypt} functions.

https://github.com/brycx/orion/blob/04a0db6d2a2fa387d4499068c0087ef941c2da6a/src/default.rs#L373-L379

https://github.com/brycx/orion/blob/04a0db6d2a2fa387d4499068c0087ef941c2da6a/src/default.rs#L424-L430

As far as I'm aware, this makes it impossible to handle errors in the caller, and will make the application panic.

Is there any way to handle the error, or should unwrap() be changed to use the ? operator?

Calling hazardous::aead::chacha20poly1305::seal() and hazardous::aead::xchacha20poly1305::seal() with an out parameter, that has a length above that of plaintext + size_of_poly1305_tag, will result in a panic. This is because when appending the Poly1305 tag, slice lengths are not explicitly defined.

Before the default API was introduced, hkdf.compute() was constructed to call hkdf.extract() itself, for simplicity. Now that the API is available, to make HKDF fully compliant with the RFC, there should be:

• Seperation between extract and expand function
• Function should be named accordingly

Relevant APIs should be tested in CI to increase confidence in actual constant-time execution. Relevant tools to use:

• dudect-bencher
• ctverif
• ctgrind

https://github.com/brycx/orion/blob/f5db759f157921c31c571868757900eedd2bcdcf/src/aead.rs#L48-L57

I do not like the word "Exception" here, because Rust doesn't have the term exception in its library, in contrast to other languages, e.g. C#, Java, C++, ...

The Rust documentation ueses the word "Panics" when it will panic under certain conditions (e.g. u32::overflowing_div).

I would suggest to adopt the phrasing.

It looks like QuickCheck would be a good option. Some initial parts where it seems useful to integrate such tests are:

Streaming interfaces (currently: cSHAKE, HMAC and Poly1305):

• Never panic when calling update() on an object that is not finalized.
• Never panic when calling reset().
• Never panic when calling finalize() (with a correct potential destination buffer) on an object that is not finalized.
• Never panic when calling init() using valid parameters.
• After calling reset() on a finalized object, updating and finalizing again, using the same parameters, should always produce the same outcome, for example Tag.
• After calling reset() on an object where update() has been called, updating again and finalizing, using the same parameters, should always produce the same outcome, for example Tag.

Other:

• A verification function should always return true when valid parameters are passed.
• Both authenticated and unauthenticated encryption should always produce the same plaintext after encryption/decryption, when valid parameters are used.
• On a macro-defined type with constant size, calling from_slice() should never panic when passing a slice of the required length.
• On a macro-defined type with variable size, calling from_slice() should never panic when passing a slice with a length above 0.
• On a macro-defined type, calling get_length() should never panic.
• On a macro-defined type, calling as_bytes() or unprotected_as_bytes() should never panic.

This list will be updated if anything else needs to be added.

The below test modules rely on ring's testing framework to read the test vectors from files in test_data:

• other_poly1305.rs
• boringssl_chacha20_poly1305.rs
• boringssl_xchacha20_poly1305.rs

These should be converted into pure-Rust testing modules, like it has been done with the NIST CAVP HMAC test vectors. Python conversion scripts used to convert such test vector files can be found in test_generation for reference. Conversion scripts for any of the above modules are also preferred to be in Python or Rust.

Hi, sir. I like to help open source projects. I have an idea for you. Do you want me to design a logo?

Currently there are three structs which are used for error handling:

• FinalizationCryptoError
• UnknownCryptoError
• ValidationCryptoError

Errors are propagated with ?, but this means for example that if a function is specified to return a ValidationCryptoError and internally uses a function that for some reason would return a UnkownCryptoError, the user would have no way of knowing. The verify function from hazardous::blake2b is an example of this, when using an invalid size parameter with otherwise valid parameters.

This means that having different structs for error handling in some cases is completely useless. I think, either there should only be one struct for error handling, namely UnknownCryptoError, or an enum should be used.

Arguments for a single struct:

• Errors represented by FinalizationCryptoError are so obvious, as the user would call a streaming state function more than once explicitly, they don't really need a separate struct to be represented.
• Functions returning ValidationCryptoError already return a bool and thus a failed verification could just be represented as false. Again, a separate error type is most likely not actually needed here.
• Ensures we aren't leaking information as a side channel.

Arguments for an enum:

• We can propagate errors as they are intended, and represent things such as failed verifications, etc.
• Here we could add a variant to represent an error from a failing OsRng, such as in aead::seal(). This would actually be useful for the user to know, instead of just receiving an opaque error.

This is a proposal for the high-level interface, providing stream-based encryption based on the hazardous implementation in #99. I find the interface provided by sodiumoxide quite nice and my proposal borrows a fair amount from there for that reason.

The streaming encryption will be a part of the orion::aead module, defining two types. One for handling stream encryption (StreamSealer) and one to handle stream decryption (StreamOpener). Nonce generation will be done automatically. Providing separate types for encryption/decryption is to make misuse harder and is also a concern @snsmac has brought up. Here is a rough implementation of what this interface may look like:

pub struct StreamSealer {
	internal_sealer: SecretStreamXChaCha20Poly1305,
	nonce: Nonce
}

impl StreamSealer {

	pub fn get_nonce(&self) -> Nonce {
		self.nonce
	}

	pub fn new(secret_key: &SecretKey) -> Result<Self, UnknownCryptoError> {
		let nonce = Nonce::generate();
		let sk = &chacha20::SecretKey::from_slice(secret_key.unprotected_as_bytes())?;
		
		Ok(Self {
			internal_sealer: SecretStreamXChaCha20Poly1305::new(&sk, &nonce),
			nonce
		})
	}

	pub fn seal_chunk(&mut self, data: &[u8], ad: Option<&[u8]>, tag: ChunkTag) -> Result<Vec<u8>, UnknownCryptoError> {
		let sealed_chunk_len = data.len().checked_add(SECRETSTREAM_XCHACHA20POLY1305_ABYTES);
		if sealed_chunk_len.is_none() {
			return Err(UnknownCryptoError);
		}
		
		let mut sealed_chunk = vec![0u8; sealed_chunk_len.unwrap()];
		self.internal_sealer.encrypt_message(data, ad, &mut sealed_chunk, tag)?;

		Ok(sealed_chunk)	
	}
}

pub struct StreamOpener {
	internal_sealer: SecretStreamXChaCha20Poly1305,
}

impl StreamOpener {

	pub fn new(secret_key: &SecretKey, nonce: &Nonce) -> Result<Self, UnknownCryptoError> {
		let sk = &chacha20::SecretKey::from_slice(secret_key.unprotected_as_bytes())?;
		
		Ok(Self {
			internal_sealer: SecretStreamXChaCha20Poly1305::new(&sk, &nonce),
		})
	}


	pub fn open_chunk(&mut self, data: &[u8], ad: Option<&[u8]>) -> Result<(Vec<u8>, ChunkTag), UnknownCryptoError> {
		if data.len() < SECRETSTREAM_XCHACHA20POLY1305_ABYTES {
			return Err(UnknownCryptoError);
		}
		
		let mut opened_chunk = vec![0u8; data.len() - SECRETSTREAM_XCHACHA20POLY1305_ABYTES];
		let tag = self.internal_sealer.decrypt_message(data, ad, &mut opened_chunk)?;

		Ok((opened_chunk, tag))
	}
}

I specifically try to move away from calling the encryption/decryption operations push or pull, as I find these to be bad names as they don't clearly communicate the purpose or functionality behind IMO. Including "seal" and "open" was to be consistent with naming of AEAD-related functions throughout the rest of orion.

Once #88 is finalised, there will be a high-level type called Tag as well as all other Tags used in hazardous. That is the reasoning behind importing the stream Tag as ChunkTag, though I don't know if there's something better to call it.

Another remaining question is whether the nonce used should be called a Header as libsodium does it, or if we should stick with Nonce. I personally find Nonce to be a more fitting name, because I find it to convey it's use more clearly.

Any feedback/input is greatly appreciated!

Currently XChaCha20 (with IETF ChaCha20) is tested directly with only one RFC test vector. Other than that, it's tested implicitly through XChaCha20Poly1305.

Like HChaCha20 is tested using test vectors generated with Monocypher, there should be generated test vectors for XChaCha20 as well. These can be generated using Monocypher, libsodium or some other well-tested cryptography library that offers XChaCha20 for that matter.

Before a stable version of orion is released, an audit should be done. Preferably of the whole library, though it may end up only being partly. This depends on the financial means available.

Edit: I currently have no idea about when I would be able to afford this.

Functions that return a Result generate a warning when these are not used:

warning: unused `std::result::Result` that must be used
   --> src/high_level_api.rs:111:5
    |
111 |     orion::aead::seal(&aead_key, "Test".as_bytes());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(unused_must_use)] on by default
    = note: this `Result` may be an `Err` variant, which should be handled

This is the standard warning for a Result, but adding an explicit security warning to these functions may help users understand the importance of not ignoring a Result. Adding this would then produce two separate warnings by the compiler:

#[must_use = "SECURITY WARNING: Ignoring a Result may have security-implications."]

warning: unused `std::result::Result` that must be used
   --> src/high_level_api.rs:111:5
    |
111 |     orion::aead::seal(&aead_key, "Test".as_bytes());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(unused_must_use)] on by default
    = note: this `Result` may be an `Err` variant, which should be handled

warning: unused return value of `orion::aead::seal` that must be used
   --> src/high_level_api.rs:111:5
    |
111 |     orion::aead::seal(&aead_key, "Test".as_bytes());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: SECURITY WARNING: Ignoring a Result may have security-implications.

The question is whether this approach is worthwhile.

Edit: If this is decided against, then we should remove the useless #[must_use] on Result as mentioned in #89.

Currently reset() checks whether or not the state is already finalized. If it is not finalized, it will not reset the state. So if someone were to call reset() not having finalized the state before, incorrect results would be produced. Such streaming states include hmac, poly1305, blake2b and cshake.

Either solve this by using another crate that provides Keccak or wait until the issue gets fixed for tiny-keccak. Issue

As @vlmutolo mentioned in this issue, the current hyperlinks in the documentation all link to docs.rs.

These should be changed to follow the Rust API guidelines and not specifically link to docs.rs if the item is part of orion. This will make development easier, since the links will automatically point to locally built docs, instead of docs.rs.

The new format will probably look as follows (example taken from orion::aead, not tested to work with docs.rs):

//! [`seal`]: fn.seal.html
//! [`open`]: fn.open.html
//! [`POLY1305_OUTSIZE`]: ../hazardous/mac/poly1305/constant.POLY1305_OUTSIZE.html
//! [`XCHACHA_NONCESIZE`]: ../hazardous/stream/xchacha20/constant.XCHACHA_NONCESIZE.html
//! [`SecretKey::default()`]: struct.SecretKey.html

The PBKDF2 maximum derived key length check will have a overflowing literal on 32bit targets.

For some unknown reason, CI fails when running all tests (tests in /src and /tests) using ASan. LeakSanitizer seems not to have any issues with this. One of the more recent fails with ASan is here. Fuzzing with ASan also seems to work as expected.

Until this has been resolved, only tests in the /tests directory are run with ASan in CI.

haybale-pitchfork uses symbolic execution with LLVM IR to verify constant-time execution.