Page MenuHome GnuPG

The digest&sign/verify API with SHAKE-class digests does not work
Closed, ResolvedPublic

Description

One of the issues that we got from our lab was regarding to their attempt to use SHAKE digests with the new digest&sign API, which was failing in unexpected ways. Given that this should not work in FIPS mode, it was not an issue, but it still leaves a question whether this should be a valid use case outside of the FIPS Mode or whether we want to improve the error reporting of this case. Currently, the SHAKE digests will fail with the following error when we try to get the read method from them, which is not present:

_gcry_fatal_error (GPG_ERR_DIGEST_ALGO,
                  "requested algo has no fixed digest length");

We have similar use in the PBKDF2 which rejects SHAKE a bit nicer:

hlen = _gcry_md_get_algo_dlen (hashalgo);
if (!hlen)
  return GPG_ERR_DIGEST_ALGO;

So the proposal here is to implement something similar in the new digest&sign API to fail more gracefully (with error code instead of fatal error).

Details

Version
master, 1.10.x

Event Timeline

Another possibility for digest&sign API: it is possible to determine the length of required hash function by the underlining field Fp of the curve in use. Then, use this length instead. It's better than to (try to) get the length by _gcry_md_get_algo_dlen (for SHAKE, it's undefined).

I found this use case: RFC 8702
"Use of the SHAKE One-Way Hash Functions in the Cryptographic Message Syntax (CMS)": https://www.rfc-editor.org/rfc/rfc8702.html

gniibe triaged this task as Normal priority.

Reading RFC 8702, I realized that it defines the hash size in the use of CMS as: SHAKE128 : 32-byte SHAKE256 : 64-byte.

I concluded that my idea of determining hash length by key size is not good.

I think that adding support of _gcry_md_read method for SHAKE128 and SHAKE256 (with reasonable default of 32 and 64) is good.

Here is a possible change (... to master, assuming it's good to support use case of RFC 8702):

diff --git a/cipher/keccak.c b/cipher/keccak.c
index 22c40302..76e08cb5 100644
--- a/cipher/keccak.c
+++ b/cipher/keccak.c
@@ -1630,8 +1630,8 @@ const gcry_md_spec_t _gcry_digest_spec_sha3_512 =
 const gcry_md_spec_t _gcry_digest_spec_shake128 =
   {
     GCRY_MD_SHAKE128, {0, 1},
-    "SHAKE128", shake128_asn, DIM (shake128_asn), oid_spec_shake128, 0,
-    shake128_init, keccak_write, keccak_final, NULL, keccak_extract,
+    "SHAKE128", shake128_asn, DIM (shake128_asn), oid_spec_shake128, 32,
+    shake128_init, keccak_write, keccak_final, keccak_read, keccak_extract,
     _gcry_shake128_hash_buffers,
     sizeof (KECCAK_CONTEXT),
     run_selftests
@@ -1639,8 +1639,8 @@ const gcry_md_spec_t _gcry_digest_spec_shake128 =
 const gcry_md_spec_t _gcry_digest_spec_shake256 =
   {
     GCRY_MD_SHAKE256, {0, 1},
-    "SHAKE256", shake256_asn, DIM (shake256_asn), oid_spec_shake256, 0,
-    shake256_init, keccak_write, keccak_final, NULL, keccak_extract,
+    "SHAKE256", shake256_asn, DIM (shake256_asn), oid_spec_shake256, 64,
+    shake256_init, keccak_write, keccak_final, keccak_read, keccak_extract,
     _gcry_shake256_hash_buffers,
     sizeof (KECCAK_CONTEXT),
     run_selftests

Thank you for having a look into that! The proposed patch looks good. Should we have this change also in master?

In regards to the second patch, I can not speak for your use case, but for FIPS using the SHAKE with the digest & sign API is not allowed by this time so we would either need to prevent it or create yet another fips service indicator to mark it a non-approved. Probably the same for the PBKDF2.

I found the case of X.509, which also uses fixed length output for RSA-PSS and ECDSA: https://www.rfc-editor.org/rfc/rfc8692.html

Also, I found that FIPS 186-5 addresses this use of SHAKE.

So, I suppose that the second patch is relevant to master.

From the FIPS 186-5 there are some limitations to use the SHAKE in FIPS Mode that we will have to reflect:

  • It can not be used with deterministic ECDSA (RFC6979) as FIPS mandates the use of the same as in the internal DRBG (and there is no DRBG with SHAKE defined) -- not claimed at this moment anyway, but I think we wanted to consider that one too. Described in the draft IG.
  • With EdDSA, it looks like only SHA512 and SHAKE256 are allowed, for Ed25519 and Ed448 respectively.

For the first issue, I added a check in: rCf65c30d470f5: cipher:ecc:fips: Reject use of SHAKE when it's ECDSA with RFC6979.

For the second issue, the choice of hash algorithm is embedded in Ed25519 (with SHA512) and in Ed448 (with SHAKE256).
For me, it would be better not to allow specifying it by a user.

diff --git a/cipher/ecc.c b/cipher/ecc.c
index e4f7ea3b..adc27890 100644
--- a/cipher/ecc.c
+++ b/cipher/ecc.c
@@ -941,9 +941,15 @@ ecc_sign (gcry_sexp_t *r_sig, gcry_sexp_t s_data, gcry_sexp_t keyparms)
   if (rc)
     goto leave;
 
-  /* Hash algo is determined by curve in EdDSA.  Fill it if not specified.  */
-  if ((ctx.flags & PUBKEY_FLAG_EDDSA) && !ctx.hash_algo)
+  /* Hash algo is determined by curve in EdDSA.  */
+  if ((ctx.flags & PUBKEY_FLAG_EDDSA))
     {
+      if (ctx.hash_algo)
+        {
+          rc = GPG_ERR_DIGEST_ALGO;
+          goto leave;
+        }
+
       if (ec->dialect == ECC_DIALECT_ED25519)
         ctx.hash_algo = GCRY_MD_SHA512;
       else if (ec->dialect == ECC_DIALECT_SAFECURVE)

No, there are use cases in GnuPG, where we specify the hash algo for signing, and our own tests/benchmark.c.

The fix is in 1.10.3.