[WIP]Add dsa encryption and decryption algorithm

[heisen-li]

The rough outline of the algorithm is now completed. There is also a lack of test files and detailed modifications. Rust is too difficult. If there is something wrong, please point it out.

[tarcieri]

Hi, thanks for opening this PR!

As a few general points, it doesn't look like you added the dsa crate to the members section of the toplevel Cargo.toml. If you do that, some of the basic lints will run as part of the CI process.

When they do, it looks like there are some non-idiomatic things you're doing like CamelCase function names. You can either have CI check for them or install clippy (rustup component add clippy) and then run cargo clippy locally to check for them.

Also please set up a GitHub Actions workflow: you can do this by copying e.g. .github/workflows/ed25519.yml to .github/workflows/dsa.yml and find/replacing ed25519 with dsa.

[heisen-li]

Thank you very much for your suggestions and responses, I have made changes according to your suggestions. But there is one conflict that has not been resolved. Your response is very detailed, I am very grateful, but I have been too busy recently, I will slowly complete the submission of this algorithm. Please give me time.

[rvolgers]

Hi, I was working in rPGP and to get some compatibility tests working I decided to do a simple DSA implementation. It has become a little too large and I thought this functionality should probably live in its own crate, and then I found this PR. Even though we have worked on the same thing I think there is actually not that much duplicated.

In particular my code implements signing with full RFC6979 (deterministic signatures) support, and uses the test vectors from that RFC (which also test the basic DSA functionality, of course). It uses the hmac_drbg crate, which I found due to a comment in the ecdsa crate that they want to switch to that crate soon.

Of course the code style is different and it is missing some tricks like the fermat inverse, and it does not have key generation at all. It is written to be simple to understand.

The code is here: https://github.com/rpgp/rpgp/pull/124/files

If I can help you with anything, please let me know. I am not in a huge hurry and don't mind if you continue when you have time.

[heisen-li]

Hi, I was working in rPGP and to get some compatibility tests working I decided to do a simple DSA implementation. It has become a little too large and I thought this functionality should probably live in its own crate, and then I found this PR. Even though we have worked on the same thing I think there is actually not that much duplicated.

In particular my code implements signing with full RFC6979 (deterministic signatures) support, and uses the test vectors from that RFC (which also test the basic DSA functionality, of course). It uses the hmac_drbg crate, which I found due to a comment in the ecdsa crate that they want to switch to that crate soon.

Of course the code style is different and it is missing some tricks like the fermat inverse, and it does not have key generation at all. It is written to be simple to understand.

The code is here: https://github.com/rpgp/rpgp/pull/124/files

If I can help you with anything, please let me know. I am not in a huge hurry and don't mind if you continue when you have time.

Sorry, just saw your message. My work is too busy, I have not had time to finish this, I almost forgot. I am very sorry about this PR, I may need to work on other things recently. If you have any ideas to discuss, I will try to spare the time to complete it.

[rvolgers]

Hi, no problem, and thanks for the quick response!

If you expect to be busy for a long time I can probably continue work on this PR, I do not mind doing it. But as I said I do not mind waiting either, if you want to finish it yourself later.

[heisen-li]

Hi, no problem, and thanks for the quick response!

If you expect to be busy for a long time I can probably continue work on this PR, I do not mind doing it. But as I said I do not mind waiting either, if you want to finish it yourself later.

Welcome to participate in the encryption and decryption algorithm, you can complete this algorithm.

[tarcieri]

@rvolgers if you have a mostly complete DSA implementation, it'd be great if you can upstream it, so long as you're okay with licensing the code under MIT+Apache2.0 which is SOP for all of the crates in this organization.

Regarding RFC6979, as you noted the ecdsa crate already has an implementation too. Perhaps we could look into extracting a common implementation, possibly as a separate crate.

As for the hmac-drbg crate, we ended up not using it as it was harming upgrade agility. It's still using hmac v0.9 when the latest version is v0.10 (and we're about to release v0.11)

[rvolgers]

I'm actually kind of happy to hear an external crate won't be used. This is just my professional paranoia, but due to the role of HMAC DRBG it is ideally suited for inserting subtle bugs which instantly leak the DSA private key during normal use of (EC)DSA and having precisely that crate live elsewhere makes me a little itchy.

The HMAC DRBG code currently in ecdsa causes the test suite to fail if I use it though (the external crate works). It assumes that a single hash output block is sufficient to produce the requested output size, and this does not hold for the dsa 2048 + sha1 and dsa 2048 + sha2-224 testcases (i.e. big DSA key, small hash). So, this needs to be fixed, besides splitting the code to a separate crate.

I'm definitely planning on upstreaming it, but I may work on the code a little more before transplanting it because I currently have it all hooked up nicely.

