[SSSD] [PATCH] Use custom implementation of glibc extension

Lukas Slebodnik lslebodn at redhat.com
Tue Jun 3 10:59:00 UTC 2014


On (03/06/14 12:22), Pavel Březina wrote:
>On 05/31/2014 04:04 PM, Lukas Slebodnik wrote:
>>On (27/05/14 10:23), Jakub Hrozek wrote:
>>>On Tue, May 27, 2014 at 10:08:02AM +0200, Sumit Bose wrote:
>>>>On Tue, May 27, 2014 at 09:36:29AM +0200, Lukas Slebodnik wrote:
>>>>>On (27/05/14 09:27), Pavel Reichl wrote:
>>>>>>On Tue, 2014-05-27 at 09:17 +0200, Lukas Slebodnik wrote:
>>>>>>>On (27/05/14 09:11), Pavel Reichl wrote:
>>>>>>>>On Tue, 2014-05-27 at 00:19 +0200, Lukas Slebodnik wrote:
>>>>>>>>>ehlo,
>>>>>>>>>
>>>>>>>>>strchrnul() and mempcpy() are GNU extensions to libc
>>>>>>>>>This patch add configure time test for this functions and provide
>>>>>>>>>custom implementation if functions are not available.
>>>>>>>>>
>>>>>>>>>patch is attached.
>>>>>>>>>
>>>>>>>>>LS
>>>>>>>>>_______________________________________________
>>>>>>>>>sssd-devel mailing list
>>>>>>>>>sssd-devel at lists.fedorahosted.org
>>>>>>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>>>>>
>>>>>>>>Hello Lukas,
>>>>>>>>
>>>>>>>>is it really necessary to have two definitions of
>>>>>>>>mempcpy()?
>>>>>>>Yes,
>>>>>>>
>>>>>>>libsss_crypt.so is built either with nss or with libcrypto.
>>>>>>>Look into Makefile.am for details.
>>>>>>>
>>>>>>>LS
>>>>>>>_______________________________________________
>>>>>>>sssd-devel mailing list
>>>>>>>sssd-devel at lists.fedorahosted.org
>>>>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>>>
>>>>>>Yes, I'm aware of that. I was just thinking if you could move the
>>>>>>definition into separate file and thus avoid code duplication...
>>>>>>
>>>>>No, there will be no benefit.
>>>>>There isn't any shared file between two implementation of libsss_crypt.
>>>>>static function can be easily inlined and I don't want to have conditional
>>>>>definition of function in header file.
>>>>
>>>>I wonder if using a "library" like gnulib might be useful in the long
>>>>run to handle cases like this? I'm not saying we should do it now, but
>>Problematic functions are in old stable code and only in one module.
>>SSSD has a lot of other dependencies I do not want to add another one.
>>
>>>>since you are currently looking at glibc, XOPEN and other extension I
>>>>wonder what's your opinion here?
>>macro XOPEN is not related to these functions. It is different story :-)
>>
>>>>
>>>>bye,
>>>>Sumit
>>>
>>>Yes, I prefer using code that was tested by others as well.
>>>
>>>Also, unlike the includes pushed yesterday, I would prefer either a test
>>>for these functions
>>The function strchrnul is used in confdb function parse_section
>>and it is indirectly tested in test_utils, simple_access-tests, sysdb-tests,
>>sysdb_ssh-tests
>>
>>The function mempcpy is used in computation of has sha512.
>>I added new test for function s3crypt_sha512.
>>
>>Thess passes with:
>>      --with-crypto=nss
>>      --with-crypto=libcrypto
>>      with custom implementation of mempcpy
>>
>>The last one can be tested with next diff:
>>diff --git a/src/util/crypto/nss/nss_sha512crypt.c b/src/util/crypto/nss/nss_sha512crypt.c
>>index 11194e5..54f6cbd 100644
>>--- a/src/util/crypto/nss/nss_sha512crypt.c
>>+++ b/src/util/crypto/nss/nss_sha512crypt.c
>>@@ -29,11 +29,11 @@
>>  #include <sechash.h>
>>  #include <pk11func.h>
>>
>>-#ifndef HAVE_MEMPCPY
>>+#undef mempcpy
>>+#define mempcpy mymempcpy
>>  static void * mempcpy(void *dst, const void *src, size_t len) {
>>      return (void*) (((char*)memcpy(dst, src, len)) + len);
>>  }
>>-#endif /* HAVE_MEMPCPY */
>>
>>  /* Define our magic string to mark salt for SHA512 "encryption" replacement. */
>>  const char sha512_salt_prefix[] = "$6$";
>>
>>LS
>
>Can you also add a memory leak check? See test_encrypt_decrypt.
sure
memory leak check was added.

updated version attached.

