The Samba-Bugzilla – Attachment 16000 Details for
Bug 14378
CVE-2020-10745 [SECURITY] invalid DNS or NBT queries containing dots use several seconds of CPU each
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
additional patches for master
further-patches-for-master.patch (text/plain), 29.66 KB, created by
Douglas Bagnall
on 2020-05-21 03:55:33 UTC
(
hide
)
Description:
additional patches for master
Filename:
MIME Type:
Creator:
Douglas Bagnall
Created:
2020-05-21 03:55:33 UTC
Size:
29.66 KB
patch
obsolete
>From 2a52262b36c74a15b1cd7e0d0aadd20d81c78f69 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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<MAX_COMPONENTS; >- num_components++) { >- uint8_t *component = NULL; >- NDR_CHECK(ndr_pull_component(ndr, &component, &offset, >- &max_offset)); >- if (component == NULL) break; >- if (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<MAX_COMPONENTS; >+ num_components++) { >+ uint8_t *component = NULL; >+ NDR_CHECK(ndr_pull_component(ndr, &component, &offset, >+ &max_offset)); >+ if (component == NULL) break; >+ if (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 <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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<MAX_COMPONENTS; >+ for (num_components = 0; >+ num_components < MAX_COMPONENTS; > num_components++) { > uint8_t *component = NULL; >- NDR_CHECK(ndr_pull_component(ndr, &component, &offset, >- &max_offset)); >- if (component == NULL) break; >+ NDR_CHECK(ndr_pull_component(ndr, >+ mem_ctx, >+ &component, &offset, >+ &max_offset, >+ err_name)); >+ if (component == NULL) { >+ break; >+ } > if (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_components<MAX_COMPONENTS;num_components++) { >- uint8_t *component = NULL; >- NDR_CHECK(ndr_pull_component(ndr, &component, &offset, &max_offset)); >- if (component == NULL) break; >- if (name) { >- name = talloc_asprintf_append_buffer(name, ".%s", component); >- NDR_ERR_HAVE_NO_MEMORY(name); >- } else { >- name = (char *)component; >- } >- } >- if (num_components == MAX_COMPONENTS) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD NBT NAME too many components"); >- } >- if (num_components == 0) { >- name = talloc_strdup(ndr->current_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 <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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 <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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 <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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 <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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 <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >--- > 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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 14378
:
15978
|
15982
|
15999
|
16000
|
16001
|
16015
|
16016
|
16034
|
16035
|
16036
|
16037
|
16038
|
16039
|
16040
|
16041
|
16042
|
16045
|
16046
|
16047
|
16048
|
16049
|
16050
|
16051
|
16052
|
16053
|
16057
|
16068
|
16090
|
16093
|
16099