Is there any "idiomatic" code I can reference for how error handling should look? And are there any standards at all with regard to trying to avoid allocations and secret-dependent timing differences in BigInt expressions, or is it fine if it just works? I expect those to be the main complications, once I have key generation working.

[tarcieri]

having precisely that crate live elsewhere makes me a little itchy.

Yeah, I have similar feelings, in addition to it being a blocker on upgrades

The HMAC DRBG code currently in ecdsa causes the test suite to fail if I use it though (the external crate works). It assumes that a single hash output block is sufficient to produce the requested output size, and this does not hold for the dsa 2048 + sha1 and dsa 2048 + sha2-224 testcases

Aah, interesting. It is somewhat specialized to the ECDSA use case at the moment.

Is there any "idiomatic" code I can reference for how error handling should look?

For cryptographic errors, we generally use unit structs as error types. The intent is to avoid things like sidechannel attacks based on an attacker's ability to differentiate various error cases.

And are there any standards at all with regard to trying to avoid allocations and secret-dependent timing differences in BigInt expressions, or is it fine if it just works?

We don't have any crates at the moment which use a more general-purpose bigint library and support no_std. The rsa crate uses num-bigint-dig, which is @dignifiedquire's fork of num-bigint: https://github.com/dignifiedquire/num-bigint

It would definitely be nice to align the bigint implementation used by rsa and dsa, but for the moment we'd be happy to have something that works and then cross that bridge when we get there.

I feel like a no_std-friendly bigint library will be something which is easier to implement after const generics land in Rust.

[rvolgers]

Posting here because it's the best place for now. My work remains in the rPGP PR I linked earlier.

Key generation is now fully implemented including two unit tests with official test vectors. Signing and validation were already finished and covered by tests using official test vectors.

DSA supports quite a few hash algorithms for key generation and signing. The general approach in RustCrypto seems to be static dispatch for everything, but I'm worried about the effect on code size. Any thoughts on using dynamic dispatch to parameterize by hash function for DSA key generation and hashing? Or clever hybrid solutions? I'm currently at "DynDigest exists but isn't a great API for this, but I'll probably end up using it."

Things that remain to be done:

• Decide on the public API, including N and L parameter selection
• Add basic doc comments
• Move the HMAC DBRG from ecdsa to a separate crate, and fix it to work with output sizes larger than the hash size. Ideally we'd also add unit tests and documentation (but note that it is already exercised by the DSA tests, which have official test vectors). A suggestion for a low-effort short term solution would be very welcome here, as this seems the most likely task to hold things up as I have little motivation for it. The DSA standard is not super well written but the DBRG standard is actually painful.
• Look at switching to dynamic dispatch for hash functions. Including whether to do this outside or inside HMAC DBRG. I may end up ignoring this for now. But it's annoying as it's likely to affect the public API.
• Migrate everything to a PR for RustCrypto

[newpavlov]

@rvolgers

DynDigest exists but isn't a great API for this, but I'll probably end up using it

We are currently working on a big breaking update to digest, so if you have proposals on how we can improve DynDigest, it's a really great time to incorporate them.

Also I don't think static dispatch of hash functions will bloat the final code too much. If anything it could reduce code size a bit, since compiler will be able to remove dead code more efficiently.

[rvolgers]

@rvolgers

DynDigest exists but isn't a great API for this, but I'll probably end up using it

We are currently working on a big breaking update to digest, so if you have proposals on how we can improve DynDigest, it's a really great time to incorporate them.

What would be a good place to discuss this?

[newpavlov]

You can simply open an issue in the traits repository with your thoughts and proposals.

[rvolgers]

Also I don't think static dispatch of hash functions will bloat the final code too much. If anything it could reduce code size a bit, since compiler will be able to remove dead code more efficiently.

It is my understanding that because generate_p_q and sign have a generic Digest parameter rustc will create independent instances of these functions for every hash function they are actually used with. In a program such as rPGP that would be every hash function.

[tarcieri]

DSA supports quite a few hash algorithms for key generation and signing. The general approach in RustCrypto seems to be static dispatch for everything, but I'm worried about the effect on code size. Any thoughts on using dynamic dispatch to parameterize by hash function for DSA key generation and hashing?

It would be problematic if you wanted to use the traits in the signature crate, namely signature::DigestSigner and signature::DigestVerifier.

I'm curious to what extent you think there's excessive monomorphization overhead for using generics instead of trait objects for digests. I'm a big fan of trait objects, but to me it seems like such overhead would be a drop-in-the-bucket compared to the code for the hash functions themselves. Have you measured to determine if this is actually a problem in practice?

All that said, if you do use DigestSigner and DigestVerifier, you can derive the code needed to impl Signer and Verifier which are both designed for use as trait objects, and then you can handle dynamic dispatch at that level if you desire.

It is my understanding that because generate_p_q and sign have a generic Digest parameter rustc will create independent instances of these functions for every hash function they are actually used with

