Hello, again,
I think that the behavior changes will be inevitably introduced, with
the FIPS 140 service indicator revamp. Under the FIPS mode, currently,
(in many cases,) it rejects computation in existing code of libgcrypt.
After the revamp, libgcrypt API won't reject the computation, but a
FIPS-conscious application needs to check if it's compliant or not
(using gcry_get_fips_service_indicator macro).
Perhaps, we need to introduce compatible flag to keep old behavior for
an application expecting rejection.
NIIBE Yutaka <gni...@fsij.org> wrote:
> I'm going to try #3 and #4 for gcry_md_buffer function.
This was done by the commit to master:
3478caac62c712547f7c0e07f4cf9602bc317997
Then, I proceed to modifying a set of functions which starts with
gcry_md_open.
Modifying those, I realized that the approach with the internal API of
fips_service_indicator
fips_service_indicator_mark_sucess
doesn't work well. That's because there may be multiple components
involved. For example, external API gcry_pk_* function may call
internal _gcry_md_* functions. External API gcry_mac_* function may
call internal _gcry_cipher_* functions, and/or _gcry_md_* functions.
Here, all calls should be compliant, not only one. When there are
multiple calls, a single path including mark_sucess doesn't mean it's
compliant.
Because of this, I'd like to propose a different apporach with:
fips_service_indicator
fips_service_indicator_mark_non_compliant
That is, initially, it assumes it's compliant and when anything
non-compliant computation involved, it marks non-compliant. In the
existing code of libgcrypt, it tries to reject non-compliant computation
actually, so, this approach matches well to the existing code.
Well, evaluating correctness with mark_success would be easier but this
approach limited to simple functions.
Attached are two patches. One for the internal API change. Another for
modifying a set of functions which starts with gcry_md_open. An
application which expects rejection can use the
GCRY_MD_FLAG_REJECT_NON_FIPS with gcry_md_open.
It's tracked by:
https://dev.gnupg.org/T7338
--
>From 90e1536abff97cb0f63c3f6f4213a672f5c6c21e Mon Sep 17 00:00:00 2001
From: NIIBE Yutaka <gni...@fsij.org>
Date: Thu, 12 Dec 2024 11:03:38 +0900
Subject: [PATCH 1/2] fips: Change the internal API for new FIPS service
indicator.
* src/gcrypt-int.h (fips_service_indicator_init): Initialize by 0.
(fips_service_indicator_mark_success): Remove.
(fips_service_indicator_mark_non_compliant): New.
* cipher/kdf.c (_gcry_kdf_derive): Follow the change of the API.
* cipher/md.c (_gcry_md_hash_buffer): Likewise.
(_gcry_md_hash_buffers_extract): Likewise.
--
GnuPG-bug-id: 7338
Signed-off-by: NIIBE Yutaka <gni...@fsij.org>
---
cipher/kdf.c | 17 +++++++++--------
cipher/md.c | 8 ++++----
src/gcrypt-int.h | 9 +++------
3 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/cipher/kdf.c b/cipher/kdf.c
index 1eae2b90..71156ea4 100644
--- a/cipher/kdf.c
+++ b/cipher/kdf.c
@@ -248,6 +248,7 @@ _gcry_kdf_derive (const void *passphrase, size_t passphraselen,
size_t keysize, void *keybuffer)
{
gpg_err_code_t ec;
+ int is_compliant_algo = 0;
if (!passphrase)
{
@@ -279,35 +280,32 @@ _gcry_kdf_derive (const void *passphrase, size_t passphraselen,
break;
case GCRY_KDF_PBKDF2:
+ is_compliant_algo = 1;
if (!saltlen || !iterations)
ec = GPG_ERR_INV_VALUE;
else
{
- int is_compliant = 1;
-
if (fips_mode ())
{
/* FIPS requires minimum passphrase length, see FIPS 140-3 IG D.N */
if (passphraselen < 8)
- is_compliant &= 0;
+ fips_service_indicator_mark_non_compliant ();
/* FIPS requires minimum salt length of 128 b (SP 800-132 sec. 5.1, p.6) */
if (saltlen < 16)
- is_compliant &= 0;
+ fips_service_indicator_mark_non_compliant ();
/* FIPS requires minimum iterations bound (SP 800-132 sec 5.2, p.6) */
if (iterations < 1000)
- is_compliant &= 0;
+ fips_service_indicator_mark_non_compliant ();
/* Check minimum key size */
if (keysize < 14)
- is_compliant &= 0;
+ fips_service_indicator_mark_non_compliant ();
}
ec = _gcry_kdf_pkdf2 (passphrase, passphraselen, subalgo,
salt, saltlen, iterations, keysize, keybuffer);
- if (!ec)
- fips_service_indicator_mark_success (is_compliant);
}
break;
@@ -326,6 +324,9 @@ _gcry_kdf_derive (const void *passphrase, size_t passphraselen,
break;
}
+ if (!ec && !is_compliant_algo && fips_mode ())
+ fips_service_indicator_mark_non_compliant ();
+
leave:
return ec;
}
diff --git a/cipher/md.c b/cipher/md.c
index c2bd18c6..ef2fc5a4 100644
--- a/cipher/md.c
+++ b/cipher/md.c
@@ -1286,8 +1286,8 @@ _gcry_md_hash_buffer (int algo, void *digest,
if (fips_mode ())
{
- int is_compliant = spec->flags.fips;
- fips_service_indicator_mark_success (is_compliant);
+ if (!spec->flags.fips)
+ fips_service_indicator_mark_non_compliant ();
}
}
@@ -1384,8 +1384,8 @@ _gcry_md_hash_buffers_extract (int algo, unsigned int flags, void *digest,
if (fips_mode ())
{
- int is_compliant = spec->flags.fips;
- fips_service_indicator_mark_success (is_compliant);
+ if (!spec->flags.fips)
+ fips_service_indicator_mark_non_compliant ();
}
return 0;
diff --git a/src/gcrypt-int.h b/src/gcrypt-int.h
index 7f894737..aa49d766 100644
--- a/src/gcrypt-int.h
+++ b/src/gcrypt-int.h
@@ -303,13 +303,10 @@ unsigned long _gcry_thread_context_get_fsi (void);
#define fips_service_indicator_init() do \
{ \
if (fips_mode ()) \
- _gcry_thread_context_set_fsi (1); \
- } while (0)
-#define fips_service_indicator_mark_success(is_compliant) do \
- { \
- if (is_compliant && fips_mode ()) \
- _gcry_thread_context_set_fsi (0); \
+ _gcry_thread_context_set_fsi (0); \
} while (0)
+/* Should be used only when fips_mode()==TRUE. */
+#define fips_service_indicator_mark_non_compliant() _gcry_thread_context_set_fsi (1)
/* Return a pointer to a string containing a description of the error
code in the error value ERR. */
--
2.39.5
>From f2ef8c98fe5305b9d08c351724386f3111e611f3 Mon Sep 17 00:00:00 2001
From: NIIBE Yutaka <gni...@fsij.org>
Date: Thu, 12 Dec 2024 11:40:31 +0900
Subject: [PATCH 2/2] fips,md: Implement new FIPS service indicator for
gcry_md_open API.
* src/gcrypt.h.in (GCRY_MD_FLAG_FIPS_NO_REJECTION): Remove.
(GCRY_MD_FLAG_REJECT_NON_FIPS): New.
* cipher/md.c (struct gcry_md_context): Add reject_non_fips.
(md_enable): Remove NO_REJECT argument.
(md_open): Change the FLAGS handling.
(_gcry_md_open): Add checking of FIPS compliance against ALGO.
(_gcry_md_enable): Likewise.
(_gcry_md_hash_buffer): Follow the change of md_open change
which now defaults to no rejection.
(_gcry_md_hash_buffers_extract): Likewise.
* src/visibility.c (gcry_md_open): Add fips_service_indicator_init.
(gcry_md_enable): Likewise.
(gcry_md_setkey): Don't reject but mark non-compliance.
* tests/t-kdf.c (check_fips_gcry_kdf_derive): Add a test with
non-compliant hash function.
* cipher/mac-hmac.c (_gcry_mac_type_spec_hmac_md5): It's not
compliant.
* cipher/md5.c (gcry_md_oid_spec_t oid_spec_md5): It's not compliant.
* tests/t-digest.c (check_hash_buffer, check_hash_buffers): MD5
tests enabled.
--
See 6376 for the MD5 compliance change in the past. This commit
reverts the change in:
dc4a60e2d70bc52ba2955f8e676341d675ab89a0
GnuPG-bug-id: 7338
Signed-off-by: NIIBE Yutaka <gni...@fsij.org>
---
cipher/mac-hmac.c | 2 +-
cipher/md.c | 57 +++++++++++++++++++++++++++++++++++++++--------
cipher/md5.c | 2 +-
src/gcrypt.h.in | 2 +-
src/visibility.c | 6 +++--
tests/t-digest.c | 6 ++---
tests/t-kdf.c | 12 ++++++++++
7 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/cipher/mac-hmac.c b/cipher/mac-hmac.c
index a5acab70..09e259bb 100644
--- a/cipher/mac-hmac.c
+++ b/cipher/mac-hmac.c
@@ -1413,7 +1413,7 @@ const gcry_mac_spec_t _gcry_mac_type_spec_hmac_tiger1 = {
#endif
#if USE_MD5
const gcry_mac_spec_t _gcry_mac_type_spec_hmac_md5 = {
- GCRY_MAC_HMAC_MD5, {0, 1}, "HMAC_MD5",
+ GCRY_MAC_HMAC_MD5, {0, 0}, "HMAC_MD5",
&hmac_ops
};
#endif
diff --git a/cipher/md.c b/cipher/md.c
index ef2fc5a4..666e1dfa 100644
--- a/cipher/md.c
+++ b/cipher/md.c
@@ -275,6 +275,7 @@ struct gcry_md_context
unsigned int finalized:1;
unsigned int bugemu1:1;
unsigned int hmac:1;
+ unsigned int reject_non_fips:1;
} flags;
size_t actual_handle_size; /* Allocated size of this handle. */
FILE *debug;
@@ -285,7 +286,7 @@ struct gcry_md_context
#define CTX_MAGIC_NORMAL 0x11071961
#define CTX_MAGIC_SECURE 0x16917011
-static gcry_err_code_t md_enable (gcry_md_hd_t hd, int algo, int no_reject);
+static gcry_err_code_t md_enable (gcry_md_hd_t hd, int algo);
static void md_close (gcry_md_hd_t a);
static void md_write (gcry_md_hd_t a, const void *inbuf, size_t inlen);
static byte *md_read( gcry_md_hd_t a, int algo );
@@ -508,6 +509,7 @@ md_open (gcry_md_hd_t *h, int algo, unsigned int flags)
ctx->flags.secure = secure;
ctx->flags.hmac = hmac;
ctx->flags.bugemu1 = !!(flags & GCRY_MD_FLAG_BUGEMU1);
+ ctx->flags.reject_non_fips = !!(flags & GCRY_MD_FLAG_REJECT_NON_FIPS);
}
if (! err)
@@ -517,8 +519,7 @@ md_open (gcry_md_hd_t *h, int algo, unsigned int flags)
if (algo)
{
- err = md_enable (hd, algo,
- !!(flags & GCRY_MD_FLAG_FIPS_NO_REJECTION));
+ err = md_enable (hd, algo);
if (err)
md_close (hd);
}
@@ -543,24 +544,44 @@ _gcry_md_open (gcry_md_hd_t *h, int algo, unsigned int flags)
if ((flags & ~(GCRY_MD_FLAG_SECURE
| GCRY_MD_FLAG_HMAC
+ | GCRY_MD_FLAG_REJECT_NON_FIPS
| GCRY_MD_FLAG_BUGEMU1)))
rc = GPG_ERR_INV_ARG;
else
rc = md_open (&hd, algo, flags);
*h = rc? NULL : hd;
+
+ if (!rc && fips_mode ())
+ {
+ GcryDigestEntry *entry = hd->ctx->list;
+ /* No ENTRY means that ALGO==0.
+ It's not yet known, if it's FIPS compliant or not. */
+ int is_compliant_algo = 1;
+
+ if (entry)
+ {
+ const gcry_md_spec_t *spec = entry->spec;
+ is_compliant_algo = spec->flags.fips;
+ }
+
+ if (!is_compliant_algo)
+ fips_service_indicator_mark_non_compliant ();
+ }
+
return rc;
}
static gcry_err_code_t
-md_enable (gcry_md_hd_t hd, int algorithm, int no_reject)
+md_enable (gcry_md_hd_t hd, int algorithm)
{
struct gcry_md_context *h = hd->ctx;
const gcry_md_spec_t *spec;
GcryDigestEntry *entry;
gcry_err_code_t err = 0;
+ int reject_non_fips = h->flags.reject_non_fips;
for (entry = h->list; entry; entry = entry->next)
if (entry->spec->algo == algorithm)
@@ -577,7 +598,7 @@ md_enable (gcry_md_hd_t hd, int algorithm, int no_reject)
err = GPG_ERR_DIGEST_ALGO;
/* Any non-FIPS algorithm should go this way */
- if (!err && !no_reject && !spec->flags.fips && fips_mode ())
+ if (!err && reject_non_fips && !spec->flags.fips && fips_mode ())
err = GPG_ERR_DIGEST_ALGO;
if (!err && h->flags.hmac && spec->read == NULL)
@@ -620,7 +641,26 @@ md_enable (gcry_md_hd_t hd, int algorithm, int no_reject)
gcry_err_code_t
_gcry_md_enable (gcry_md_hd_t hd, int algorithm)
{
- return md_enable (hd, algorithm, 0);
+ gcry_err_code_t rc;
+
+ rc = md_enable (hd, algorithm);
+ if (!rc && fips_mode ())
+ {
+ GcryDigestEntry *entry = hd->ctx->list;
+ /* No ENTRY means, something goes wrong. */
+ int is_compliant_algo = 0;
+
+ if (entry)
+ {
+ const gcry_md_spec_t *spec = entry->spec;
+ is_compliant_algo = spec->flags.fips;
+ }
+
+ if (!is_compliant_algo)
+ fips_service_indicator_mark_non_compliant ();
+ }
+
+ return rc;
}
@@ -1274,7 +1314,7 @@ _gcry_md_hash_buffer (int algo, void *digest,
gcry_md_hd_t h;
gpg_err_code_t err;
- err = md_open (&h, algo, GCRY_MD_FLAG_FIPS_NO_REJECTION);
+ err = md_open (&h, algo, 0);
if (err)
log_bug ("gcry_md_open failed for algo %d: %s",
algo, gpg_strerror (gcry_error(err)));
@@ -1355,8 +1395,7 @@ _gcry_md_hash_buffers_extract (int algo, unsigned int flags, void *digest,
gcry_md_hd_t h;
gpg_err_code_t rc;
- rc = md_open (&h, algo, ((hmac? GCRY_MD_FLAG_HMAC:0)
- | GCRY_MD_FLAG_FIPS_NO_REJECTION));
+ rc = md_open (&h, algo, (hmac? GCRY_MD_FLAG_HMAC:0));
if (rc)
return rc;
diff --git a/cipher/md5.c b/cipher/md5.c
index b807da55..e6cccc5b 100644
--- a/cipher/md5.c
+++ b/cipher/md5.c
@@ -314,7 +314,7 @@ static const gcry_md_oid_spec_t oid_spec_md5[] =
const gcry_md_spec_t _gcry_digest_spec_md5 =
{
- GCRY_MD_MD5, {0, 1},
+ GCRY_MD_MD5, {0, 0},
"MD5", asn, DIM (asn), oid_spec_md5, 16,
md5_init, _gcry_md_block_write, md5_final, md5_read, NULL,
NULL,
diff --git a/src/gcrypt.h.in b/src/gcrypt.h.in
index 18d04a38..96bf88f6 100644
--- a/src/gcrypt.h.in
+++ b/src/gcrypt.h.in
@@ -1318,7 +1318,7 @@ enum gcry_md_flags
{
GCRY_MD_FLAG_SECURE = 1, /* Allocate all buffers in "secure" memory. */
GCRY_MD_FLAG_HMAC = 2, /* Make an HMAC out of this algorithm. */
- GCRY_MD_FLAG_FIPS_NO_REJECTION = 4, /* Don't reject for FIPS. */
+ GCRY_MD_FLAG_REJECT_NON_FIPS = 4, /* Reject non-FIPS-compliant algo. */
GCRY_MD_FLAG_BUGEMU1 = 0x0100
};
diff --git a/src/visibility.c b/src/visibility.c
index be5deda1..44b05eb2 100644
--- a/src/visibility.c
+++ b/src/visibility.c
@@ -1204,7 +1204,7 @@ gcry_md_open (gcry_md_hd_t *h, int algo, unsigned int flags)
*h = NULL;
return gpg_error (fips_not_operational ());
}
-
+ fips_service_indicator_init ();
return gpg_error (_gcry_md_open (h, algo, flags));
}
@@ -1219,6 +1219,7 @@ gcry_md_enable (gcry_md_hd_t hd, int algo)
{
if (!fips_is_operational ())
return gpg_error (fips_not_operational ());
+ fips_service_indicator_init ();
return gpg_error (_gcry_md_enable (hd, algo));
}
@@ -1382,8 +1383,9 @@ gcry_md_setkey (gcry_md_hd_t hd, const void *key, size_t keylen)
if (!fips_is_operational ())
return gpg_error (fips_not_operational ());
+ fips_service_indicator_init ();
if (fips_mode () && keylen < 14)
- return GPG_ERR_INV_VALUE;
+ fips_service_indicator_mark_non_compliant ();
return gpg_error (_gcry_md_setkey (hd, key, keylen));
}
diff --git a/tests/t-digest.c b/tests/t-digest.c
index a6179f57..f5549403 100644
--- a/tests/t-digest.c
+++ b/tests/t-digest.c
@@ -48,8 +48,7 @@ check_hash_buffer (void)
const char *expect;
int expect_failure;
} tv[] = {
-#undef ENABLE_THIS_AFTER_T6376_CHANGE_REVISED
-#if USE_MD5 && defined(ENABLE_THIS_AFTER_T6376_CHANGE_REVISED)
+#if USE_MD5
{ GCRY_MD_MD5, "abc", 3,
"\x90\x01\x50\x98\x3C\xD2\x4F\xB0\xD6\x96\x3F\x7D\x28\xE1\x7F\x72", 1 },
#endif
@@ -156,8 +155,7 @@ check_hash_buffers (void)
const char *expect;
int expect_failure;
} tv[] = {
-#undef ENABLE_THIS_AFTER_T6376_CHANGE_REVISED
-#if USE_MD5 && defined(ENABLE_THIS_AFTER_T6376_CHANGE_REVISED)
+#if USE_MD5
{ GCRY_MD_MD5, "abc", 3,
"key", 3,
"\xd2\xfe\x98\x06\x3f\x87\x6b\x03\x19\x3a\xfb\x49\xb4\x97\x95\x91", 1 },
diff --git a/tests/t-kdf.c b/tests/t-kdf.c
index 4b92bd30..3682c040 100644
--- a/tests/t-kdf.c
+++ b/tests/t-kdf.c
@@ -2008,6 +2008,18 @@ check_fips_gcry_kdf_derive (void)
"\xd8\x36\x62",
1 /* not-compliant because key size too small */
},
+ {
+ "passwordPASSWORDpassword", 24,
+ GCRY_KDF_PBKDF2, GCRY_MD_BLAKE2B_512,
+ "saltSALTsaltSALTsaltSALTsaltSALTsalt", 36,
+ 4096,
+ 60,
+ "\xa4\x6b\x53\x35\xdb\xdd\xa3\xd2\x5d\x19\xbb\x11\xfe\xdd\xd9\x9e"
+ "\x45\x2a\x7c\x34\x47\x41\x98\xca\x31\x74\xb6\x34\x22\xac\x83\xb0"
+ "\x38\x6e\xf5\x93\x0f\xf5\x16\x46\x0b\x97\xdc\x6c\x27\x5b\xe7\x25"
+ "\xc2\xcb\xec\x50\x02\xc6\x52\x8b\x34\x68\x53\x65",
+ 1 /* not-compliant because subalgo is not the one of approved */
+ }
};
int tvidx;
--
2.39.5
_______________________________________________
Gcrypt-devel mailing list
Gcrypt-devel@gnupg.org
https://lists.gnupg.org/mailman/listinfo/gcrypt-devel
