From 2a52262b36c74a15b1cd7e0d0aadd20d81c78f69 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 29 Apr 2020 16:16:26 +1200 Subject: [PATCH 1/7] CVE-2020-10745: ndr/dns: shift pull_dns_string to ndr_dns_util.c This will allow NBT to use the same function (after modifications in the next commit). BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns.c | 101 ++------------------------------- librpc/ndr/ndr_dns_utils.c | 111 +++++++++++++++++++++++++++++++++++++ librpc/ndr/ndr_dns_utils.h | 4 ++ 3 files changed, 119 insertions(+), 97 deletions(-) diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index 966e0b59786..6b0dbd2c067 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -35,8 +35,6 @@ #include "lib/util/util_net.h" #include "ndr_dns_utils.h" -/* don't allow an unlimited number of name components */ -#define MAX_COMPONENTS 128 /** print a dns string @@ -48,65 +46,6 @@ _PUBLIC_ void ndr_print_dns_string(struct ndr_print *ndr, ndr_print_string(ndr, name, s); } -/* - pull one component of a dns_string -*/ -static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, - uint8_t **component, - uint32_t *offset, - uint32_t *max_offset) -{ - uint8_t len; - unsigned int loops = 0; - while (loops < 5) { - if (*offset >= ndr->data_size) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, bad offset"); - } - len = ndr->data[*offset]; - if (len == 0) { - *offset += 1; - *max_offset = MAX(*max_offset, *offset); - *component = NULL; - return NDR_ERR_SUCCESS; - } - if ((len & 0xC0) == 0xC0) { - /* its a label pointer */ - if (1 + *offset >= ndr->data_size) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, " \ - "bad label offset"); - } - *max_offset = MAX(*max_offset, *offset + 2); - *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; - *max_offset = MAX(*max_offset, *offset); - loops++; - continue; - } - if ((len & 0xC0) != 0) { - /* its a reserved length field */ - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, " \ - "reserved length field: 0x%02x", - (len &0xC)); - } - if (*offset + len + 1 > ndr->data_size) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, "\ - "length too long"); - } - *component = (uint8_t*)talloc_strndup(ndr, - (const char *)&ndr->data[1 + *offset], len); - NDR_ERR_HAVE_NO_MEMORY(*component); - *offset += len + 1; - *max_offset = MAX(*max_offset, *offset); - return NDR_ERR_SUCCESS; - } - - /* too many pointers */ - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, too many pointers"); -} /** pull a dns_string from the wire @@ -115,44 +54,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_dns_string(struct ndr_pull *ndr, int ndr_flags, const char **s) { - uint32_t offset = ndr->offset; - uint32_t max_offset = offset; - unsigned num_components; - char *name; - - if (!(ndr_flags & NDR_SCALARS)) { - return NDR_ERR_SUCCESS; - } - - name = talloc_strdup(ndr->current_mem_ctx, ""); - - /* break up name into a list of components */ - for (num_components=0; num_components 0) { - name = talloc_asprintf_append_buffer(name, ".%s", - component); - } else { - name = talloc_asprintf_append_buffer(name, "%s", - component); - } - NDR_ERR_HAVE_NO_MEMORY(name); - } - if (num_components == MAX_COMPONENTS) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME too many components"); - } - - (*s) = name; - ndr->offset = max_offset; - - return NDR_ERR_SUCCESS; + return ndr_pull_dns_string_list(ndr, + ndr_flags, + s); } + /** push a dns string to the wire */ diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index b054091c46c..6509f101145 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -2,6 +2,117 @@ #include "../librpc/ndr/libndr.h" #include "ndr_dns_utils.h" +/* don't allow an unlimited number of name components. The string must be less + than 255, with at least one character between dots, so 128 components is + plenty. + */ +#define MAX_COMPONENTS 128 + +/* + pull one component of a dns_string +*/ +static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, + uint8_t **component, + uint32_t *offset, + uint32_t *max_offset) +{ + uint8_t len; + unsigned int loops = 0; + while (loops < 5) { + if (*offset >= ndr->data_size) { + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD DNS NAME component, bad offset"); + } + len = ndr->data[*offset]; + if (len == 0) { + *offset += 1; + *max_offset = MAX(*max_offset, *offset); + *component = NULL; + return NDR_ERR_SUCCESS; + } + if ((len & 0xC0) == 0xC0) { + /* its a label pointer */ + if (1 + *offset >= ndr->data_size) { + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD DNS NAME component, " \ + "bad label offset"); + } + *max_offset = MAX(*max_offset, *offset + 2); + *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; + *max_offset = MAX(*max_offset, *offset); + loops++; + continue; + } + if ((len & 0xC0) != 0) { + /* its a reserved length field */ + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD DNS NAME component, " \ + "reserved length field: 0x%02x", + (len &0xC)); + } + if (*offset + len + 1 > ndr->data_size) { + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD DNS NAME component, "\ + "length too long"); + } + *component = (uint8_t*)talloc_strndup(ndr, + (const char *)&ndr->data[1 + *offset], len); + NDR_ERR_HAVE_NO_MEMORY(*component); + *offset += len + 1; + *max_offset = MAX(*max_offset, *offset); + return NDR_ERR_SUCCESS; + } + + /* too many pointers */ + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD DNS NAME component, too many pointers"); +} + +/** + pull a dns_string from the wire +*/ +enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, + int ndr_flags, + const char **s) +{ + uint32_t offset = ndr->offset; + uint32_t max_offset = offset; + unsigned num_components; + char *name; + + if (!(ndr_flags & NDR_SCALARS)) { + return NDR_ERR_SUCCESS; + } + + name = talloc_strdup(ndr->current_mem_ctx, ""); + + /* break up name into a list of components */ + for (num_components=0; num_components 0) { + name = talloc_asprintf_append_buffer(name, ".%s", + component); + } else { + name = talloc_asprintf_append_buffer(name, "%s", + component); + } + NDR_ERR_HAVE_NO_MEMORY(name); + } + if (num_components == MAX_COMPONENTS) { + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD DNS NAME too many components"); + } + + (*s) = name; + ndr->offset = max_offset; + + return NDR_ERR_SUCCESS; +} + /** push a dns/nbt string list to the wire diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h index 71a65433bbb..9d1b9c3a275 100644 --- a/librpc/ndr/ndr_dns_utils.h +++ b/librpc/ndr/ndr_dns_utils.h @@ -1,4 +1,8 @@ +enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, + int ndr_flags, + const char **s); + enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, int ndr_flags, -- 2.17.1 From df7e328eb67fbfd694f1a837dc44aabfde27d04d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 19 May 2020 13:55:53 +1200 Subject: [PATCH 2/7] CVE-2020-10745: ndr/nbt: use ndr_dns_utils/ndr_pull_dns_string_list To retain exactly the same behaviour with regard to memory contexts and error messages, we add an is_nbt flag. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns.c | 3 +- librpc/ndr/ndr_dns_utils.c | 60 ++++++++++++++++------- librpc/ndr/ndr_dns_utils.h | 3 +- librpc/ndr/ndr_nbt.c | 99 ++------------------------------------ 4 files changed, 50 insertions(+), 115 deletions(-) diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index 6b0dbd2c067..de168c82e7d 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -56,7 +56,8 @@ _PUBLIC_ enum ndr_err_code ndr_pull_dns_string(struct ndr_pull *ndr, { return ndr_pull_dns_string_list(ndr, ndr_flags, - s); + s, + false); } diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 6509f101145..41b4ee8e0ea 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -9,19 +9,22 @@ #define MAX_COMPONENTS 128 /* - pull one component of a dns_string + pull one component of a dns/nbt string */ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, + TALLOC_CTX *mem_ctx, uint8_t **component, uint32_t *offset, - uint32_t *max_offset) + uint32_t *max_offset, + const char *err_name) { uint8_t len; unsigned int loops = 0; while (loops < 5) { if (*offset >= ndr->data_size) { return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, bad offset"); + "BAD %s NAME component, bad offset", + err_name); } len = ndr->data[*offset]; if (len == 0) { @@ -34,8 +37,9 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, /* its a label pointer */ if (1 + *offset >= ndr->data_size) { return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, " \ - "bad label offset"); + "BAD %s NAME component, " \ + "bad label offset", + err_name); } *max_offset = MAX(*max_offset, *offset + 2); *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; @@ -46,16 +50,17 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, if ((len & 0xC0) != 0) { /* its a reserved length field */ return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, " \ + "BAD %s NAME component, " \ "reserved length field: 0x%02x", - (len &0xC)); + err_name, (len &0xC)); } if (*offset + len + 1 > ndr->data_size) { return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, "\ - "length too long"); + "BAD %s NAME component, "\ + "length too long", + err_name); } - *component = (uint8_t*)talloc_strndup(ndr, + *component = (uint8_t*)talloc_strndup(mem_ctx, (const char *)&ndr->data[1 + *offset], len); NDR_ERR_HAVE_NO_MEMORY(*component); *offset += len + 1; @@ -65,20 +70,32 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, /* too many pointers */ return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME component, too many pointers"); + "BAD %s NAME component, too many pointers", + err_name); } /** - pull a dns_string from the wire + pull a dns/nbt string from the wire */ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, int ndr_flags, - const char **s) + const char **s, + bool is_nbt) { uint32_t offset = ndr->offset; uint32_t max_offset = offset; unsigned num_components; char *name; + const char *err_name = NULL; + TALLOC_CTX *mem_ctx = NULL; + + if (is_nbt) { + err_name = "NBT"; + mem_ctx = ndr->current_mem_ctx; + } else { + err_name = "DNS"; + mem_ctx = ndr; + } if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; @@ -87,12 +104,18 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, name = talloc_strdup(ndr->current_mem_ctx, ""); /* break up name into a list of components */ - for (num_components=0; num_components 0) { name = talloc_asprintf_append_buffer(name, ".%s", component); @@ -104,7 +127,8 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, } if (num_components == MAX_COMPONENTS) { return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD DNS NAME too many components"); + "BAD %s NAME too many components", + err_name); } (*s) = name; diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h index 9d1b9c3a275..834fa9138f0 100644 --- a/librpc/ndr/ndr_dns_utils.h +++ b/librpc/ndr/ndr_dns_utils.h @@ -1,7 +1,8 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, int ndr_flags, - const char **s); + const char **s, + bool is_nbt); enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, diff --git a/librpc/ndr/ndr_nbt.c b/librpc/ndr/ndr_nbt.c index 8ed9f0a5f05..48f51ebbc23 100644 --- a/librpc/ndr/ndr_nbt.c +++ b/librpc/ndr/ndr_nbt.c @@ -28,9 +28,6 @@ #include "ndr_dns_utils.h" -/* don't allow an unlimited number of name components */ -#define MAX_COMPONENTS 128 - /** print a nbt string */ @@ -39,103 +36,15 @@ _PUBLIC_ void ndr_print_nbt_string(struct ndr_print *ndr, const char *name, cons ndr_print_string(ndr, name, s); } -/* - pull one component of a nbt_string -*/ -static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, - uint8_t **component, - uint32_t *offset, - uint32_t *max_offset) -{ - uint8_t len; - unsigned int loops = 0; - while (loops < 5) { - if (*offset >= ndr->data_size) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD NBT NAME component"); - } - len = ndr->data[*offset]; - if (len == 0) { - *offset += 1; - *max_offset = MAX(*max_offset, *offset); - *component = NULL; - return NDR_ERR_SUCCESS; - } - if ((len & 0xC0) == 0xC0) { - /* its a label pointer */ - if (1 + *offset >= ndr->data_size) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD NBT NAME component"); - } - *max_offset = MAX(*max_offset, *offset + 2); - *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; - *max_offset = MAX(*max_offset, *offset); - loops++; - continue; - } - if ((len & 0xC0) != 0) { - /* its a reserved length field */ - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD NBT NAME component"); - } - if (*offset + len + 1 > ndr->data_size) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD NBT NAME component"); - } - *component = (uint8_t*)talloc_strndup( - ndr->current_mem_ctx, - (const char *)&ndr->data[1 + *offset], len); - NDR_ERR_HAVE_NO_MEMORY(*component); - *offset += len + 1; - *max_offset = MAX(*max_offset, *offset); - return NDR_ERR_SUCCESS; - } - - /* too many pointers */ - return ndr_pull_error(ndr, NDR_ERR_STRING, "BAD NBT NAME component"); -} - /** pull a nbt_string from the wire */ _PUBLIC_ enum ndr_err_code ndr_pull_nbt_string(struct ndr_pull *ndr, int ndr_flags, const char **s) { - uint32_t offset = ndr->offset; - uint32_t max_offset = offset; - unsigned num_components; - char *name; - - if (!(ndr_flags & NDR_SCALARS)) { - return NDR_ERR_SUCCESS; - } - - name = NULL; - - /* break up name into a list of components */ - for (num_components=0;num_componentscurrent_mem_ctx, ""); - NDR_ERR_HAVE_NO_MEMORY(name); - } - - (*s) = name; - ndr->offset = max_offset; - - return NDR_ERR_SUCCESS; + return ndr_pull_dns_string_list(ndr, + ndr_flags, + s, + true); } /** -- 2.17.1 From aa7ab42162ebc491db90eb95a94dff26c5b7d28f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 20 May 2020 19:18:14 +1200 Subject: [PATCH 3/7] CVE-2020-10745: ndr: pull_dns_string: drop nbt/dns mem_ctx difference Until now NBT and DNS have used talloc contexts of different lifetimes to allocate component strings. The actual talloc context doesn't really matter -- these strings are immediately copied and can be freed straight after. So that is what we do. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns.c | 2 +- librpc/ndr/ndr_dns_utils.c | 17 +++-------------- librpc/ndr/ndr_dns_utils.h | 2 +- librpc/ndr/ndr_nbt.c | 2 +- 4 files changed, 6 insertions(+), 17 deletions(-) diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index de168c82e7d..15aba4ddb5c 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -57,7 +57,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_dns_string(struct ndr_pull *ndr, return ndr_pull_dns_string_list(ndr, ndr_flags, s, - false); + "DNS"); } diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 41b4ee8e0ea..898e91de12c 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -12,7 +12,6 @@ pull one component of a dns/nbt string */ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, - TALLOC_CTX *mem_ctx, uint8_t **component, uint32_t *offset, uint32_t *max_offset, @@ -60,7 +59,7 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, "length too long", err_name); } - *component = (uint8_t*)talloc_strndup(mem_ctx, + *component = (uint8_t*)talloc_strndup(ndr, (const char *)&ndr->data[1 + *offset], len); NDR_ERR_HAVE_NO_MEMORY(*component); *offset += len + 1; @@ -80,22 +79,12 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, int ndr_flags, const char **s, - bool is_nbt) + const char *err_name) { uint32_t offset = ndr->offset; uint32_t max_offset = offset; unsigned num_components; char *name; - const char *err_name = NULL; - TALLOC_CTX *mem_ctx = NULL; - - if (is_nbt) { - err_name = "NBT"; - mem_ctx = ndr->current_mem_ctx; - } else { - err_name = "DNS"; - mem_ctx = ndr; - } if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; @@ -109,7 +98,6 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, num_components++) { uint8_t *component = NULL; NDR_CHECK(ndr_pull_component(ndr, - mem_ctx, &component, &offset, &max_offset, err_name)); @@ -124,6 +112,7 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, component); } NDR_ERR_HAVE_NO_MEMORY(name); + TALLOC_FREE(component); } if (num_components == MAX_COMPONENTS) { return ndr_pull_error(ndr, NDR_ERR_STRING, diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h index 834fa9138f0..6604c24e2c6 100644 --- a/librpc/ndr/ndr_dns_utils.h +++ b/librpc/ndr/ndr_dns_utils.h @@ -2,7 +2,7 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, int ndr_flags, const char **s, - bool is_nbt); + const char *err_name); enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, diff --git a/librpc/ndr/ndr_nbt.c b/librpc/ndr/ndr_nbt.c index 48f51ebbc23..2d0ab4711c4 100644 --- a/librpc/ndr/ndr_nbt.c +++ b/librpc/ndr/ndr_nbt.c @@ -44,7 +44,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_nbt_string(struct ndr_pull *ndr, int ndr_fla return ndr_pull_dns_string_list(ndr, ndr_flags, s, - true); + "NBT"); } /** -- 2.17.1 From 2ac20bda836c4884e207ed26a422b52e15d2e27c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 20 May 2020 12:18:26 +1200 Subject: [PATCH 4/7] CVE-2020-10745: ndr: pull_dns_string: check length, use buffers/memcpy RFC 1035 says the maximum length for a DNS name is 255 characters, and one of the factors that allowed CVE-2020-10745 is that Samba did not enforce that directly, enabling names around 8k long. We fix that by keeping track of the name length. It is easier and more efficient to use a 64 byte buffer for the components, and this will help us to introduce further hardening in the next commit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 56 +++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 898e91de12c..97d5a200882 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -12,7 +12,8 @@ pull one component of a dns/nbt string */ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, - uint8_t **component, + uint8_t *component, + size_t *component_len, uint32_t *offset, uint32_t *max_offset, const char *err_name) @@ -29,7 +30,7 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, if (len == 0) { *offset += 1; *max_offset = MAX(*max_offset, *offset); - *component = NULL; + *component_len = 0; return NDR_ERR_SUCCESS; } if ((len & 0xC0) == 0xC0) { @@ -53,15 +54,15 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, "reserved length field: 0x%02x", err_name, (len &0xC)); } - if (*offset + len + 1 > ndr->data_size) { + if (*offset + len + 1 > ndr->data_size || + len > 63 /* impossible!, but we live in fear */ ) { return ndr_pull_error(ndr, NDR_ERR_STRING, "BAD %s NAME component, "\ "length too long", err_name); } - *component = (uint8_t*)talloc_strndup(ndr, - (const char *)&ndr->data[1 + *offset], len); - NDR_ERR_HAVE_NO_MEMORY(*component); + memcpy(component, &ndr->data[1 + *offset], len); + *component_len = len; *offset += len + 1; *max_offset = MAX(*max_offset, *offset); return NDR_ERR_SUCCESS; @@ -85,41 +86,58 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, uint32_t max_offset = offset; unsigned num_components; char *name; + size_t name_len; if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; } - name = talloc_strdup(ndr->current_mem_ctx, ""); + name = talloc_array(ndr->current_mem_ctx, char, 256); + NDR_ERR_HAVE_NO_MEMORY(name); + name_len = 0; - /* break up name into a list of components */ for (num_components = 0; num_components < MAX_COMPONENTS; num_components++) { - uint8_t *component = NULL; + uint8_t component[64]; + size_t component_len; NDR_CHECK(ndr_pull_component(ndr, - &component, &offset, + component, + &component_len, + &offset, &max_offset, err_name)); - if (component == NULL) { + if (component_len == 0) { break; } - if (num_components > 0) { - name = talloc_asprintf_append_buffer(name, ".%s", - component); - } else { - name = talloc_asprintf_append_buffer(name, "%s", - component); + if (name_len + component_len > 253) { + /* + * maximum *wire* size is 255, per RFC1035, but we + * compare to 253 because a) there is one more length + * field than there are separating dots, and b) there + * is a final zero-length root node, not represented + * in dotted form. + */ + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD %s NAME too long", + err_name); } - NDR_ERR_HAVE_NO_MEMORY(name); - TALLOC_FREE(component); + if (name_len > 0) { + name[name_len] = '.'; + name_len++; + } + + memcpy(name + name_len, component, component_len); + name_len += component_len; } + if (num_components == MAX_COMPONENTS) { return ndr_pull_error(ndr, NDR_ERR_STRING, "BAD %s NAME too many components", err_name); } + name[name_len] = '\0'; (*s) = name; ndr->offset = max_offset; -- 2.17.1 From cd451a3e79b6206b158f56c0e19c68c261a79afd Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 20 May 2020 10:05:16 +1200 Subject: [PATCH 5/7] CVE-2020-10745: ndr: pull_dns_string: don't allow dots or '\0' in labels We use a modified version of memcpy that returns NULL if the copied string contains the bad characters. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 97d5a200882..dd7b2ce7f45 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -8,6 +8,27 @@ */ #define MAX_COMPONENTS 128 + + +/* + * If the src component is "good" as a DNS component, we copy it to dest and + * return dest, just like memcpy. If it is bad, we do the copy anyway but + * return NULL. + */ +static uint8_t* dns_component_copy(uint8_t *dest, uint8_t *src, size_t len) +{ + size_t i; + uint8_t *ret = dest; + for (i = 0; i < len; i++) { + uint8_t c = src[i]; + dest[i] = c; + if (c == '\0' || c == '.') { + ret = NULL; + } + } + return ret; +} + /* pull one component of a dns/nbt string */ @@ -20,6 +41,7 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, { uint8_t len; unsigned int loops = 0; + uint8_t *valid_comp; while (loops < 5) { if (*offset >= ndr->data_size) { return ndr_pull_error(ndr, NDR_ERR_STRING, @@ -61,7 +83,16 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, "length too long", err_name); } - memcpy(component, &ndr->data[1 + *offset], len); + + valid_comp = dns_component_copy(component, + &ndr->data[1 + *offset], + len); + if (valid_comp == NULL) { + return ndr_pull_error(ndr, NDR_ERR_STRING, + "BAD %s NAME component, "\ + "undesirable characters", + err_name); + } *component_len = len; *offset += len + 1; *max_offset = MAX(*max_offset, *offset); -- 2.17.1 From 3a5e37c8eb87fb43cb8069e523dfe33a76999fcb Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 20 May 2020 12:18:49 +1200 Subject: [PATCH 6/7] CVE-2020-10745: ndr: pull_dns_string: remove dead branch Because we now keep track of the name length (which must be < 254), there is no way for the number of components to exceed 127, so this branch is never seen. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index dd7b2ce7f45..3d05961c089 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -162,12 +162,6 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, name_len += component_len; } - if (num_components == MAX_COMPONENTS) { - return ndr_pull_error(ndr, NDR_ERR_STRING, - "BAD %s NAME too many components", - err_name); - } - name[name_len] = '\0'; (*s) = name; ndr->offset = max_offset; -- 2.17.1 From 39bfa5db34607061788f4062b36bd26e63499d36 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 14:56:05 +1200 Subject: [PATCH 7/7] CVE-2020-10745: ndr/util push_dns_string: avoid unnecessary tallocs We know the components are all less than 64 bytes long. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 3d05961c089..6abd0970cfe 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -193,7 +193,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, while (s && *s) { enum ndr_err_code ndr_err; - char *compname; + uint8_t compname[64]; size_t complen; uint32_t offset; @@ -248,12 +248,6 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, complen++; } - compname = talloc_asprintf(ndr, "%c%*.*s", - (unsigned char)complen, - (unsigned char)complen, - (unsigned char)complen, s); - NDR_ERR_HAVE_NO_MEMORY(compname); - /* remember the current component + the rest of the string * so it can be reused later */ @@ -263,9 +257,9 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, } /* push just this component into the blob */ - NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, - complen+1)); - talloc_free(compname); + compname[0] = complen; + memcpy(compname + 1, s, complen); + NDR_CHECK(ndr_push_bytes(ndr, compname, complen + 1)); s += complen; if (*s == '.') { -- 2.17.1