URL: https://github.com/SSSD/sssd/pull/826 Author: alexey-tikhonov Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1() Action: opened
PR body: """ Changed libcrypto/sss_hmac_sha1 implementation to be FIPS140 compliant.
Resolves: https://pagure.io/SSSD/sssd/issue/4022 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/826/head:pr826 git checkout pr826
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
simo5 commented: """ LGTM """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-501428262
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
mzidek-rh commented: """ I just tried to do the following as part of testing. I am already tired so, maybe I am just not seeing something, but I think the cypto-tests in the unit tests suite are not working. I made these changes that should probably break the function:
``` diff --git a/src/util/crypto/libcrypto/crypto_hmac_sha1.c b/src/util/crypto/libcrypto/crypto_hmac_sha1.c index 1499ace1b..ab26926a4 100644 --- a/src/util/crypto/libcrypto/crypto_hmac_sha1.c +++ b/src/util/crypto/libcrypto/crypto_hmac_sha1.c @@ -46,7 +46,7 @@ int sss_hmac_sha1(const unsigned char *key, return ENOMEM; }
- if (key_len > HMAC_SHA1_BLOCKSIZE) { + if (key_len < HMAC_SHA1_BLOCKSIZE) { /* keys longer than blocksize are shortened */ if (!EVP_DigestInit_ex(ctx, EVP_sha1(), NULL)) { ret = EIO; @@ -60,14 +60,14 @@ int sss_hmac_sha1(const unsigned char *key, /* keys shorter than blocksize are zero-padded */ memcpy(ikey, key, key_len); if (key_len < HMAC_SHA1_BLOCKSIZE) { - memset(ikey + key_len, 0, HMAC_SHA1_BLOCKSIZE - key_len); + memset(ikey + key_len, 1, HMAC_SHA1_BLOCKSIZE - key_len); } }
/* HMAC(key, msg) = HASH(key XOR opad, HASH(key XOR ipad, msg)) */ - for (i = 0; i < HMAC_SHA1_BLOCKSIZE; i++) { - okey[i] = ikey[i] ^ 0x5c; - ikey[i] ^= 0x36; + for (i = 0; i > HMAC_SHA1_BLOCKSIZE; i++) { + okey[0] = ikey[i] ^ 0x5c; + ikey[0] ^= 0x36; }
if (!EVP_DigestInit_ex(ctx, EVP_sha1(), NULL)) {
```
Just added a bunch of random "bugs". But the tests still pass :/
My plan was to: 1. check if we have tests that use sss_hmac_sha1 function 2. break the function to see the tests fail (just checking if the tests are testing something) 3. apply this PR and see if the tests pass
But it looks like the test is unbreakable for me :/
That being said, it says nothing about changes in this PR, and they are probably good. Just wanted to note this. """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-501837341
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
alexey-tikhonov commented: """ @mzidek-rh , I applied your patch and test fails for me: ``` Running suite(s): sss_crypto 83%: Checks: 6, Failures: 1, Errors: 0 ../src/tests/crypto-tests.c:125:F:sss crypto tests:test_hmac_sha1:0: Failure 'ret == EOK && memcmp(out, results[i], SSS_SHA1_LENGTH) != 0' occurred ``` """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-502061835
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
mzidek-rh commented: """ @alexey-tikhonov Indeed, I was testing it on my localhost machine with different configuration and it did not use that function. Running in VM with enabled libcrypto makes the test fail.
@jhrozek do you know if the PR CI uses libcrypto or nss? """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-502127106
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
alexey-tikhonov commented: """
@jhrozek do you know if the PR CI uses libcrypto or nss?
If I get things right,
`contrib/ci/configure.sh`: ``` if [[ "$DISTRO_BRANCH" == -redhat-fedora-29* || "$DISTRO_BRANCH" == -redhat-fedora-3* || "$DISTRO_BRANCH" == -debian-* || "$DISTRO_BRANCH" == -redhat-redhatenterprise*-8.*- || "$DISTRO_BRANCH" == -redhat-centos-8.*- ]]; then CONFIGURE_ARG_LIST+=( "--with-crypto=libcrypto" ) fi ```
And for other targets it is nss by default. """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-502133590
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
mzidek-rh commented: """ Ok, @alexey-tikhonov (thanks) pointed out that libcrypto is used in never fedoras for PR CI as well. And since we already run them, I think it is good enough.
Ack.
Thanks. """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-502133953
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
jhrozek commented: """ master: 6839e67 """
See the full comment at https://github.com/SSSD/sssd/pull/826#issuecomment-502638236
URL: https://github.com/SSSD/sssd/pull/826 Author: alexey-tikhonov Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1() Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/826/head:pr826 git checkout pr826
URL: https://github.com/SSSD/sssd/pull/826 Title: #826: util/crypto/libcrypto: changed sss_hmac_sha1()
Label: +Pushed
sssd-devel@lists.fedorahosted.org