Hi NIIBE, thank you for your proposal and your opinions on the matter.
I mostly share the same opinions as Stephan.
I wouldn't say that it is difficult to handle but its definitely a little bit confusing as one would normally expect an errno to be set in conjunction with a error code and not success, i.e., by following this path one has to properly document the behavior. I also wouldn't say that errno is overloaded because at the point errno is set for FIPS, other errno values related to errors should have already been handled internally.Could we ask you to reserve this decision, at this point? I can imagine the need for a thread local variable for the service indicator, but I think that overloading ERRNO might be difficult to handle and/or confusing for applications (library users).
Well, may I ask for your patch also including a change of tests/t-kdf.c? By doing so, it will be more clear to see how the change of libgcrypt asks application changes.
Sure, I've attached a diff. Best regards David On 24.10.24 11:08, Stephan Mueller wrote:
Am Donnerstag, 24. Oktober 2024, 03:34:31 Mitteleuropäische Sommerzeit schrieb NIIBE Yutaka via Gcrypt-devel: Hi NIIBE,Hello, Thank you for your proposal with a patch. David Sugar wrote:Libgcrypt implements a FIPS 140 service indicator to comply with the rules stipulated by FIPS 140-3. However, based on recent clarifications provided by NIST around the topic, the current implementation falls short of meeting these clarifications. The current service indicator "statically" returns the indicator whether an algorithm operates in a FIPS-compliant manner or not. This includes the indicator whether the cryptographic algorithm is an "approved" algorithm or not. The issue with this approach is that it is "static" in the sense that a caller asks libgcrypt whether algorithm X is approved or not. NIST clarified that such static behavior is not sufficient. NIST requests a "dynamic" indicator in the sense that during the processing of the actual request to perform a cryptographic operation, the indicator must be "generated" and "returned" to the caller. I.e. when the caller performs, say an RSA 1024 operation, that very API call is required to generate the indicator that the request was not FIPS compliant. Conversely, when the caller performs an RSA 2048 operation, that very API call must generate the indicator that it was an approved mechanism.I see the situation. Let us improve. (Well, I don't have a skill for the better wording/expression in English. I use "us", meaning: gpg-team + you + all, including its users.)Our solution we offer here is the use of the errno to convey the FIPS status indicator.Could we ask you to reserve this decision, at this point? I can imagine the need for a thread local variable for the service indicator, but I think that overloading ERRNO might be difficult to handle and/or confusing for applications (library users). For this, firstly, I propose the API of following: void _gcry_thread_context_set_fsi (unsigned long fsi); unsigned long _gcry_thread_context_get_fsi (void); unsigned long gcry_thread_context_get_fsi (void); (Function name change/suggestion is welcome, here, fsi stands for FIPS service indicator.) Two functions are internal use for libgcrypt. The last function is exposed to libgcrypt users. Implementation may be the one with ERRNO actually later, but for now, let us hide and defer the implementation detail. It's true that libgcrypt code-base doesn't yet have thread specific context things; That's long-standing issue to be done. Since libgcrypt assumes C90+something (only) for build environment and tries to support various OS and compilers, it would be tricky to implement. For this particular problem, I created a separate ticket (and a branch gniibe/t7340): https://dev.gnupg.org/T7340 # So far, I found that use of pre-C99 non-standard __thread specifier # seems work well. Any feedback is welcome for this.Thanks very much for sharing this suggestion. I see no issue with it from an API / FIPS point of view. But since I am also maintaining a crypto lib (leancrypto), I am intrigued to learn how such thread-local variables can be maintained. I think I have to learn about the __thread specifier :-)The provided patch adjusts one cryptographic service to show the chosen approach. If we all agree, we will proceed in developing the patch series to cover the other services appropriately.Thanks a lot for this constructive approach.- if the API return code is 0 -> errno contains the FIPS service indicator - if the API return code is != 0 -> errno contains the "regular" error indicator (i.e. the patch will not touch the errno)Currently, I don't know if this is good. My concern here is that this will be a fundamental API change for libgcrypt users; I'm afraid it requires changes for all/most applications.Ok, but I do not see it this way, because the API (disregarding the errno in the successful conclusion of the API) is unchanged. Only if the API concludes successfully *and* a caller is interested in FIPS, it may want to consult th errno. In a success case, usually no caller would consult the errno.In my opinion, two points are important. * A function may be finished earlier, not completing the operation. In this case, the API return code should not be 0 (keeping the API).That case should not be affected by the suggestion, because the API returns with a GPG error indicator.* Introducing use of gcry_thread_context_get_fsi (after a function call) makes sense for FIPS certified applications to make sure if the function call of libgcrypt does not violate FIPS (new API).Ok sure, as said above, no issue from our side.* * * Well, may I ask for your patch also including a change of tests/t-kdf.c? By doing so, it will be more clear to see how the change of libgcrypt asks application changes.I would like to ask my colleague, David, for that. Ciao Stephan
-- atsec information security GmbH, Ismaninger Str. 19, 81675 München, Germany Phone: +49-89-44249840 / Web: atsec.com HRB: 129439 (Amtsgericht München) Geschaeftsfuehrer: Staffan Persson, Dr. Michael Vogel
--- libgcrypt/tests/t-kdf.c 2024-10-10 11:13:04.000000000 +0200
+++ libgcrypt-fips2/tests/t-kdf.c 2024-10-14 13:11:57.305374486 +0200
@@ -26,6 +26,7 @@
#include <string.h>
#include <stdarg.h>
#include <assert.h>
+#include <errno.h>
#include "stopwatch.h"
#define PGM "t-kdf"
@@ -1102,6 +1103,7 @@
gpg_error_t err;
unsigned char outbuf[100];
int i;
+ int my_errno;
for (tvidx=0; tvidx < DIM(tv); tvidx++)
{
@@ -1117,14 +1119,15 @@
GCRY_KDF_PBKDF2, tv[tvidx].hashalgo,
tv[tvidx].salt, tv[tvidx].saltlen,
tv[tvidx].c, tv[tvidx].dklen, outbuf);
+ my_errno = errno;
if (in_fips_mode && tvidx > 7)
{
- if (!err)
+ if (my_errno != EOPNOTSUPP)
fail ("pbkdf2 test %d unexpectedly passed in FIPS mode: %s\n",
tvidx, gpg_strerror (err));
continue;
}
- if (err)
+ if (err || my_errno == EOPNOTSUPP)
{
if (in_fips_mode && (tv[tvidx].plen < 14 || tv[tvidx].dklen < 14))
{
@@ -1926,6 +1929,145 @@
}
}
+static void
+check_fips_gcry_kdf_derive(void)
+{
+ static struct {
+ const char *p; /* Passphrase. */
+ size_t plen; /* Length of P. */
+ int algo;
+ int subalgo;
+ const char *salt;
+ size_t saltlen;
+ unsigned long iterations;
+ int dklen; /* Requested key length. */
+ const char *dk; /* Derived key. */
+ int expect_error;
+ } tv[] = {
+ {
+ "passwordPASSWORDpassword", 24,
+ GCRY_KDF_PBKDF2, GCRY_MD_SHA1,
+ "saltSALTsaltSALTsaltSALTsaltSALTsalt", 36,
+ 4096,
+ 25,
+ "\x3d\x2e\xec\x4f\xe4\x1c\x84\x9b\x80\xc8"
+ "\xd8\x36\x62\xc0\xe4\x4a\x8b\x29\x1a\x96"
+ "\x4c\xf2\xf0\x70\x38",
+ 0
+ },
+ {
+ "pleaseletmein", 13,
+ GCRY_KDF_SCRYPT, 8,
+ "SodiumChloride", 14,
+ 1,
+ 64,
+ "\x70\x23\xbd\xcb\x3a\xfd\x73\x48\x46\x1c\x06\xcd\x81\xfd\x38\xeb"
+ "\xfd\xa8\xfb\xba\x90\x4f\x8e\x3e\xa9\xb5\x43\xf6\x54\x5d\xa1\xf2"
+ "\xd5\x43\x29\x55\x61\x3f\x0f\xcf\x62\xd4\x97\x05\x24\x2a\x9a\xf9"
+ "\xe6\x1e\x85\xdc\x0d\x65\x1e\x40\xdf\xcf\x01\x7b\x45\x57\x58\x87",
+ 1 /* forbidden because unallowed algo */
+ },
+ {
+ "passwor", 7,
+ GCRY_KDF_PBKDF2, GCRY_MD_SHA1,
+ "saltSALTsaltSALTsaltSALTsaltSALTsalt", 36,
+ 4096,
+ 25,
+ "\x3d\x2e\xec\x4f\xe4\x1c\x84\x9b\x80\xc8"
+ "\xd8\x36\x62\xc0\xe4\x4a\x8b\x29\x1a\x96"
+ "\x4c\xf2\xf0\x70\x38", /* this is wrong but we don't care because
+ it should fail anyway */
+ 1 /* forbidden because passphrase len is too small */
+ },
+ {
+ "passwordPASSWORDpassword", 24,
+ GCRY_KDF_PBKDF2, GCRY_MD_SHA1,
+ "saltSALTsaltSAL", 15,
+ 4096,
+ 25,
+ "\x3d\x2e\xec\x4f\xe4\x1c\x84\x9b\x80\xc8"
+ "\xd8\x36\x62\xc0\xe4\x4a\x8b\x29\x1a\x96"
+ "\x4c\xf2\xf0\x70\x38", /* this is wrong but we don't care because
+ it should fail anyway */
+ 1 /* forbidden because salt len is too small */
+ },
+ {
+ "passwordPASSWORDpassword", 24,
+ GCRY_KDF_PBKDF2, GCRY_MD_SHA1,
+ "saltSALTsaltSALTsaltSALTsaltSALTsalt", 36,
+ 999,
+ 25,
+ "\x3d\x2e\xec\x4f\xe4\x1c\x84\x9b\x80\xc8"
+ "\xd8\x36\x62\xc0\xe4\x4a\x8b\x29\x1a\x96"
+ "\x4c\xf2\xf0\x70\x38", /* this is wrong but we don't care because
+ it should fail anyway */
+ 1 /* forbidden because too few iterations */
+ },
+ {
+ "passwordPASSWORDpassword", 24,
+ GCRY_KDF_PBKDF2, GCRY_MD_SHA1,
+ "saltSALTsaltSALTsaltSALTsaltSALTsalt", 36,
+ 4096,
+ 13,
+ "\x3d\x2e\xec\x4f\xe4\x1c\x84\x9b\x80\xc8"
+ "\xd8\x36\x62", /* this is wrong but we don't care because
+ it should fail anyway */
+ 1 /* forbidden because key size too small */
+ },
+ };
+
+ int tvidx;
+ gpg_error_t err;
+ unsigned char outbuf[100];
+ int i;
+ int my_errno;
+
+ for (tvidx=0; tvidx < DIM(tv); tvidx++)
+ {
+ if (verbose)
+ fprintf (stderr, "checking gcry_kdf_derive test vector %d algo %d for
fips\n",
+ tvidx, tv[tvidx].algo);
+ assert (tv[tvidx].dklen <= sizeof outbuf);
+ err = gcry_kdf_derive (tv[tvidx].p, tv[tvidx].plen,
+ tv[tvidx].algo, tv[tvidx].subalgo,
+ tv[tvidx].salt, tv[tvidx].saltlen,
+ tv[tvidx].iterations, tv[tvidx].dklen, outbuf);
+ // Errno has to be fetched directly after the call
+ my_errno = errno;
+
+ if (err)
+ {
+ fail ("gcry_kdf_derive test %d unexpectedly returned an error in
FIPS mode: %s\n",
+ tvidx, gpg_strerror (err));
+ continue;
+ }
+ else
+ {
+ if (my_errno == EOPNOTSUPP && tv[tvidx].expect_error == 0)
+ {
+
+ fail ("gcry_kdf_derive test %d unexpectedly set errno '%s' in
FIPS mode.\n",
+ tvidx, strerror(my_errno), strerror(EOPNOTSUPP));
+ continue;
+ }
+ else if (my_errno != EOPNOTSUPP && tv[tvidx].expect_error)
+ {
+ fail ("gcry_kdf_derive test %d expected EOPNOTSUPP in FIPS
mode\n",
+ tvidx);
+ continue;
+ }
+
+ if (my_errno == 0 && memcmp (outbuf, tv[tvidx].dk, tv[tvidx].dklen))
+ {
+ fail ("gcry_kdf_derive test %d failed: mismatch\n", tvidx);
+ fputs ("got:", stderr);
+ for (i=0; i < tv[tvidx].dklen; i++)
+ fprintf (stderr, " %02x", outbuf[i]);
+ putc ('\n', stderr);
+ }
+ }
+ }
+}
int
main (int argc, char **argv)
@@ -2009,6 +2151,8 @@
check_hkdf ();
if (in_fips_mode)
check_fips_indicators();
+ if (in_fips_mode)
+ check_fips_gcry_kdf_derive();
}
return error_count ? 1 : 0;
OpenPGP_signature.asc
Description: OpenPGP digital signature
_______________________________________________ Gcrypt-devel mailing list Gcrypt-devel@gnupg.org https://lists.gnupg.org/mailman/listinfo/gcrypt-devel
