User Details
- User Since
- Jan 6 2021, 10:04 AM (28 w, 2 d)
- Availability
- Available
Tue, Jul 13
I went through the patches above + what I suggested in previous comments, tested everything against both upstream and libgcrypt in Fedora in FIPS mode. There were slight differences, some cases were already fixed in master, some needed to upstream some of our changes, but the result is 10 patches working in both FIPS and non-fips mode, hopefully enough annotated. If not, please, ask for clarifications.
Mon, Jul 12
I went through the OpenSSL drafts. The module boundary in OpenSSL will be separate fips.so object and only non-deprecated functions of OpenSSL 3.0 will be FIPS compliant. There is a global state, that will allow only approved algorithms and modes and there will be API to query the FIPS mode status using OSSL_PARAM_get* functions, but we still have some unknowns so I hope we will know more on the next meeting.
Thu, Jul 8
I have couple of references from libssh:
There is no point in questioning whether a couple of words change racism or any other human problems of these days. It will not.
I was so far testing with changes on top of our patches.
Right. The AES-GCM was not allowed in FIPS mode until recently and I think now it is acceptable only for certain protocols (TLS, SSH), which guarantee that the IV is handled "correctly". As mentioned by gniibe, the requirements is that one should not be able to set IV to any specific value. The IV should be incremented automatically inside of the library (with some mask length + some generator configuration), somehow similarly as it is done with openssl, which would probably requite a new API in libgcrypt.
Wed, Jul 7
Thank you for checking and for revised patch. I tested your patch and it works fine for the basic test up until this failure with the crcalgo:
basic: algo 316, crcalgo: 3, gcry_md_open failed: Invalid digest algorithm basic: algo 317, crcalgo: 3, gcry_md_open failed: Invalid digest algorithm
These are GCRY_MD_SHAKE128 and GCRY_MD_SHAKE256, but the md used here is actually GCRY_MD_RMD160 which is hardcoded and not allowed in FIPS.
Tue, Jul 6
Mon, Jun 28
Thu, Jun 24
Jun 16 2021
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);
May 24 2021
Thank you. I checked what was missing and all looks good. But do not understand why the last gpgsplit xfree was not applied. We are leaving a block where this variable is dynamically allocated so even without error we need to free it.
May 11 2021
May 3 2021
Thank you for taking time to look into that. There are couple of issues in the CAcert bug tracker talking about the same issue but if, (I see right), the certs still miss the usage flags:
Any chance looking into this @werner?
Apr 28 2021
The patch references the following bug:
Apr 20 2021
Apr 15 2021
I hope last amendment is the following, which can happen if the tty can be opened only for reading but not for writing:
--- a/tty/pinentry-tty.c +++ b/tty/pinentry-tty.c @@ -583,7 +583,8 @@ tty_cmd_handler (pinentry_t pinentry) if (pinentry->ttyname) { fclose (ttyfi); - fclose (ttyfo); + if (ttyfo) + fclose (ttyfo); }
Apr 14 2021
Thank you for applying the provided changes!
Apr 13 2021
There is couple of issues that I did not want to propose a patch for, but might require some attention:
Error: IDENTICAL_BRANCHES (CWE-398): [#def28] [important]
gnupg-2.3.0/common/tlv-builder.c:353: identical_branches: The same code is executed regardless of whether "tag < 31" is true, because the 'then' and 'else' branches are identical. Should one of the branches be modified, or the entire 'if' statement replaced?
# 351| (void)constructed; /* Not used, but passed for uniformity of such calls. */
# 352|
# 353|-> if (tag < 0x1f)
# 354| {
# 355| buflen++;There are also couple of reports about the function default_homedir(), which is supposed to return const char * but in reality, it sometimes allocates memory while callers do not expect it so they do not free:
Error: RESOURCE_LEAK (CWE-772): [#def11] gnupg-2.2.27/common/homedir.c:477: alloc_fn: Storage is returned from allocation function "default_homedir". gnupg-2.2.27/common/homedir.c:477: var_assign: Assigning: "newdir" = storage returned from "default_homedir()". gnupg-2.2.27/common/homedir.c:488: noescape: Resource "newdir" is not freed or pointed-to in "make_absfilename". gnupg-2.2.27/common/homedir.c:490: leaked_storage: Returning without freeing "newdir" leaks the storage that it points to. # 488| the_gnupg_homedir = make_absfilename (newdir, NULL);; # 489| xfree (tmp); # 490|-> } # 491| # 492|
Thank you. The initial run was against olderer version of gnupg (and had one issue in g10/keyedit.c -- see the new patch with fixup). Now I ran it against the version 2.3 and there are couple of more issues to be fixed (rebased on top of already applied changes and the previous commits).
Apr 12 2021
(FYI I did not notice any other errors with 2.3 so far)
Apr 9 2021
Apr 8 2021
Apr 7 2021
Thanks. I understand that this is no big issue in the test code, but half of the code paths have proper cleaning already so fixing it once should save anyone in the future going through the same issues over and over again during our releases or anyone else who would run your code through static analyzer.
Apr 6 2021
FYI, I sent DCO to gnupg-devel@gnupg.org some moments ago, so I hope it arrived correctly.
Mar 30 2021
I already backported the above for Fedora so I am not in hurry now. But I believe others might hit the same issue.
Mar 25 2021
Thanks! Tested the above patches and now all the tests pass on the machine where I saw the failures.
Mar 24 2021
I have a minimal reproducer:
diff --git a/tests/basic.c b/tests/basic.c index 9a7e33cc..73ae01db 100644 --- a/tests/basic.c +++ b/tests/basic.c @@ -6346,11 +6346,152 @@ do_check_ocb_cipher (int inplace) "033ac4d13c3decc4c62d7de718ace802" "140452dc850989f6762e3578bbb04be3" "1a237c599c4649f4e586b2de" + }, + { GCRY_CIPHER_AES, 12, "0F0E0D0C0B0A09080706050403020100", + "BBAA9988776655443322110D", + "000102030405060708090A0B0C0D0E0F1011121314151617" + "18191A1B1C1D1E1F2021222324252627", + /* test vector for checksumming */ + "01000000000000000000000000000000" + "02000000000000000000000000000000" + "04000000000000000000000000000000" + "08000000000000000000000000000000" + "10000000000000000000000000000000" + "20000000000000000000000000000000" + "40000000000000000000000000000000" + "80000000000000000000000000000000" + "00010000000000000000000000000000" + "00020000000000000000000000000000" + "00040000000000000000000000000000" + "00080000000000000000000000000000" + "00100000000000000000000000000000" + "00200000000000000000000000000000" + "00400000000000000000000000000000" + "00800000000000000000000000000000" + "00000100000000000000000000000000" + "00000200000000000000000000000000" + "00000400000000000000000000000000" + "00000800000000000000000000000000" + "00001000000000000000000000000000" + "00002000000000000000000000000000" + "00004000000000000000000000000000" + "00008000000000000000000000000000" + "00000001000000000000000000000000" + "00000002000000000000000000000000" + "00000004000000000000000000000000" + "00000008000000000000000000000000" + "00000010000000000000000000000000" + "00000020000000000000000000000000" + "00000040000000000000000000000000" + "00000080000000000000000000000000" + "00000000010000000000000000000000" + "00000000020000000000000000000000" + "00000000040000000000000000000000" + "00000000080000000000000000000000" + "00000000100000000000000000000000" + "00000000200000000000000000000000" + "00000000400000000000000000000000" + "00000000800000000000000000000000" + "00000000000100000000000000000000" + "00000000000200000000000000000000" + "00000000000400000000000000000000" + "00000000000800000000000000000000" + "00000000001000000000000000000000" + "00000000002000000000000000000000" + "00000000004000000000000000000000" + "00000000008000000000000000000000" + "02000000000000000000000000000000" + "04000000000000000000000000000000" + "08000000000000000000000000000000" + "10000000000000000000000000000000" + "20000000000000000000000000000000" + "40000000000000000000000000000000" + "80000000000000000000000000000000" + "00010000000000000000000000000000" + "00020000000000000000000000000000" + "00040000000000000000000000000000" + "00080000000000000000000000000000" + "00100000000000000000000000000000" + "00200000000000000000000000000000" + "00400000000000000000000000000000" + "00800000000000000000000000000000" + "00000100000000000000000000000000" + "00000200000000000000000000000000" + "00000400000000000000000000000000" + "00000800000000000000000000000000", + "01105c6e36f6ac480f022c51e31ed702" + "90fda4b7b783194d4b4be8e4e1e2dff4" + "6a0804d1c5f9f808ea7933e31c063233" + "2bf65a22b20bb13cde3b80b3682ba965" + "b1207c58916f7856fa9968b410e50dee" + "98b35c071163d1b352b9bbccd09fde29" + "b850f40e71a8ae7d2e2d577f5ee39c46" + "7fa28130b50a123c29958e4665dda9a5" + "e0793997f8f19633a96392141d6e0e88" + "77850ed4364065d1d2f8746e2f1d5fd1" + "996cdde03215306503a30e41f58ef3c4" + "400365cfea4fa6381157c12a46598edf" + "18604854462ec66e3d3cf26d4723cb6a" + "9d801095048086a606fdb9192760889b" + "a8ce2e70e1b55a469137a9e2e6734565" + "283cb1e2c74f37e0854d03e33f8ba499" + "ef5d9af4edfce077c6280338f0a64286" + "2e6bc27ebd5a4c91b3778e22631251c8" + "c5bb75a10945597a9d6c274fc82d3338" + "b403a0a549d1375f26e71ef22bce0941" + "93ea87e2ed72fce0546148c351eec3be" + "867bb1b96070c377fff3c98e21562beb" + "475cfe28abcaaedf49981f6599b15140" + "ea6130d24407079f18ba9d4a8960b082" + "b39c57320e2e064f02fde88c23112146" + "1cac3655868aef584714826ee4f361fb" + "e6d692e1589cbb9dd3c74fa628df2a1f" + "3b0029b1d62b7e9978013ed3c793c1dd" + "1f184c8f7022a853cac40b74ac749aa3" + "f33f0d14732dfda0f2c3c20591bf1f5a" + "710ec0d0bca342baa5146068a78ff58c" + "66316312b7a98af35a0f4e92799b4047" + "f047ae61f25c28d232ce5c168cc745d6" + "6da13cb0f9e38a696635dba7a21571cf" + "cd64ec8cc33db7879f59a90d9edd00f6" + "a899e39ab36b9269a3ac04ebad9326bf" + "53cd9b400168a61714cd628a4056d236" + "bd8622c76daa54cb65f5db2fe03bafbe" + "0b23549ae31136f607293e8093a21934" + "74fd5e9c2451b4c8e0499e6ad34fafc8" + "ab77722a282f7f84b14ddebf7e696300" + "c1ef92d4a0263c6cca104530f996e272" + "f58992ff68d642b071a5848dc4acf2ae" + "28fb1f27ae0f297d5136a7a0a4a03e89" + "b588755b8217a1c62773790e69261269" + "19f45daf7b3ccf18e3fc590a9a0e172f" + "033ac4d13c3decc4c62d7de718ace802" + "140452dc850989f6762e3578bbb04be3" + "a8ae66427697167e85725b37b304baf0" + "56dbcef79fbb97cdfe1590e5f3d0bd1b" + "ce518f2f141960a1c80a4fe787b90b63" + "e7b0e0a0d8d522619130c544bb1abad0" + "b267c650e8916b5d7ececfeea7f0ad15" + "206a92581319946b138764f209109a20" + "0146b4cfb2ce8bd0db5c2cd5b495c56f" + "8f8a7934fe1f9add0674d4549080bf0d" + "01149ed18dbdccc5e54a3e7039546970" + "401ecc885902ee3dcfad504a68066f92" + "c779f1e1c48d37ba0e177ac652c1827b" + "f1f6723d533f0cdf36331e3ad1e1b1af" + "bc89a29c87fe3603353130d0dfbe1f29" + "13ad144e7c6515fb92005b6ece218b4f" + "baedc42d484fffee39df88041b49342a" + "6134cc7ca46d40d274c1ffafa98956e6" + "a492486989c4e328761c01798abcb09b" + "a42eb115334619daaeae9175f365fe9f" + "e5c3b254379d546005016784015f729f" + "4715ff6db16c5d16333e03fd" } }; gpg_error_t err = 0; gcry_cipher_hd_t hde, hdd; - unsigned char out[1024]; + unsigned char out[2048]; unsigned char tag[16]; int tidx;
Mar 23 2021
I did a bit digging and it looks like the code path using accelerator is not hit because the test vecors have max ~48 blocks, but accelerator is involved only with 64 blocks and more if I read the code right. So we need 1) larger test vector to invoke this code path in libgcrypt 2) figure out what goes wrong there.
Mar 5 2021
Feb 17 2021
Jan 27 2021
@werner hi. I do not seem to be able to open a bug report so I submitted this fix for a typo here. Hope it is fine. Can you check if there is something wrong with my account or configuration that I can not submit bug reports? It gives me
You do not have permission to prioritize tasks. Users with the "Can Prioritize Tasks" capability: