URL: https://github.com/SSSD/sssd/pull/503
Author: lslebodn
 Title: #503: Regression test for false possitive "corrupted" memory cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/503/head:pr503
git checkout pr503
From 313af382da04ccb4d021a4325f0797ca2742f081 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Wed, 24 Jan 2018 11:24:01 +0100
Subject: [PATCH 1/4] pysss_murmur: Allow to have NUL character in python
 bindings

---
 src/python/pysss_murmur.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/python/pysss_murmur.c b/src/python/pysss_murmur.c
index 060d29df3..8f1752a29 100644
--- a/src/python/pysss_murmur.c
+++ b/src/python/pysss_murmur.c
@@ -38,15 +38,16 @@ static PyObject * py_murmurhash3(PyObject *module, PyObject *args)
     long key_len;
     long long seed;
     uint32_t hash;
+    int input_len;
 
-    if (!PyArg_ParseTuple(args, sss_py_const_p(char, "slL"),
-                          &key, &key_len, &seed)) {
+    if (!PyArg_ParseTuple(args, sss_py_const_p(char, "z#lL"),
+                          &key, &input_len, &key_len, &seed)) {
         PyErr_Format(PyExc_ValueError, "Invalid argument\n");
         return NULL;
     }
 
     if (seed > UINT32_MAX || key_len > INT_MAX || key_len < 0 ||
-        (size_t)key_len > strlen(key)) {
+        (size_t)key_len > input_len) {
         PyErr_Format(PyExc_ValueError, "Invalid value\n");
         return NULL;
     }

From fac978156129ce13576a3494fb456086e4ce68f3 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Wed, 24 Jan 2018 11:34:59 +0100
Subject: [PATCH 2/4] TESTS: Extend code coverage for murmurhash3

* add positive test for trailing NUL character
* add test for corner cases (0, input_len + 1)
---
 src/tests/pysss_murmur-test.py | 62 ++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/src/tests/pysss_murmur-test.py b/src/tests/pysss_murmur-test.py
index 7237c95b0..9fb1a0494 100755
--- a/src/tests/pysss_murmur-test.py
+++ b/src/tests/pysss_murmur-test.py
@@ -60,26 +60,53 @@ def testImport(self):
             raise e
         self.assertEqual(pysss_murmur.__file__, MODPATH + "/pysss_murmur.so")
 
-class PySssMurmurTest(unittest.TestCase):
-    @classmethod
-    def tearDownClass(cls):
-        os.unlink(MODPATH + "/pysss_murmur.so")
-        os.rmdir(MODPATH)
 
-    def testExpectedHash(self):
-        hash = pysss_murmur.murmurhash3("S-1-5-21-2153326666-2176343378-3404031434", 41, 0xdeadbeef)
-        self.assertEqual(hash, 93103853)
-
-    def testInvalidArguments(self):
+class PySssMurmurTestNeg(unittest.TestCase):
+    def test_invalid_arguments(self):
         self.assertRaises(ValueError, pysss_murmur.murmurhash3, 1, 2, 3)
         self.assertRaises(ValueError, pysss_murmur.murmurhash3, "test", 2)
         self.assertRaises(ValueError, pysss_murmur.murmurhash3, "test")
         self.assertRaises(ValueError, pysss_murmur.murmurhash3)
-        self.assertRaises(ValueError, pysss_murmur.murmurhash3, "test", -1, 3)
-        self.assertRaises(ValueError, pysss_murmur.murmurhash3, "test", 2,
-                          0xffffffffff)
+
+    def test_invalid_length(self):
+        seed = 12345
+
+        self.assertRaises(ValueError, pysss_murmur.murmurhash3, "t", -1, seed)
+        # length is off by one
+        self.assertRaises(ValueError, pysss_murmur.murmurhash3, "test", 5,
+                          seed)
         self.assertRaises(ValueError, pysss_murmur.murmurhash3, "test",
-                          0xffffffffff, 3)
+                          0xffffffffff, seed)
+
+
+class PySssMurmurTestPos(unittest.TestCase):
+    @classmethod
+    def tear_down_dlass(cls):
+        os.unlink(MODPATH + "/pysss_murmur.so")
+        os.rmdir(MODPATH)
+
+    def testExpectedHash(self):
+        sid_str = "S-1-5-21-2153326666-2176343378-3404031434"
+        seed = 0xdeadbeef
+
+        hash_val = pysss_murmur.murmurhash3(sid_str, 0, seed)
+        self.assertEqual(hash_val, 233162409)
+
+        hash_val = pysss_murmur.murmurhash3(sid_str, len(sid_str), seed)
+        self.assertEqual(hash_val, 93103853)
+
+    def test_memory_cache_usage(self):
+        seed = 0xbeefdead
+        input_str = "test_user1"
+        input_len = len(input_str)
+
+        val_bin = pysss_murmur.murmurhash3(input_str + '\0',
+                                           input_len + 1, seed)
+        self.assertEqual(val_bin, 1198610880)
+
+        val_bin = pysss_murmur.murmurhash3(input_str + '\0' * 5,
+                                           input_len + 5, seed)
+        self.assertEqual(val_bin, 2917868047)
 
 
 if __name__ == "__main__":
