>From c089fa0d999d89f91617b20d5ecd74bbd50e36c6 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 7 Dec 2016 14:35:58 +1300 Subject: [PATCH 1/2] s4-torture: better, failing, tests for GUID_from_string These tests reveal that the current implementation accepts all kinds of invalid GUIDs. In particular, we fail on these ones: "00000001-0002-0003-0405--060708090a0" "-0000001-0002-0003-0405-060708090a0b" "-0000001-0002-0003-04-5-060708090a0b" "d0000001-0002-0003-0405-060708090a-b" "00000001- -2-0003-0405-060708090a0b" "00000001-0002-0003-0405- 060708090a0" "0x000001-0002-0003-0405-060708090a0b" "00000001-0x02-0x03-0405-060708090a0b" This test is added to selftest/knownfail. The test for valid string GUIDs is extended to test upper and mixed case GUIDs. Signed-off-by: Douglas Bagnall --- selftest/knownfail | 1 + source4/torture/ndr/ndr.c | 64 ++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/selftest/knownfail b/selftest/knownfail index da37827..0067d2e 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -310,3 +310,4 @@ ^samba4.drs.ridalloc_exop.python.*ridalloc_exop.DrsReplicaSyncTestCase.test_join_time_ridalloc ^samba4.drs.ridalloc_exop.python.*ridalloc_exop.DrsReplicaSyncTestCase.test_rid_set_dbcheck_after_seize ^samba4.drs.ridalloc_exop.python.*ridalloc_exop.DrsReplicaSyncTestCase.test_rid_set_dbcheck +^samba4.local.ndr.*.guid_from_string_invalid diff --git a/source4/torture/ndr/ndr.c b/source4/torture/ndr/ndr.c index 7d28eb0..d67585c 100644 --- a/source4/torture/ndr/ndr.c +++ b/source4/torture/ndr/ndr.c @@ -296,17 +296,61 @@ static bool test_guid_from_string_null(struct torture_context *tctx) static bool test_guid_from_string_invalid(struct torture_context *tctx) { struct GUID g1; - torture_assert_ntstatus_equal(tctx, NT_STATUS_INVALID_PARAMETER, - GUID_from_string("bla", &g1), - "parameter not invalid"); + bool failed = false; + int i; + const char *bad_guids[] = { + "bla", + "", + /* + "00000001-0002-0003-0405-060708090a0b", correct + */ + "00000001-0002-0003-0405-060708090a0b1", /* too long */ + "00000001-0002-0003-0405-060708090a0", /* too short */ + "00000001-0002-0003-0405--060708090a0", /* negative */ + "00000001-0002-0003--0405-060708090a0", /* negative */ + "-0000001-0002-0003-0405-060708090a0b", /* negative */ + "-0000001-0002-0003-04-5-060708090a0b", /* negative */ + "d0000001-0002-0003-0405-060708090a-b", /* negative */ + "00000001- -2-0003-0405-060708090a0b", /* negative, space */ + "00000001-0002-0003-0405- 060708090a0", /* whitespace */ + " 0000001-0002-0003--0405-060708090a0", /* whitespace */ + "00000001-0002-0003--0405-060708090a ", /* whitespace */ + "0000001-00002-0003-04050-60708090a0b", /* misshapen */ + "00000010-0002-0003-04050-60708090a0b", /* misshapen */ + "00000001-0002-0003-0405-0z0708090a0b", /* bad char */ + "00000001-00x2-0x03-0405-060708090a0b", /* bad char (00x) */ + "0x000001-0002-0003-0405-060708090a0b", /* 0x char */ + "00000001-0x02-0x03-0405-060708090a0b", /* 0x char */ + }; + + for (i = 0; i < ARRAY_SIZE(bad_guids); i++) { + NTSTATUS status = GUID_from_string(bad_guids[i], &g1); + if (! NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) { + torture_comment(tctx, "bad guid %s parsed as OK\n", + bad_guids[i]); + failed = true; + } + } + if (failed) { + torture_fail(tctx, "wrongly allowing invalid guids"); + } return true; } static bool test_guid_from_string(struct torture_context *tctx) { struct GUID g1, exp; + /* we are asserting all these guids are valid and equal */ + const char *guids[4] = { + "00000001-0002-0003-0405-060708090a0b", + "{00000001-0002-0003-0405-060708090a0b}", + "{00000001-0002-0003-0405-060708090a0B}", /* mixed */ + "00000001-0002-0003-0405-060708090A0B", /* upper */ + }; + int i; + torture_assert_ntstatus_ok(tctx, - GUID_from_string("00000001-0002-0003-0405-060708090a0b", &g1), + GUID_from_string(guids[0], &g1), "invalid return code"); exp.time_low = 1; exp.time_mid = 2; @@ -319,12 +363,14 @@ static bool test_guid_from_string(struct torture_context *tctx) exp.node[3] = 9; exp.node[4] = 10; exp.node[5] = 11; - torture_assert(tctx, GUID_equal(&g1, &exp), "UUID parsed incorrectly"); - torture_assert_ntstatus_ok(tctx, - GUID_from_string("{00000001-0002-0003-0405-060708090a0b}", &g1), - "invalid return code"); - torture_assert(tctx, GUID_equal(&g1, &exp), "UUID parsed incorrectly"); + for (i = 1; i < ARRAY_SIZE(guids); i++) { + torture_assert_ntstatus_ok(tctx, + GUID_from_string(guids[i], &g1), + "invalid return code"); + torture_assert(tctx, GUID_equal(&g1, &exp), + "UUID parsed incorrectly"); + } return true; } -- 2.7.4 >From fefd45f26d653c89eec8b326c5f05c24adc72d11 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 7 Dec 2016 11:54:41 +1300 Subject: [PATCH 2/2] 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 --- librpc/ndr/uuid.c | 134 ++++++++++++++++++++++++++++++++++++++++++++--------- selftest/knownfail | 1 - 2 files changed, 113 insertions(+), 22 deletions(-) diff --git a/librpc/ndr/uuid.c b/librpc/ndr/uuid.c index a3f68d1..6628aca 100644 --- a/librpc/ndr/uuid.c +++ b/librpc/ndr/uuid.c @@ -52,6 +52,104 @@ _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 +173,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 0067d2e..da37827 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -310,4 +310,3 @@ ^samba4.drs.ridalloc_exop.python.*ridalloc_exop.DrsReplicaSyncTestCase.test_join_time_ridalloc ^samba4.drs.ridalloc_exop.python.*ridalloc_exop.DrsReplicaSyncTestCase.test_rid_set_dbcheck_after_seize ^samba4.drs.ridalloc_exop.python.*ridalloc_exop.DrsReplicaSyncTestCase.test_rid_set_dbcheck -^samba4.local.ndr.*.guid_from_string_invalid -- 2.7.4