FIPS 140 service indicator revamp
David Sugar
david at atsec.com
Thu Oct 24 14:21:26 CEST 2024
Hi NIIBE,
thank you for your proposal and your opinions on the matter.
I mostly share the same opinions as Stephan.
> 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).
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.
> 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
-------------- next part --------------
--- 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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.gnupg.org/pipermail/gcrypt-devel/attachments/20241024/ed4c7d98/attachment.sig>
More information about the Gcrypt-devel
mailing list