@@ -97,9 +124,14 @@ def testInvalidArguments(self):
     sys.path.insert(0, MODPATH)
     import pysss_murmur
 
-    suite = unittest.TestLoader().loadTestsFromTestCase(PySssMurmurTest)
+    suite = unittest.TestLoader().loadTestsFromTestCase(PySssMurmurTestNeg)
     res = unittest.TextTestRunner().run(suite)
     if not res.wasSuccessful():
         error |= 0x2
 
+    suite = unittest.TestLoader().loadTestsFromTestCase(PySssMurmurTestPos)
+    res = unittest.TextTestRunner().run(suite)
+    if not res.wasSuccessful():
+        error |= 0x4
+
     sys.exit(error)

From a53590ef7cc4e4107447e50a6e62c8b603d4aa55 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Wed, 24 Jan 2018 13:44:09 +0100
Subject: [PATCH 3/4] mmap_cache: Remove unnecessary memchr in client code

Corrupting of memory cache was fixed in the ticket
https://pagure.io/SSSD/sssd/issue/2049.

We have some fast sanity check for length of strings and memchr is not
used in responder. We should remove it also from client code.

Related to:
https://pagure.io/SSSD/sssd/issue/3592
---
 src/sss_client/nss_mc_group.c  | 4 +---
 src/sss_client/nss_mc_initgr.c | 4 +---
 src/sss_client/nss_mc_passwd.c | 4 +---
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index 422c25efc..6a2336b61 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -157,9 +157,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
         if (data->name < strs_offset
             || data->name >= strs_offset + data->strs_len
             || data->strs_len > rec->len
-            || (uint8_t *) rec + rec->len > gr_mc_ctx.data_table + data_size
-            || memchr(rec_name, '\0',
-                      (strs_offset + data->strs_len) - data->name) == NULL) {
+            || (uint8_t *) rec + rec->len > gr_mc_ctx.data_table + data_size) {
             ret = ENOENT;
             goto done;
         }
diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c
index 82cbcf702..5a8c661c7 100644
--- a/src/sss_client/nss_mc_initgr.c
+++ b/src/sss_client/nss_mc_initgr.c
@@ -141,9 +141,7 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,
             || data->strs_len > data->data_len
             || data->data_len > rec->len
             || (uint8_t *) rec + rec->len
-                                       > initgr_mc_ctx.data_table + data_size
-            || memchr(rec_name, '\0',
-                      (data_offset + data->data_len) - data->name) == NULL) {
+                                      > initgr_mc_ctx.data_table + data_size) {
             ret = ENOENT;
             goto done;
         }
diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c
index 5853e1227..3c6248177 100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -150,9 +150,7 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,
         if (data->name < strs_offset
             || data->name >= strs_offset + data->strs_len
             || data->strs_len > rec->len
-            || (uint8_t *) rec + rec->len > pw_mc_ctx.data_table + data_size
-            || memchr(rec_name, '\0',
-                      (strs_offset + data->strs_len) - data->name) == NULL ) {
+            || (uint8_t *) rec + rec->len > pw_mc_ctx.data_table + data_size) {
             ret = ENOENT;
             goto done;
         }

