librpc/ndr/uuid.c: improve speed and accuracy of GUID string parsing

GUID_from_data_blob() was relying on sscanf to parse strings, which was
slow and quite accepting of invalid GUIDs. Instead we directly read a
fixed number of hex bytes for each field.

This now passes the samba4.local.ndr.*.guid_from_string_invalid tests.

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Autobuild-User(master): Douglas Bagnall <dbagnall@samba.org>
Autobuild-Date(master): Wed Dec 14 08:55:42 CET 2016 on sn-devel-144

diff --git a/librpc/ndr/uuid.c b/librpc/ndr/uuid.c
index a3f68d1d344bea0dfc12b2b7ad151a8fe5e393a4..037dd909fce3e83c8de92334f433ed152a4b8889 100644 (file)
--- a/librpc/ndr/uuid.c
+++ b/librpc/ndr/uuid.c
@@ -52,6 +52,102 @@ _PUBLIC_ NTSTATUS GUID_from_ndr_blob(const DATA_BLOB *b, struct GUID *guid)
        return ndr_map_error2ntstatus(ndr_err);
 }
 
+static NTSTATUS read_hex_bytes(const char *s, uint hexchars, uint64_t *dest)
+{
+       uint64_t x = 0;
+       uint i;
+       char c;
+
+       if ((hexchars & 1) || hexchars > 16) {
+               return NT_STATUS_INVALID_PARAMETER;
+       }
+
+       for (i = 0; i < hexchars; i++) {
+               x <<= 4;
+               c = s[i];
+               if (c >= '0' && c <= '9') {
+                       x += c - '0';
+               }
+               else if (c >= 'a' && c <= 'f') {
+                       x += c - 'a' + 10;
+               }
+               else if (c >= 'A' && c <= 'F') {
+                       x += c - 'A' + 10;
+               }
+               else {
+                       /* BAD character (including '\0') */
+                       return NT_STATUS_INVALID_PARAMETER;
+               }
+       }
+       *dest = x;
+       return NT_STATUS_OK;
+}
+
+
+static NTSTATUS parse_guid_string(const char *s,
+                                 uint32_t *time_low,
+                                 uint32_t *time_mid,
+                                 uint32_t *time_hi_and_version,
+                                 uint32_t *clock_seq,
+                                 uint32_t *node)
+{
+       uint64_t tmp;
+       NTSTATUS status;
+       int i;
+       /* "e12b56b6-0a95-11d1-adbb-00c04fd8d5cd"
+                |     |    |    |    |
+                |     |    |    |    \ node[6]
+                |     |    |    \_____ clock_seq[2]
+                |     |    \__________ time_hi_and_version
+               |     \_______________ time_mid
+               \_____________________ time_low
+       */
+       status = read_hex_bytes(s, 8, &tmp);
+       if (!NT_STATUS_IS_OK(status) || s[8] != '-') {
+               return NT_STATUS_INVALID_PARAMETER;
+       }
+       *time_low = tmp;
+       s += 9;
+
+       status = read_hex_bytes(s, 4, &tmp);
+       if (!NT_STATUS_IS_OK(status) || s[4] != '-') {
+               return NT_STATUS_INVALID_PARAMETER;
+       }
+       *time_mid = tmp;
+       s += 5;
+
+       status = read_hex_bytes(s, 4, &tmp);
+       if (!NT_STATUS_IS_OK(status) || s[4] != '-') {
+               return NT_STATUS_INVALID_PARAMETER;
+       }
+       *time_hi_and_version = tmp;
+       s += 5;
+
+       for (i = 0; i < 2; i++) {
+               status = read_hex_bytes(s, 2, &tmp);
+               if (!NT_STATUS_IS_OK(status)) {
+                       return NT_STATUS_INVALID_PARAMETER;
+               }
+               clock_seq[i] = tmp;
+               s += 2;
+       }
+       if (s[0] != '-') {
+               return NT_STATUS_INVALID_PARAMETER;
+       }
+
+       s++;
+
+       for (i = 0; i < 6; i++) {
+               status = read_hex_bytes(s, 2, &tmp);
+               if (!NT_STATUS_IS_OK(status)) {
+                       return NT_STATUS_INVALID_PARAMETER;
+               }
+               node[i] = tmp;
+               s += 2;
+       }
+
+       return NT_STATUS_OK;
+}
 
 /**
   build a GUID from a string
@@ -75,32 +171,26 @@ _PUBLIC_ NTSTATUS GUID_from_data_blob(const DATA_BLOB *s, struct GUID *guid)
        switch(s->length) {
        case 36:
        {
-               char string[37];
-               memcpy(string, s->data, 36);
-               string[36] = 0;
-
-               if (11 == sscanf(string,
-                                "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
-                                &time_low, &time_mid, &time_hi_and_version, 
-                                &clock_seq[0], &clock_seq[1],
-                                &node[0], &node[1], &node[2], &node[3], &node[4], &node[5])) {
-                       status = NT_STATUS_OK;
-               }
+               status = parse_guid_string((char *)s->data,
+                                          &time_low,
+                                          &time_mid,
+                                          &time_hi_and_version,
+                                          clock_seq,
+                                          node);
                break;
        }
        case 38:
        {
-               char string[39];
-               memcpy(string, s->data, 38);
-               string[38] = 0;
-
-               if (11 == sscanf(string, 
-                                "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}",
-                                &time_low, &time_mid, &time_hi_and_version, 
-                                &clock_seq[0], &clock_seq[1],
-                                &node[0], &node[1], &node[2], &node[3], &node[4], &node[5])) {
-                       status = NT_STATUS_OK;
+               if (s->data[0] != '{' || s->data[37] != '}') {
+                       break;
                }
+
+               status = parse_guid_string((char *)s->data + 1,
+                                          &time_low,
+                                          &time_mid,
+                                          &time_hi_and_version,
+                                          clock_seq,
+                                          node);
                break;
        }
        case 32:

diff --git a/selftest/knownfail b/selftest/knownfail
index f6493818468ce136e216060b9f107cb473f37c21..97ec6ef1d64df46b6cbf2fead46f10bda1cb3831 100644 (file)
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -309,4 +309,3 @@
 ^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_add_duplicate_different_type.*
 ^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_rank_none.*
 ^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_security_descriptor.*
-^samba4.local.ndr.*.guid_from_string_invalid