ldb-samba: ldif-handlers: make ldif_comparison_objectSid() accurate

This function compares blobs that might be SID strings or might be SID
structures. Until now, if they were both (seemingly) strings, they were
compared as strings, otherwise if either was a string it was converted to
a structure blob, then the blobs were compared. This had two big problems:

1. There is variety in the way a SID can be stringified. For example,
   "s-1-02-3" means the same SID as "S-1-2-3", but those wouldn't compare
   equal.

2. SID comparison was crazily non-transitive. Consider the three values
     a = "S-1-2-3-4-5",
     b = "S-1-9-1",
     c = SID("S-1-11-1"), where c is a struct and the others are string.

   then we had,
     a < b, because the 5th character '2' < '9'.
     a > c, because when converted to a structure, the number of sub-auths
            is the first varying byte. a has 3, c has 0.
     b < c, because after the sub-auth count comes the id_auth value
            (big-endian, which doesn't matter in this case).

That made the function unreliable for sorting, AND for simple equality
tests. Also it leaked.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c
index c30fd6358c88706e018edea87c79754e7bae4672..8873ee6e4e64df5fbfef3f877d38396adaf8b968 100644 (file)
--- a/lib/ldb-samba/ldif_handlers.c
+++ b/lib/ldb-samba/ldif_handlers.c
@@ -150,36 +150,47 @@ bool ldif_comparision_objectSid_isString(const struct ldb_val *v)
 
 /*
   compare two objectSids
+
+  If the SIDs seem to be strings, they are converted to binary form.
 */
 static int ldif_comparison_objectSid(struct ldb_context *ldb, void *mem_ctx,
                                    const struct ldb_val *v1, const struct ldb_val *v2)
 {
-       if (ldif_comparision_objectSid_isString(v1) && ldif_comparision_objectSid_isString(v2)) {
-               return ldb_comparison_binary(ldb, mem_ctx, v1, v2);
-       } else if (ldif_comparision_objectSid_isString(v1)
-                  && !ldif_comparision_objectSid_isString(v2)) {
-               struct ldb_val v;
-               int ret;
-               if (ldif_read_objectSid(ldb, mem_ctx, v1, &v) != 0) {
-                       /* Perhaps not a string after all */
-                       return ldb_comparison_binary(ldb, mem_ctx, v1, v2);
+       bool v1_is_string = ldif_comparision_objectSid_isString(v1);
+       bool v2_is_string = ldif_comparision_objectSid_isString(v2);
+       struct ldb_val parsed_1 = {};
+       struct ldb_val parsed_2 = {};
+       int ret;
+       /*
+        * If the ldb_vals look like SID strings (i.e. start with "S-"
+        * or "s-"), we try to parse them as such. If that fails, we
+        * assume they are binary SIDs, even though that's not really
+        * possible -- the first two bytes of a struct dom_sid are the
+        * version (1), and the number of sub-auths (<= 15), neither
+        * of which are close to 'S' or '-'.
+        */
+       if (v1_is_string) {
+               int r = ldif_read_objectSid(ldb, mem_ctx, v1, &parsed_1);
+               if (r == 0) {
+                       v1 = &parsed_1;
                }
-               ret = ldb_comparison_binary(ldb, mem_ctx, &v, v2);
-               talloc_free(v.data);
-               return ret;
-       } else if (!ldif_comparision_objectSid_isString(v1)
-                  && ldif_comparision_objectSid_isString(v2)) {
-               struct ldb_val v;
-               int ret;
-               if (ldif_read_objectSid(ldb, mem_ctx, v2, &v) != 0) {
-                       /* Perhaps not a string after all */
-                       return ldb_comparison_binary(ldb, mem_ctx, v1, v2);
+       }
+       if (v2_is_string) {
+               int r = ldif_read_objectSid(ldb, mem_ctx, v2, &parsed_2);
+               if (r == 0) {
+                       v2 = &parsed_2;
                }
-               ret = ldb_comparison_binary(ldb, mem_ctx, v1, &v);
-               talloc_free(v.data);
-               return ret;
        }
-       return ldb_comparison_binary(ldb, mem_ctx, v1, v2);
+
+       ret = ldb_comparison_binary(ldb, mem_ctx, v1, v2);
+
+       if (v1_is_string) {
+               TALLOC_FREE(parsed_1.data);
+       }
+       if (v2_is_string) {
+               TALLOC_FREE(parsed_2.data);
+       }
+       return ret;
 }
 
 /*