From b2426ef9baf4e13f9ba02e3b33b54a59e3f9dd01 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Wed, 24 Jan 2018 12:19:38 +0100
Subject: [PATCH 4/4] test_memory_cache: Regression test for #3571

Resolves:
https://pagure.io/SSSD/sssd/issue/3592
---
 src/tests/intg/test_memory_cache.py | 81 +++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/src/tests/intg/test_memory_cache.py b/src/tests/intg/test_memory_cache.py
index cac9feb00..0d1327336 100644
--- a/src/tests/intg/test_memory_cache.py
+++ b/src/tests/intg/test_memory_cache.py
@@ -22,10 +22,15 @@
 import grp
 import pwd
 import config
+import random
 import signal
+import string
+import struct
 import subprocess
 import time
 import pytest
+import pysss_murmur
+
 import ds_openldap
 import ldap_ent
 import sssd_id
@@ -770,6 +775,82 @@ def test_invalidate_everything_after_stop(ldap_conn, sanity_rfc2307):
     assert_missing_mc_records_for_user1()
 
 
+def get_random_string(length):
+    return ''.join([random.choice(string.ascii_letters + string.digits)
+                    for n in range(length)])
+
+
+class MemoryCache(object):
+    SIZEOF_UINT32_T = 4
+
+    def __init__(self, path):
+        with open(path, "rb") as fin:
+            fin.seek(4 * self.SIZEOF_UINT32_T)
+            self.seed = struct.unpack('i', fin.read(4))[0]
+            self.data_size = struct.unpack('i', fin.read(4))[0]
+            self.ft_size = struct.unpack('i', fin.read(4))[0]
+            hash_len = struct.unpack('i', fin.read(4))[0]
+            self.hash_size = hash_len / self.SIZEOF_UINT32_T
+
+    def sss_nss_mc_hash(self, key):
+        input_key = key + '\0'
+        input_len = len(key) + 1
+
+        murmur_hash = pysss_murmur.murmurhash3(input_key, input_len, self.seed)
+        return murmur_hash % self.hash_size
+
+
+def test_colliding_hashes(ldap_conn, sanity_rfc2307):
+    """
+    Regression test for ticket:
+    https://pagure.io/SSSD/sssd/issue/3571
+    """
+
+    first_user = 'user1'
+
+    # initialize data in memcache
+    ent.assert_passwd_by_name(
+        first_user,
+        dict(name='user1', passwd='*', uid=1001, gid=2001,
+             gecos='1001', shell='/bin/bash'))
+
+    mem_cache = MemoryCache(config.MCACHE_PATH + '/passwd')
+
+    colliding_hash = mem_cache.sss_nss_mc_hash(first_user)
+
+    while True:
+        # string for colliding hash need to be longer then data for user1
+        # stored in memory cache (almost equivalent to:
+        #   `getent passwd user1 | wc -c` ==> 45
+        second_user = get_random_string(80)
+        val = mem_cache.sss_nss_mc_hash(second_user)
+        if val == colliding_hash:
+            break
+
+    # add new user to LDAP
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+    ent_list.add_user(second_user, 5001, 5001)
+    ldap_conn.add_s(ent_list[0][0], ent_list[0][1])
+
+    ent.assert_passwd_by_name(
+        second_user,
+        dict(name=second_user, passwd='*', uid=5001, gid=5001,
+             gecos='5001', shell='/bin/bash'))
+
+    stop_sssd()
+
+    # check that both users are stored in cache
+    ent.assert_passwd_by_name(
+        first_user,
+        dict(name='user1', passwd='*', uid=1001, gid=2001,
+             gecos='1001', shell='/bin/bash'))
+
+    ent.assert_passwd_by_name(
+        second_user,
+        dict(name=second_user, passwd='*', uid=5001, gid=5001,
+             gecos='5001', shell='/bin/bash'))
+
+
 def test_removed_mc(ldap_conn, sanity_rfc2307):
     """
     Regression test for ticket:
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

Reply via email to