Unless I'm missing something, it sounds like you should refactor these functions to take a raw digest output, rather than making them generic over the hash function.

[rvolgers]

It is my understanding that because generate_p_q and sign have a generic Digest parameter rustc will create independent instances of these functions for every hash function they are actually used with

Unless I'm missing something, it sounds like you should refactor these functions to take a raw digest output, rather than making them generic over the hash function.

This is not about hashing input data to be signed.

In key generation the hash function needs to be invoked multiple times because it is used as a key expansion / entropy extraction mechanism, to produce keys in a deterministic way from a seed.

It is similar for signing. The hash function is used with HMAC DBRG, which is seeded and used in a specific way defined by the standard to produce a deterministic value for k.

[tarcieri]

Aah, RFC6979's usage of HMAC-DRBG is a concern, if you have an excessive number of hash functions.

FWIW the ecdsa crate uses the same hash function for prehashing the message as it does for HMAC-DRBG for RFC6979 (conveniently chained together via the DigestSigner impl), but the cardinality of hash functions used by any particular elliptic curve is on the order of 2. I guess the DSA situation is a lot less streamlined than that.

Note that the hmac crate doesn't support using DynDigest, so if you really want to have HMAC-DRBG with virtual dispatch, you'd need to start there...

...but the flipside is DynDigest requires alloc due to its use of Box, whereas nearly every crate we maintain is intended to support "heapless" usage. I think alloc is fine as a mandatory requirement for DSA (at least for now), but if you wanted RFC6979 based on DynDigest, that would mean you couldn't share an implementation with ecdsa as we have users in "heapless" embedded environments where a hard alloc requirement is a no-go.

[rvolgers]

Isn't heapless pretty much a dream for now anyway because the bigint implementation uses allocation? It's not just an implementation detail either, it would have to move to fixed size bigints to support heapless and this is a major change. Unless I'm missing something.

I understand const generics makes a fixed size bigint implementation seem close, but it seems a little premature.

[tarcieri]

Yeah, as I said for DSA I think a hard alloc dependency is ok.

The ecdsa crate (and the elliptic curve implementations which use it) all have full heapless support in their currently released versions, and we'd like to keep it that way.

[newpavlov]

As a temporary solution I think you can create an object-safe wrapper trait with only necessary functionality and pass to your functions &dyn Trait. Thus it should prevent the monomorphization bloat without relying on alloc.

Isn't heapless pretty much a dream for now anyway because the bigint implementation uses allocation?

I plan to implement stack-based bigint library using generic-array. We already have some peaces done as part of RustCrypto/elliptic-curves#218.

[rvolgers]

That's very cool. Hope somebody does a similar library for modular arithmetic including Montgomery representation, I'd be happy to adjust the DSA code to be properly zero allocation and constant time -ish.

[tarcieri]

@rvolgers I believe that code implements Montgomery representation (the code I believe it's based on did, anyway)

[rvolgers]

...but the flipside is DynDigest requires alloc due to its use of Box, whereas nearly every crate we maintain is intended to support "heapless" usage.

This is actually part of the DynDigest API I object to. It should have a finalize_reset_into_slice() which takes a dynamic length slice and just copies in as much of the digest as will fit. Caller can just use output_size() right when it first gets the DynDigest and use that to plan accordingly.

[tarcieri]

@rvolgers that sounds like something good to address before the next digest release

[tarcieri]

@rvolgers another way to avoid monomorphizing HMAC-DRBG over several hash functionsis to pick a single hash function to always use for RFC6979 (e.g. SHA-256) and instantiate HMAC-DRBG with that, regardless of what hash function is used for the message digest.

That sort of approach would work with ECDSA at least, so I assume it would work with DSA unless there are specific RFC6979 test vectors you're trying to match which work otherwise.

[rvolgers]

@tarcieri RFC6979 specifies that the hash for HMAC-DRBG should be the same as used for signing. I don't see any real security downside in always using SHA256, but it does mean taking on a bit more responsibility for the overall security picture (which I'm trying to avoid). We could also use only two of the test vectors from the RFC then.

[tarcieri]

@rvolgers it suggests it but doesn't mandate it (although as it were, the ecdsa crate implements it that way)

From RFC 6979 Section 3.6:

In this document, we use the same hash function H for processing
the input message and as a parameter to HMAC. Two distinct hash
functions could be used, provided that both are adequately secure.

[rvolgers]

@tarcieri ah, interesting. I'll give it some thought, but I still don't expect to take that direction though.

[tarcieri]

FYI, we now own the dsa crate: https://crates.io/crates/dsa 🎉

[tarcieri]

Going to close this PR as stale.

It'd be great to have a DSA implementation if anyone wants to pick it up again. Note that to use the rfc6979 crate defined in this repo, the implementation will need to use the crypto-bigint crate.