Does the patch really work, or is it a sketch to describe the intended use?

For me, the API looks not yet perfect. Specifically, in calculate_hash, s_hash is used to build an S-expression, but it's gone after exit of the function.

Please give us an example program which uses the new API.

Our public key functions are stateless. For several reasons it would be good to have an option to keep some state (think pre-computations). Our gcry_ctx_t would be a perfect fit for this and it will allow us to join a pubkey function with for example a hash function.

FWIW, there is also this newer patch: https://dev.gnupg.org/differential/diff/1476/
and SUSE seems to already use a modified API:
https://sources.suse.com/SUSE:Maintenance:15118/libgcrypt.SUSE_SLE-15_Update/26a8df5f96d27d6abca7bd7ba9b0def0/libgcrypt-FIPS-RSA-DSA-ECDSA-hashing-operation.patch

I think that {D1476} is still a sketch (not real code which works). I would guess an intended use, but it's good to have concrete example program which uses the feature being added.

OK. I think that the patch at SUSE is updated one which works.
As I understand correctly, this is a kind of very old patch, which intended to work around old libgcrypt limitation of RSA PSS.

For libgcrypt 1.7 or later, RSA PSS implementation got improved to support use cases in FIPS 186-4, namely different saltlen.

In an email from @werner couple days back, I got a suggestion that we could use hashing tied to the context, rather than this one-shot call tied only to digests. I circled back this suggestion to Stephan and he confirmed that it should be fine from the FIPS point of view so I am posting the suggested API here too:

ctx = gcry_pk_new (someflags)
md = gcry_md_open (...)
gcry_ctx_set_md (md);
gcry_pk_sign_ext (ctx, result, data, skey)
[...]
gcry_ctx_release (ctx);

Some ideas:

• the someflags thing will probably just be a reserved parameter
• If DATA is not NULL but an MD is set the sign function should fail
• Should ownership of MD be moved to the CTX?

I am considering API enhancement, for this task.

Along with an API enhancement, I think that discussion of use case(s) makes sense. So, here we go...

I reviewd patches here: https://build.opensuse.org/package/show/devel:libraries:c_c++/libgcrypt?expand=1

libgcrypt-PCT-DSA.patch
libgcrypt-PCT-ECC.patch
libgcrypt-PCT-RSA.patch
libgcrypt-FIPS-RSA-DSA-ECDSA-hashing-operation.patch

are the patches for this discussion. (I was wrong, it is not intended to support RSA-PSS.)

PCT: pairwise consistency test

I think that the interpretation of FIPS 140 (-2, for this case) was that PCT requires hashing in testing computation of signatures.

I am reading "Implementation Guidance for FIPS 140-3" and OpenSSL from git (for FIPS 140-2). Reading those as references, in my understanding, PCT would not actually require hashing in computation of signatures.

How do you thing about "2.4.B Tracking the Component Validation List" (page 17) of "Implementation Guidance for FIPS 140-3"?
https://csrc.nist.gov/CSRC/media/Projects/cryptographic-module-validation-program/documents/fips%20140-3/FIPS%20140-3%20IG.pdf

And... as long as I read the PCT patches, it is not needed to export those API to users.
It is only needed internally for PCT tests (at most).

Even with the interpretation of those signature+hash requirement, it can be done with no ABI changes (not exposing sign_md and verify_md routines).

So, my approach is... for now: considering an internal API enhancement, which can be possibly used for sign_md and verify_md.

In RHEL, we do not have anything about PCT so the PCT requirement is not completely clear to me: https://git.centos.org/rpms/libgcrypt/blob/c8s/f/SOURCES

The requirement for MD + sign operation together is not for the PCT tests, but for public API to make sure that the FIPS module is used to create the digest that we are signing and we are not able to sign arbitrary data. This means the new API, shuold be used by gnupg for example, but I am not sure how this would work with the agent interfaces.

Simo pointed out that usage of the digest flag might cause issues for signing very large data (RPMs, mail attachments ...) as the data would have to be re-encoded into the s-expressions in this case.

Ah, I understand the point (at least, partially); My understanding is: With FIPS mode, at the module boundary (== libgcrypt), it ensures that all cipher/digest/etc. operations are done under the standard compliance, and it is considered wrong (violation) when non-FIPS mode operation (such as SHA-1) and FIPS mode operation are mixed.

Also, I understand the point of larger data.

My suggestion for a combined function is a simple:

/* Variant of gcry_pk_sign which takes as additional parameter a HASH
 * handle and an optional context.  The hash algorithm used by the
 * handle needs to have the algorithm given by the DATA parameter
 * enabled.  The hash handle must not yet been finalized; the function
 * takes a copy of the state and does a finalize on the copy.  This
 * function shall be used if a policy requires that hashing and signing
 * is done by the same function.  CTX is currently not used and should
 * be passed as NULL.  */