LS
-------------- next part --------------
>From 3a2edabed0fbdeb132618b8440cd499a55d8f456 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Mon, 26 May 2014 23:24:54 +0200
Subject: [PATCH 1/2] Use custom implementation of glibc extension.

strchrnul() and mempcpy() are GNU extensions to libc
This patch add configure time test for this functions and provide
custom implementation if functions are not available.
---
 configure.ac                                   | 2 ++
 src/confdb/confdb.c                            | 7 +++++++
 src/util/crypto/libcrypto/crypto_sha512crypt.c | 6 ++++++
 src/util/crypto/nss/nss_sha512crypt.c          | 6 ++++++
 4 files changed, 21 insertions(+)

diff --git a/configure.ac b/configure.ac
index d603cda1ab7e18933bb51d3e69a8d434351c08ad..19cd7d3f8cd9f99367e1e7b738f988d382fc8305 100644
--- a/configure.ac
+++ b/configure.ac
@@ -42,6 +42,8 @@ AC_CONFIG_HEADER(config.h)
 
 AC_CHECK_TYPES([errno_t], [], [], [[#include <errno.h>]])
 
+AC_CHECK_FUNCS([mempcpy strchrnul])
+
 m4_include([src/build_macros.m4])
 BUILD_WITH_SHARED_BUILD_DIR
 
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de9616ffdddf640f29e6b816c246ee05da48c7..f4ec1d3d69bf66f444d53d2bd5e205d907520197 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -28,6 +28,13 @@
 #include "util/strtonum.h"
 #include "db/sysdb.h"
 
+#ifndef HAVE_STRCHRNUL
+static char *strchrnul(const char *s, int c) {
+    char *ret = strchr(s, c);
+    return ret == NULL ? discard_const_p(char, s) + strlen(s) : ret;
+}
+#endif /* HAVE_STRCHRNUL */
+
 #define CONFDB_ZERO_CHECK_OR_JUMP(var, ret, err, label) do { \
     if (!var) { \
         ret = err; \
diff --git a/src/util/crypto/libcrypto/crypto_sha512crypt.c b/src/util/crypto/libcrypto/crypto_sha512crypt.c
index 34547d08abdc3cb3ea946291bc82120c2565fd49..26507f70fa6b9f168fdddff24d89094b7758115d 100644
--- a/src/util/crypto/libcrypto/crypto_sha512crypt.c
+++ b/src/util/crypto/libcrypto/crypto_sha512crypt.c
@@ -28,6 +28,12 @@
 #include <openssl/evp.h>
 #include <openssl/rand.h>
 
+#ifndef HAVE_MEMPCPY
+static void * mempcpy(void *dst, const void *src, size_t len) {
+    return (void*) (((char*)memcpy(dst, src, len)) + len);
+}
+#endif /* HAVE_MEMPCPY */
+
 /* Define our magic string to mark salt for SHA512 "encryption" replacement. */
 const char sha512_salt_prefix[] = "$6$";
 #define SALT_PREF_SIZE (sizeof(sha512_salt_prefix) - 1)
diff --git a/src/util/crypto/nss/nss_sha512crypt.c b/src/util/crypto/nss/nss_sha512crypt.c
index 9fedd5ec6c62855d9cc0c9c2869d8c9be7fb5ade..11194e5d1cc68c89972262a165fe05363ee1afdb 100644
--- a/src/util/crypto/nss/nss_sha512crypt.c
+++ b/src/util/crypto/nss/nss_sha512crypt.c
@@ -29,6 +29,12 @@
 #include <sechash.h>
 #include <pk11func.h>
 
+#ifndef HAVE_MEMPCPY
+static void * mempcpy(void *dst, const void *src, size_t len) {
+    return (void*) (((char*)memcpy(dst, src, len)) + len);
+}
+#endif /* HAVE_MEMPCPY */
+
 /* Define our magic string to mark salt for SHA512 "encryption" replacement. */
 const char sha512_salt_prefix[] = "$6$";
 #define SALT_PREF_SIZE (sizeof(sha512_salt_prefix) - 1)
-- 
1.9.3

-------------- next part --------------
>From a26c38096e74efdab48a159e34f288d356b5aac5 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 31 May 2014 15:01:12 +0200
Subject: [PATCH 2/2] TESTS: Add test for s3crypt_sha512

---
 src/tests/crypto-tests.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/src/tests/crypto-tests.c b/src/tests/crypto-tests.c
index 87184594e2fb7cedc234350b56aa341d9de62713..1f145bfb05a1d4c551a5ff3839aaa5dc0aa73e53 100644
--- a/src/tests/crypto-tests.c
+++ b/src/tests/crypto-tests.c
@@ -122,6 +122,83 @@ START_TEST(test_hmac_sha1)
 }
 END_TEST
 
+#define SALT1 "super.salt"
+#define SALT2 "Loremipsumdolors"
+#define SALT3 "$6$rounds=10000$Loremipsumdolors$"
+
+#define PREFIX1 "$6$"SALT1"$"
+#define PREFIX2 "$6$"SALT2"$"
+#define PREFIX3 SALT3
+
+START_TEST(test_crypt_sha512)
+{
+    const char *message[] = {
+        "short",
+        "proper6789012345678901234567890123456789012345678901234567890123",
+        "longlonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglong",
+        NULL };
+    const char *results_salt1[] = {
+        PREFIX1"EHCvLEO9BgT8ObsacnLX6cn8.g.KWQdkoE2QGRGHQuC2qSY7pj47SlsiWtsi2"
+               "SL0GB25rgeZfaIrVWeiMdPK..",
+        PREFIX1"ZeJDjANbC3bAD0KkHdbrcjXGudhAF7VtxE6YVIDtoDQR8aBT4ixYbunZ1Pbxx"
+               "ejkapQYqEu4rvJrPybbvo1/6.",
+        PREFIX1"0iY1uLvQBdYJs4CnZaTcALpZ3B2Bc2jT3WEmxLrE89mAQfgGBlog9aG/Y9/ry"
+               "QroLEDutCtpiJyHBsf.yJ/uP1",
+        NULL };
+    const char *results_salt2[] = {
+        PREFIX2"Mpwr..dRHfhEt8VQ6EiU2/o75agqGFrxxaqARbb6I9QbsEv7WoEesyYKo35C."
+               "ZRviQIbIsm/kiTVlTotKJm5e/",
+        PREFIX2"/I1VnfwFWUDMhG5s.NEs2Kq2dNjDDXyjmhogfWJaHEAkRv.E.SP4p5lfIdYWq"
+               "b39GBIJGffSSno1DLX.NP9Ed0",
+        PREFIX2"DY9R5CD3QQt9UwoduerLHtjHC0YezO/v37.O0qqqXr4cTwoQHUaXlR7oKdet3"
+               "DfWPAxsTys0Nkwl65gU./99v0",
+        NULL };
+    const char *results_salt3[] = {
+        PREFIX3"5vkWlXWA/fJT0GVEo8GH4H/UaPnV2iSUjZu06kKOXyMBqaTrQ9CCLboOuyFYl"
+               "RX6N3M.RVPsoc50PbM8OCXnc.",
+        PREFIX3"v1SUGuleIb6RMnacA4x9ro3x8vbPCaO9uCy8gboNU2gT3j.LphSAT4a7hbJ2I"
+               "/VVp/EEWrjiZb8BIhrJ4rmmq1",
+        PREFIX3"uBqJ/JfPZYiuqaGCJcSxgBlvWJjg5LvSDiWGIYO4ijNuYPC9Xn0A9BVnRYpW0"
+               "32S3V07X4Q1b5g73kCjOGABG/",
+        NULL };
+    char *hash = NULL;
+    int ret;
+    int i;
+    TALLOC_CTX *tmp_ctx;
+
+    tmp_ctx = talloc_new(NULL);
+    fail_if(tmp_ctx == NULL);
+    ck_leaks_push(tmp_ctx);
+
+    for (i = 0; message[i]; i++) {
+        ret = s3crypt_sha512(tmp_ctx, message[i], SALT1, &hash);
+        fail_if(ret != EOK);
+        fail_if(hash == NULL);
+        fail_if(strcmp(hash, results_salt1[i]) != 0);
+
+        talloc_zfree(hash);
+
+        ret = s3crypt_sha512(tmp_ctx, message[i], SALT2, &hash);
+        fail_if(ret != EOK);
+        fail_if(hash == NULL);
+        fail_if(strcmp(hash, results_salt2[i]) != 0);
+
+        talloc_zfree(hash);
+
+        ret = s3crypt_sha512(tmp_ctx, message[i], SALT3, &hash);
+        fail_if(ret != EOK);
+        fail_if(hash == NULL);
+        fail_if(strcmp(hash, results_salt3[i]) != 0);
+
+        talloc_zfree(hash);
+    }
+
+    ck_leaks_pop(tmp_ctx);
+    talloc_free(tmp_ctx);
+}
+END_TEST
+
+
 START_TEST(test_base64_encode)
 {
     const unsigned char obfbuf[] = "test";
@@ -170,6 +247,7 @@ Suite *crypto_suite(void)
 #endif
     tcase_add_test(tc, test_encrypt_decrypt);
     tcase_add_test(tc, test_hmac_sha1);
+    tcase_add_test(tc, test_crypt_sha512);
     tcase_add_test(tc, test_base64_encode);
     tcase_add_test(tc, test_base64_decode);
     /* Add all test cases to the test suite */
-- 
1.9.3



More information about the sssd-devel mailing list