Re: FIPS 140 service indicator revamp

David Sugar via Gcrypt-devel <[email protected]> Thu, 24 Oct 2024 14:21:26 +0200

Newsgroups	gmane.comp.encryption.gpg.libgcrypt.devel
Message-ID	<[email protected]>

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

_______________________________________________
Gcrypt-devel mailing list
[email protected]
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel

gcry_kdf_derive_test.patch (text/plain, 5.9 KB)

--- 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 (application/pgp-signature, 840 B)