gcry_error_t gcry_pk_hash_sign (gcry_sexp_t *result,
                                gcry_sexp_t data, gcry_sexp_t skey,
                                gcry_md_hd_t hash,
                                gcry_ctx_t ctx);

Thus we keep the use of our s-expression but allow to override the to-be-signed value by finalizing and reading the hash form HASH.

I am doing an experiment to implement gcry_pk_hash_sign.

Let me clarify the interface. The second argument, DATA, is a kind of template, specifying flags and hash algo, isn't it? (The actual data input to generate signature is put into the hash handle beforehand. )

I am looking the use cases in SuSE's patch set:
https://build.opensuse.org/package/view_file/devel:libraries:c_c++/libgcrypt/libgcrypt-PCT-DSA.patch?expand=1
https://build.opensuse.org/package/view_file/devel:libraries:c_c++/libgcrypt/libgcrypt-PCT-ECC.patch?expand=1
https://build.opensuse.org/package/view_file/devel:libraries:c_c++/libgcrypt/libgcrypt-PCT-RSA.patch?expand=1

If this is the case, let me propose an improve of the API. I think that specifying hash algorithm by DATA-template is not much useful, but it is easier for a user to assume _gcry_md_read (hd, 0) will be called in the function. This is natural, as the algorithm is should be enabled, anyway. And DATA-template is better to be string, which is used to generate SEXP in the function.

Pushed my initial implementation: rC117f5c3f8028: experiment-pk_hash_sign/verify: Implement pk_hash_sign/verify.

Thank you. On the first sight, it looks reasonable, but I would like to experiment with it a bit to see all use cases are covered.

Let me clarify the interface. The second argument, DATA, is a kind of template, specifying flags and hash algo, isn't it? (The actual data input to generate signature is put into the hash handle beforehand. )

Right. In the ACVP parser code I have the sexp template is passed with all the needed signature parameters, for example:

err = gcry_sexp_build (&s_data, NULL,
                       "(data (flags pkcs1)(hash-algo %s))",
                       gcry_md_algo_name(algo));

And DATA-template is better to be string, which is used to generate SEXP in the function.

I see that the (hash-algo %s) is basically duplicate information in the API in the example above. We pass the md context carrying this information so you suggest that that the caller should provide incomplete template that will be completed with the hash-algo part?

What all values (aside of the hash-algo) that can be passed in this template? We have flags and salt len for pss, prehash for eddsa, but not much more. I think we should be fine with the string. It should not be that complicate to construct it with snprintf's which support simple escaping:

err = snprintf (&s_data, sizeof(s_data),
                "(data"
                " (flags pss)"
                " (hash-algo %%s)"
                " (salt-length %d))",
                data->saltlen);

Thanks for your comment.

For concrete discussion, in rC9342b29516c4: experiment: Add tests for RSA-PSS., I pushed the use case with RSA-PSS.

Having hash-algo in the s-exp is useful because a hash handle may carry several hashes. This is sometimes useful if you do not know the hash algorithm in advance and you need to make a guess (various PGP compatibility things in gpg). But of course we can simplify this and use the default algo from the hash handle if hash-algo is missing.

While data template preparation for RSA-PSS is a bit tricky, it's simple with ECDSA.

For testing gcry_pk_hash_sign/verify, I push a commit: rC2c96df9e1928: experiment: Test gcry_pk_hash_sign/gcry_pk_hash_verify for ECDSA.

In the original SuSE's patch, _gcry_pk_sign_md function gets data template as SEXP as an argument, and the implementation does decomposing SEXP to get hash-algo. (A user of the function needs to compose SEXP with hash-algo.)

If reuse of hash handle is important, it would be good that just supply the argument hash-algo independently. Then, composing by user, decomposing by the implementation of the function will not be needed.

How about:

• Only when hash-handle is used for multiple purposes, a user needs to compose SEXP
• when hash-handle is used for a single purpose, a user doesn't need to compose SEXP, but static one.

Pushed the change: rC082ea0efa9b1: cipher: Add sign+hash, verify+hash, and random-override API.

Add doc in gcrypt.texi.

I have just a note about this issue, that it would be helpful to exercise this new API in some tests. Right now, only the old API is tested.

I though I saw something like that before, but I can not find it right now so it was probably part of the experimental branch with contexts. Would not be that hard for you to adapt some of the tests for the new API?

We have tests in gniibe/new-pk-api, which can be backported.

• t-dsa
• t-ecdsa
• t-rsa-pss
• t-rsa-15

Pushed the backport.

It turned out that the new *.inp files are not part of the release tarball, which makes the tests from generated tarball fail. The attached patch should fix this issue.

Thank you, applied.