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

Lukas Slebodnik lslebodn at redhat.com
Sat May 31 14:04:04 UTC 2014


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
-------------- next part --------------
>From 681a53b43da4635b2dafed083f6853828f575f42 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 6f36535fdb27c0b987968cf440c1dccb63f250d9..351959f76b25cd4216d3123cd8bb9a21fb34fc95 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 7a324ccafc26d3b477fc776b2212335df2ef6f57 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 | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/src/tests/crypto-tests.c b/src/tests/crypto-tests.c
index 87184594e2fb7cedc234350b56aa341d9de62713..3f6021097d3597a5c2bb10123dcdc1bd5532ce00 100644
--- a/src/tests/crypto-tests.c
+++ b/src/tests/crypto-tests.c
@@ -122,6 +122,81 @@ 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);
+
+    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);
+    }
+
+    talloc_free(tmp_ctx);
+}
+END_TEST
+
+
 START_TEST(test_base64_encode)
 {
     const unsigned char obfbuf[] = "test";
@@ -170,6 +245,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