From 3e7e136f20aa98d35244989b5d4232b11ae8bfcd Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 9 Jun 2015 12:42:10 -0700 Subject: [PATCH 01/15] CVE-2015-3223: lib: ldb: Cope with canonicalise_fn returning string "", length 0. BUG: https://bugzilla.samba.org/show_bug.cgi?id=11325 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme --- lib/ldb/common/ldb_match.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index a493dae..7414289 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -271,6 +271,14 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, if (cnk.length > val.length) { goto mismatch; } + /* + * Empty strings are returned as length 0. Ensure + * we can cope with this. + */ + if (cnk.length == 0) { + goto mismatch; + } + if (memcmp((char *)val.data, (char *)cnk.data, cnk.length) != 0) goto mismatch; val.length -= cnk.length; val.data += cnk.length; @@ -284,7 +292,13 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, chunk = tree->u.substring.chunks[c]; if(a->syntax->canonicalise_fn(ldb, ldb, chunk, &cnk) != 0) goto mismatch; - /* FIXME: case of embedded nulls */ + /* + * Empty strings are returned as length 0. Ensure + * we can cope with this. + */ + if (cnk.length == 0) { + goto mismatch; + } p = strstr((char *)val.data, (char *)cnk.data); if (p == NULL) goto mismatch; if ( (! tree->u.substring.chunks[c + 1]) && (! tree->u.substring.end_with_wildcard) ) { -- 1.9.1 From ef6f1517cc12ef2c5ec076bf8b00c03adc14de0b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 9 Jun 2015 14:00:01 -0700 Subject: [PATCH 02/15] CVE-2015-3223: lib: ldb: Use memmem binary search, not strstr text search. Values might have embedded zeros. BUG: https://bugzilla.samba.org/show_bug.cgi?id=11325 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme --- lib/ldb/common/ldb_match.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c index 7414289..182c6ce 100644 --- a/lib/ldb/common/ldb_match.c +++ b/lib/ldb/common/ldb_match.c @@ -241,7 +241,6 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, struct ldb_val val; struct ldb_val cnk; struct ldb_val *chunk; - char *p, *g; uint8_t *save_p = NULL; unsigned int c = 0; @@ -288,6 +287,7 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, } while (tree->u.substring.chunks[c]) { + uint8_t *p; chunk = tree->u.substring.chunks[c]; if(a->syntax->canonicalise_fn(ldb, ldb, chunk, &cnk) != 0) goto mismatch; @@ -299,15 +299,24 @@ static int ldb_wildcard_compare(struct ldb_context *ldb, if (cnk.length == 0) { goto mismatch; } - p = strstr((char *)val.data, (char *)cnk.data); + /* + * Values might be binary blobs. Don't use string + * search, but memory search instead. + */ + p = memmem((const void *)val.data,val.length, + (const void *)cnk.data, cnk.length); if (p == NULL) goto mismatch; if ( (! tree->u.substring.chunks[c + 1]) && (! tree->u.substring.end_with_wildcard) ) { + uint8_t *g; do { /* greedy */ - g = strstr((char *)p + cnk.length, (char *)cnk.data); + g = memmem(p + cnk.length, + val.length - (p - val.data), + (const uint8_t *)cnk.data, + cnk.length); if (g) p = g; } while(g); } - val.length = val.length - (p - (char *)(val.data)) - cnk.length; + val.length = val.length - (p - (uint8_t *)(val.data)) - cnk.length; val.data = (uint8_t *)(p + cnk.length); c++; talloc_free(cnk.data); -- 1.9.1 From 8f25f5d2e822b890b6d18fa67399646f221f262e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 24 Nov 2015 13:07:23 +1300 Subject: [PATCH 03/15] CVE-2015-5330: ldb_dn: simplify and fix ldb_dn_escape_internal() Previously we relied on NUL terminated strings and jumped back and forth between copying escaped bytes and memcpy()ing un-escaped chunks. This simple version is easier to reason about and works with unterminated strings. It may also be faster as it avoids reading the string twice (first with strcspn, then with memcpy). Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Signed-off-by: Douglas Bagnall Pair-programmed-with: Andrew Bartlett Reviewed-by: Ralph Boehme --- lib/ldb/common/ldb_dn.c | 46 ++++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 6b6f90c..1b8e51e 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -189,33 +189,23 @@ struct ldb_dn *ldb_dn_new_fmt(TALLOC_CTX *mem_ctx, /* see RFC2253 section 2.4 */ static int ldb_dn_escape_internal(char *dst, const char *src, int len) { - const char *p, *s; + char c; char *d; - size_t l; - - p = s = src; + int i; d = dst; - while (p - src < len) { - p += strcspn(p, ",=\n\r+<>#;\\\" "); - - if (p - src == len) /* found no escapable chars */ - break; - - /* copy the part of the string before the stop */ - memcpy(d, s, p - s); - d += (p - s); /* move to current position */ - - switch (*p) { + for (i = 0; i < len; i++){ + c = src[i]; + switch (c) { case ' ': - if (p == src || (p-src)==(len-1)) { + if (i == 0 || i == len - 1) { /* if at the beginning or end * of the string then escape */ *d++ = '\\'; - *d++ = *p++; + *d++ = c; } else { /* otherwise don't escape */ - *d++ = *p++; + *d++ = c; } break; @@ -231,30 +221,30 @@ static int ldb_dn_escape_internal(char *dst, const char *src, int len) case '?': /* these must be escaped using \c form */ *d++ = '\\'; - *d++ = *p++; + *d++ = c; break; - default: { + case ';': + case '\r': + case '\n': + case '=': + case '\0': { /* any others get \XX form */ unsigned char v; const char *hexbytes = "0123456789ABCDEF"; - v = *(const unsigned char *)p; + v = (const unsigned char)c; *d++ = '\\'; *d++ = hexbytes[v>>4]; *d++ = hexbytes[v&0xF]; - p++; break; } + default: + *d++ = c; } - s = p; /* move forward */ } - /* copy the last part (with zero) and return */ - l = len - (s - src); - memcpy(d, s, l + 1); - /* return the length of the resulting string */ - return (l + (d - dst)); + return (d - dst); } char *ldb_dn_escape_value(TALLOC_CTX *mem_ctx, struct ldb_val value) -- 1.9.1 From eaec4520908b60a20ca1d8e206b516cb97e39ecd Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 24 Nov 2015 13:09:36 +1300 Subject: [PATCH 04/15] CVE-2015-5330: ldb_dn_escape_value: use known string length, not strlen() ldb_dn_escape_internal() reports the number of bytes it copied, so lets use that number, rather than using strlen() and hoping a zero got in the right place. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Signed-off-by: Douglas Bagnall Pair-programmed-with: Andrew Bartlett Reviewed-by: Ralph Boehme --- lib/ldb/common/ldb_dn.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 1b8e51e..a3b8f92 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -250,7 +250,7 @@ static int ldb_dn_escape_internal(char *dst, const char *src, int len) char *ldb_dn_escape_value(TALLOC_CTX *mem_ctx, struct ldb_val value) { char *dst; - + size_t len; if (!value.length) return NULL; @@ -261,10 +261,14 @@ char *ldb_dn_escape_value(TALLOC_CTX *mem_ctx, struct ldb_val value) return NULL; } - ldb_dn_escape_internal(dst, (const char *)value.data, value.length); - - dst = talloc_realloc(mem_ctx, dst, char, strlen(dst) + 1); + len = ldb_dn_escape_internal(dst, (const char *)value.data, value.length); + dst = talloc_realloc(mem_ctx, dst, char, len + 1); + if ( ! dst) { + talloc_free(dst); + return NULL; + } + dst[len] = '\0'; return dst; } -- 1.9.1 From fc9e5048bf2bb2f5f0f538735dbc5661f2b4ab0f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 24 Nov 2015 13:47:16 +1300 Subject: [PATCH 05/15] CVE-2015-5330: Fix handling of unicode near string endings Until now next_codepoint_ext() and next_codepoint_handle_ext() were using strnlen(str, 5) to determine how much string they should try to decode. This ended up looking past the end of the string when it was not null terminated and the final character looked like a multi-byte encoding. The fix is to let the caller say how long the string can be. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Signed-off-by: Douglas Bagnall Pair-programmed-with: Andrew Bartlett Reviewed-by: Ralph Boehme --- lib/util/charset/charset.h | 9 +++++---- lib/util/charset/codepoints.c | 24 ++++++++++++++++-------- lib/util/charset/util_str.c | 3 ++- lib/util/charset/util_unistr.c | 3 ++- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/lib/util/charset/charset.h b/lib/util/charset/charset.h index 0d69d31..ca7a437 100644 --- a/lib/util/charset/charset.h +++ b/lib/util/charset/charset.h @@ -174,15 +174,16 @@ smb_iconv_t get_conv_handle(struct smb_iconv_handle *ic, charset_t from, charset_t to); const char *charset_name(struct smb_iconv_handle *ic, charset_t ch); -codepoint_t next_codepoint_ext(const char *str, charset_t src_charset, - size_t *size); +codepoint_t next_codepoint_ext(const char *str, size_t len, + charset_t src_charset, size_t *size); codepoint_t next_codepoint(const char *str, size_t *size); ssize_t push_codepoint(char *str, codepoint_t c); /* codepoints */ codepoint_t next_codepoint_handle_ext(struct smb_iconv_handle *ic, - const char *str, charset_t src_charset, - size_t *size); + const char *str, size_t len, + charset_t src_charset, + size_t *size); codepoint_t next_codepoint_handle(struct smb_iconv_handle *ic, const char *str, size_t *size); ssize_t push_codepoint_handle(struct smb_iconv_handle *ic, diff --git a/lib/util/charset/codepoints.c b/lib/util/charset/codepoints.c index 19319ba..99d209f 100644 --- a/lib/util/charset/codepoints.c +++ b/lib/util/charset/codepoints.c @@ -16657,7 +16657,8 @@ smb_iconv_t get_conv_handle(struct smb_iconv_handle *ic, */ _PUBLIC_ codepoint_t next_codepoint_handle_ext( struct smb_iconv_handle *ic, - const char *str, charset_t src_charset, + const char *str, size_t len, + charset_t src_charset, size_t *bytes_consumed) { /* it cannot occupy more than 4 bytes in UTF16 format */ @@ -16677,7 +16678,7 @@ _PUBLIC_ codepoint_t next_codepoint_handle_ext( * we assume that no multi-byte character can take more than 5 bytes. * This is OK as we only support codepoints up to 1M (U+100000) */ - ilen_orig = strnlen(str, 5); + ilen_orig = MIN(len, 5); ilen = ilen_orig; descriptor = get_conv_handle(ic, src_charset, CH_UTF16); @@ -16733,9 +16734,16 @@ _PUBLIC_ codepoint_t next_codepoint_handle_ext( return INVALID_CODEPOINT if the next character cannot be converted */ _PUBLIC_ codepoint_t next_codepoint_handle(struct smb_iconv_handle *ic, - const char *str, size_t *size) + const char *str, size_t *size) { - return next_codepoint_handle_ext(ic, str, CH_UNIX, size); + /* + * We assume that no multi-byte character can take more than 5 bytes + * thus avoiding walking all the way down a long string. This is OK as + * Unicode codepoints only go up to (U+10ffff), which can always be + * encoded in 4 bytes or less. + */ + return next_codepoint_handle_ext(ic, str, strnlen(str, 5), CH_UNIX, + size); } /* @@ -16797,11 +16805,11 @@ _PUBLIC_ ssize_t push_codepoint_handle(struct smb_iconv_handle *ic, return 5 - olen; } -_PUBLIC_ codepoint_t next_codepoint_ext(const char *str, charset_t src_charset, - size_t *size) +_PUBLIC_ codepoint_t next_codepoint_ext(const char *str, size_t len, + charset_t src_charset, size_t *size) { - return next_codepoint_handle_ext(get_iconv_handle(), str, - src_charset, size); + return next_codepoint_handle_ext(get_iconv_handle(), str, len, + src_charset, size); } _PUBLIC_ codepoint_t next_codepoint(const char *str, size_t *size) diff --git a/lib/util/charset/util_str.c b/lib/util/charset/util_str.c index c1b81f1..90197e0 100644 --- a/lib/util/charset/util_str.c +++ b/lib/util/charset/util_str.c @@ -210,7 +210,8 @@ _PUBLIC_ size_t strlen_m_ext_handle(struct smb_iconv_handle *ic, while (*s) { size_t c_size; - codepoint_t c = next_codepoint_handle_ext(ic, s, src_charset, &c_size); + codepoint_t c = next_codepoint_handle_ext(ic, s, strnlen(s, 5), + src_charset, &c_size); s += c_size; switch (dst_charset) { diff --git a/lib/util/charset/util_unistr.c b/lib/util/charset/util_unistr.c index e4ae650..f299269 100644 --- a/lib/util/charset/util_unistr.c +++ b/lib/util/charset/util_unistr.c @@ -112,7 +112,8 @@ _PUBLIC_ char *strupper_talloc_n_handle(struct smb_iconv_handle *iconv_handle, while (n-- && *src) { size_t c_size; - codepoint_t c = next_codepoint_handle(iconv_handle, src, &c_size); + codepoint_t c = next_codepoint_handle_ext(iconv_handle, src, n, + CH_UNIX, &c_size); src += c_size; c = toupper_m(c); -- 1.9.1 From 8d718586d3f53baff81adfccb17da98e1ad029fd Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 24 Nov 2015 13:49:09 +1300 Subject: [PATCH 06/15] CVE-2015-5330: strupper_talloc_n_handle(): properly count characters When a codepoint eats more than one byte we really want to know, especially if the string is not NUL terminated. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Signed-off-by: Douglas Bagnall Pair-programmed-with: Andrew Bartlett Reviewed-by: Ralph Boehme --- lib/util/charset/util_unistr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/util/charset/util_unistr.c b/lib/util/charset/util_unistr.c index f299269..2cc8718 100644 --- a/lib/util/charset/util_unistr.c +++ b/lib/util/charset/util_unistr.c @@ -110,11 +110,12 @@ _PUBLIC_ char *strupper_talloc_n_handle(struct smb_iconv_handle *iconv_handle, return NULL; } - while (n-- && *src) { + while (n && *src) { size_t c_size; codepoint_t c = next_codepoint_handle_ext(iconv_handle, src, n, CH_UNIX, &c_size); src += c_size; + n -= c_size; c = toupper_m(c); -- 1.9.1 From 4b8de776930e9b7e4c7da05a09b6f4d482877244 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 24 Nov 2015 13:54:09 +1300 Subject: [PATCH 07/15] CVE-2015-5330: next_codepoint_handle_ext: don't short-circuit UTF16 low bytes UTF16 contains zero bytes when it is encoding ASCII (for example), so we can't assume the absense of the 0x80 bit means a one byte encoding. No current callers use UTF16. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Signed-off-by: Douglas Bagnall Pair-programmed-with: Andrew Bartlett Reviewed-by: Ralph Boehme --- lib/util/charset/codepoints.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/util/charset/codepoints.c b/lib/util/charset/codepoints.c index 99d209f..3d444a6 100644 --- a/lib/util/charset/codepoints.c +++ b/lib/util/charset/codepoints.c @@ -16669,7 +16669,10 @@ _PUBLIC_ codepoint_t next_codepoint_handle_ext( size_t olen; char *outbuf; - if ((str[0] & 0x80) == 0) { + + if (((str[0] & 0x80) == 0) && (src_charset == CH_DOS || + src_charset == CH_UNIX || + src_charset == CH_UTF8)) { *bytes_consumed = 1; return (codepoint_t)str[0]; } -- 1.9.1 From d3beacd5c51d5cfaa10ce544bc640b3dc3e66ad7 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 26 Nov 2015 11:17:11 +1300 Subject: [PATCH 08/15] CVE-2015-5330: ldb_dn_explode: copy strings by length, not terminators That is, memdup(), not strdup(). The terminators might not be there. But, we have to make sure we put the terminator on, because we tend to assume the terminator is there in other places. Use talloc_set_name_const() on the resulting chunk so talloc_report() remains unchanged. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Signed-off-by: Douglas Bagnall Pair-programmed-with: Andrew Bartlett Pair-programmed-with: Garming Sam Pair-programmed-with: Stefan Metzmacher Pair-programmed-with: Ralph Boehme --- lib/ldb/common/ldb_dn.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index a3b8f92..cd17cda 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -586,12 +586,15 @@ static bool ldb_dn_explode(struct ldb_dn *dn) p++; *d++ = '\0'; - dn->components[dn->comp_num].value.data = (uint8_t *)talloc_strdup(dn->components, dt); + dn->components[dn->comp_num].value.data = \ + (uint8_t *)talloc_memdup(dn->components, dt, l + 1); dn->components[dn->comp_num].value.length = l; if ( ! dn->components[dn->comp_num].value.data) { /* ouch ! */ goto failed; } + talloc_set_name_const(dn->components[dn->comp_num].value.data, + (const char *)dn->components[dn->comp_num].value.data); dt = d; @@ -707,11 +710,13 @@ static bool ldb_dn_explode(struct ldb_dn *dn) *d++ = '\0'; dn->components[dn->comp_num].value.length = l; dn->components[dn->comp_num].value.data = - (uint8_t *)talloc_strdup(dn->components, dt); + (uint8_t *)talloc_memdup(dn->components, dt, l + 1); if ( ! dn->components[dn->comp_num].value.data) { /* ouch */ goto failed; } + talloc_set_name_const(dn->components[dn->comp_num].value.data, + (const char *)dn->components[dn->comp_num].value.data); dn->comp_num++; -- 1.9.1 From 5b730ab846a9d4ae44b425094af2934c2f7b2843 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 8 Dec 2015 10:55:42 +0100 Subject: [PATCH 09/15] ldb: bump version of the required system ldb to 1.1.24 This is needed to ensure we build against a system ldb library that contains the fixes for CVE-2015-5330 and CVE-2015-3223. autobuild must still be able to build against the older version 1.1.21 including the patches. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11325 Bug: https://bugzilla.samba.org/show_bug.cgi?id=11599 Bug: https://bugzilla.samba.org/show_bug.cgi?id=11636 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher --- lib/ldb/wscript | 5 +++-- script/autobuild.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/ldb/wscript b/lib/ldb/wscript index 0996f51..4c70e2f 100755 --- a/lib/ldb/wscript +++ b/lib/ldb/wscript @@ -2,6 +2,7 @@ APPNAME = 'ldb' VERSION = '1.1.21' +SYSTEM_VERSION = '1.1.24' blddir = 'bin' @@ -55,11 +56,11 @@ def configure(conf): conf.env.standalone_ldb = conf.IN_LAUNCH_DIR() if not conf.env.standalone_ldb: - if conf.CHECK_BUNDLED_SYSTEM_PKG('pyldb-util', minversion=VERSION, + if conf.CHECK_BUNDLED_SYSTEM_PKG('pyldb-util', minversion=SYSTEM_VERSION, onlyif='talloc tdb tevent', implied_deps='replace talloc tdb tevent ldb'): conf.define('USING_SYSTEM_PYLDB_UTIL', 1) - if conf.CHECK_BUNDLED_SYSTEM_PKG('ldb', minversion=VERSION, + if conf.CHECK_BUNDLED_SYSTEM_PKG('ldb', minversion=SYSTEM_VERSION, onlyif='talloc tdb tevent pyldb-util', implied_deps='replace talloc tdb tevent'): conf.define('USING_SYSTEM_LDB', 1) diff --git a/script/autobuild.py b/script/autobuild.py index d2662b9..c7ca632 100755 --- a/script/autobuild.py +++ b/script/autobuild.py @@ -105,7 +105,7 @@ tasks = { ("ldb-make", "cd lib/ldb && make", "text/plain"), ("ldb-install", "cd lib/ldb && make install", "text/plain"), - ("configure", "PYTHONPATH=${PYTHON_PREFIX}/site-packages:$PYTHONPATH PKG_CONFIG_PATH=$PKG_CONFIG_PATH:${PREFIX_DIR}/lib/pkgconfig ./configure --bundled-libraries=!talloc,!tdb,!pytdb,!ldb,!pyldb,!tevent,!pytevent --abi-check --enable-debug -C ${PREFIX}", "text/plain"), + ("configure", "PYTHONPATH=${PYTHON_PREFIX}/site-packages:$PYTHONPATH PKG_CONFIG_PATH=$PKG_CONFIG_PATH:${PREFIX_DIR}/lib/pkgconfig ./configure --bundled-libraries=!talloc,!tdb,!pytdb,!ldb,!pyldb,!tevent,!pytevent --minimum-library-version=ldb:1.1.21,pyldb-util:1.1.21 --abi-check --enable-debug -C ${PREFIX}", "text/plain"), ("make", "make", "text/plain"), ("install", "make install", "text/plain"), ("dist", "make dist", "text/plain")], -- 1.9.1 From 4278ef25f64d5fdbf432ff1534e275416ec9561e Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 9 Jul 2015 10:58:11 -0700 Subject: [PATCH 10/15] CVE-2015-5252: s3: smbd: Fix symlink verification (file access outside the share). Ensure matching component ends in '/' or '\0'. BUG: https://bugzilla.samba.org/show_bug.cgi?id=11395 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke --- source3/smbd/vfs.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c index 9f3ba6d..f14ecbe 100644 --- a/source3/smbd/vfs.c +++ b/source3/smbd/vfs.c @@ -982,6 +982,7 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn, struct smb_filename *smb_fname_cwd = NULL; struct privilege_paths *priv_paths = NULL; int ret; + bool matched; DEBUG(3,("check_reduced_name_with_privilege [%s] [%s]\n", fname, @@ -1076,7 +1077,10 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn, } rootdir_len = strlen(conn_rootdir); - if (strncmp(conn_rootdir, resolved_name, rootdir_len) != 0) { + matched = (strncmp(conn_rootdir, resolved_name, rootdir_len) == 0); + + if (!matched || (resolved_name[rootdir_len] != '/' && + resolved_name[rootdir_len] != '\0')) { DEBUG(2, ("check_reduced_name_with_privilege: Bad access " "attempt: %s is a symlink outside the " "share path\n", @@ -1216,6 +1220,7 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname) if (!allow_widelinks || !allow_symlinks) { const char *conn_rootdir; size_t rootdir_len; + bool matched; conn_rootdir = SMB_VFS_CONNECTPATH(conn, fname); if (conn_rootdir == NULL) { @@ -1226,8 +1231,10 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname) } rootdir_len = strlen(conn_rootdir); - if (strncmp(conn_rootdir, resolved_name, - rootdir_len) != 0) { + matched = (strncmp(conn_rootdir, resolved_name, + rootdir_len) == 0); + if (!matched || (resolved_name[rootdir_len] != '/' && + resolved_name[rootdir_len] != '\0')) { DEBUG(2, ("check_reduced_name: Bad access " "attempt: %s is a symlink outside the " "share path\n", fname)); -- 1.9.1 From 675fd8d771f9d43e354dba53ddd9b5483ae0a1d7 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 23 Oct 2015 14:54:31 -0700 Subject: [PATCH 11/15] CVE-2015-5299: s3-shadow-copy2: fix missing access check on snapdir Fix originally from https://bugzilla.samba.org/show_bug.cgi?id=11529 Signed-off-by: Jeremy Allison Reviewed-by: David Disseldorp --- source3/modules/vfs_shadow_copy2.c | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/source3/modules/vfs_shadow_copy2.c b/source3/modules/vfs_shadow_copy2.c index 4d2ec54..d1673a4 100644 --- a/source3/modules/vfs_shadow_copy2.c +++ b/source3/modules/vfs_shadow_copy2.c @@ -30,6 +30,7 @@ */ #include "includes.h" +#include "smbd/smbd.h" #include "system/filesys.h" #include "include/ntioctl.h" #include "util_tdb.h" @@ -1180,6 +1181,42 @@ static char *have_snapdir(struct vfs_handle_struct *handle, return NULL; } +static bool check_access_snapdir(struct vfs_handle_struct *handle, + const char *path) +{ + struct smb_filename smb_fname; + int ret; + NTSTATUS status; + + ZERO_STRUCT(smb_fname); + smb_fname.base_name = talloc_asprintf(talloc_tos(), + "%s", + path); + if (smb_fname.base_name == NULL) { + return false; + } + + ret = SMB_VFS_NEXT_STAT(handle, &smb_fname); + if (ret != 0 || !S_ISDIR(smb_fname.st.st_ex_mode)) { + TALLOC_FREE(smb_fname.base_name); + return false; + } + + status = smbd_check_access_rights(handle->conn, + &smb_fname, + false, + SEC_DIR_LIST); + if (!NT_STATUS_IS_OK(status)) { + DEBUG(0,("user does not have list permission " + "on snapdir %s\n", + smb_fname.base_name)); + TALLOC_FREE(smb_fname.base_name); + return false; + } + TALLOC_FREE(smb_fname.base_name); + return true; +} + /** * Find the snapshot directory (if any) for the given * filename (which is relative to the share). @@ -1329,6 +1366,7 @@ static int shadow_copy2_get_shadow_copy_data( const char *snapdir; struct dirent *d; TALLOC_CTX *tmp_ctx = talloc_stackframe(); + bool ret; snapdir = shadow_copy2_find_snapdir(tmp_ctx, handle, fsp->fsp_name); if (snapdir == NULL) { @@ -1338,6 +1376,13 @@ static int shadow_copy2_get_shadow_copy_data( talloc_free(tmp_ctx); return -1; } + ret = check_access_snapdir(handle, snapdir); + if (!ret) { + DEBUG(0,("access denied on listing snapdir %s\n", snapdir)); + errno = EACCES; + talloc_free(tmp_ctx); + return -1; + } p = SMB_VFS_NEXT_OPENDIR(handle, snapdir, NULL, 0); -- 1.9.1 From d724f835acb9f4886c0001af32cd325dbbf1f895 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 30 Sep 2015 21:17:02 +0200 Subject: [PATCH 12/15] CVE-2015-5296: s3:libsmb: force signing when requiring encryption in do_connect() BUG: https://bugzilla.samba.org/show_bug.cgi?id=11536 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison --- source3/libsmb/clidfs.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source3/libsmb/clidfs.c b/source3/libsmb/clidfs.c index 2121ad0..d3b0580 100644 --- a/source3/libsmb/clidfs.c +++ b/source3/libsmb/clidfs.c @@ -114,6 +114,11 @@ static NTSTATUS do_connect(TALLOC_CTX *ctx, const char *domain; NTSTATUS status; int flags = 0; + int signing_state = get_cmdline_auth_info_signing_state(auth_info); + + if (force_encrypt) { + signing_state = SMB_SIGNING_REQUIRED; + } /* make a copy so we don't modify the global string 'service' */ servicename = talloc_strdup(ctx,share); @@ -152,7 +157,7 @@ static NTSTATUS do_connect(TALLOC_CTX *ctx, status = cli_connect_nb( server, NULL, port, name_type, NULL, - get_cmdline_auth_info_signing_state(auth_info), + signing_state, flags, &c); if (!NT_STATUS_IS_OK(status)) { -- 1.9.1 From 1ba49b8f389eda3414b14410c7fbcb4041ca06b1 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 30 Sep 2015 21:17:02 +0200 Subject: [PATCH 13/15] CVE-2015-5296: s3:libsmb: force signing when requiring encryption in SMBC_server_internal() BUG: https://bugzilla.samba.org/show_bug.cgi?id=11536 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison --- source3/libsmb/libsmb_server.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/source3/libsmb/libsmb_server.c b/source3/libsmb/libsmb_server.c index 07d51ea..2abd37e 100644 --- a/source3/libsmb/libsmb_server.c +++ b/source3/libsmb/libsmb_server.c @@ -273,6 +273,7 @@ SMBC_server_internal(TALLOC_CTX *ctx, char *newserver, *newshare; int flags = 0; struct smbXcli_tcon *tcon = NULL; + int signing_state = SMB_SIGNING_DEFAULT; ZERO_STRUCT(c); *in_cache = false; @@ -439,6 +440,10 @@ SMBC_server_internal(TALLOC_CTX *ctx, flags |= CLI_FULL_CONNECTION_USE_NT_HASH; } + if (context->internal->smb_encryption_level != SMBC_ENCRYPTLEVEL_NONE) { + signing_state = SMB_SIGNING_REQUIRED; + } + if (port == 0) { if (share == NULL || *share == '\0' || is_ipc) { /* @@ -446,7 +451,7 @@ SMBC_server_internal(TALLOC_CTX *ctx, */ status = cli_connect_nb(server_n, NULL, NBT_SMB_PORT, 0x20, smbc_getNetbiosName(context), - SMB_SIGNING_DEFAULT, flags, &c); + signing_state, flags, &c); } } @@ -456,7 +461,7 @@ SMBC_server_internal(TALLOC_CTX *ctx, */ status = cli_connect_nb(server_n, NULL, port, 0x20, smbc_getNetbiosName(context), - SMB_SIGNING_DEFAULT, flags, &c); + signing_state, flags, &c); } if (!NT_STATUS_IS_OK(status)) { @@ -737,6 +742,7 @@ SMBC_attr_server(TALLOC_CTX *ctx, ipc_srv = SMBC_find_server(ctx, context, server, "*IPC$", pp_workgroup, pp_username, pp_password); if (!ipc_srv) { + int signing_state = SMB_SIGNING_DEFAULT; /* We didn't find a cached connection. Get the password */ if (!*pp_password || (*pp_password)[0] == '\0') { @@ -758,6 +764,9 @@ SMBC_attr_server(TALLOC_CTX *ctx, if (smbc_getOptionUseCCache(context)) { flags |= CLI_FULL_CONNECTION_USE_CCACHE; } + if (context->internal->smb_encryption_level != SMBC_ENCRYPTLEVEL_NONE) { + signing_state = SMB_SIGNING_REQUIRED; + } nt_status = cli_full_connection(&ipc_cli, lp_netbios_name(), server, @@ -766,7 +775,7 @@ SMBC_attr_server(TALLOC_CTX *ctx, *pp_workgroup, *pp_password, flags, - SMB_SIGNING_DEFAULT); + signing_state); if (! NT_STATUS_IS_OK(nt_status)) { DEBUG(1,("cli_full_connection failed! (%s)\n", nt_errstr(nt_status))); -- 1.9.1 From a819d2b440aafa3138d95ff6e8b824da885a70e9 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 30 Sep 2015 21:23:25 +0200 Subject: [PATCH 14/15] CVE-2015-5296: libcli/smb: make sure we require signing when we demand encryption on a session BUG: https://bugzilla.samba.org/show_bug.cgi?id=11536 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison --- libcli/smb/smbXcli_base.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index 6fe4816..505d40d 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -5446,6 +5446,9 @@ uint8_t smb2cli_session_security_mode(struct smbXcli_session *session) if (conn->mandatory_signing) { security_mode |= SMB2_NEGOTIATE_SIGNING_REQUIRED; } + if (session->smb2->should_sign) { + security_mode |= SMB2_NEGOTIATE_SIGNING_REQUIRED; + } return security_mode; } @@ -5877,6 +5880,14 @@ NTSTATUS smb2cli_session_set_channel_key(struct smbXcli_session *session, NTSTATUS smb2cli_session_encryption_on(struct smbXcli_session *session) { + if (!session->smb2->should_sign) { + /* + * We need required signing on the session + * in order to prevent man in the middle attacks. + */ + return NT_STATUS_INVALID_PARAMETER_MIX; + } + if (session->smb2->should_encrypt) { return NT_STATUS_OK; } -- 1.9.1 From b000da128b5fb519d2d3f2e7fd20e4a25b7dae7d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 18 Nov 2015 17:36:21 +1300 Subject: [PATCH 15/15] CVE-2015-8467: samdb: Match MS15-096 behaviour for userAccountControl Swapping between account types is now restricted Bug: https://bugzilla.samba.org/show_bug.cgi?id=11552 Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison Reviewed-by: Ralph Boehme --- source4/dsdb/samdb/ldb_modules/samldb.c | 24 ++++++++- source4/dsdb/tests/python/user_account_control.py | 63 +++++++++++++++++++---- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c index e3a7db2..df285d9 100644 --- a/source4/dsdb/samdb/ldb_modules/samldb.c +++ b/source4/dsdb/samdb/ldb_modules/samldb.c @@ -1558,12 +1558,15 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac, struct security_token *user_token; struct security_descriptor *domain_sd; struct ldb_dn *domain_dn = ldb_get_default_basedn(ldb_module_get_ctx(ac->module)); + struct ldb_context *ldb = ldb_module_get_ctx(ac->module); const struct uac_to_guid { uint32_t uac; + uint32_t priv_to_change_from; const char *oid; const char *guid; enum sec_privilege privilege; bool delete_is_privileged; + bool admin_required; const char *error_string; } map[] = { { @@ -1592,6 +1595,16 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac, .error_string = "Adding the UF_PARTIAL_SECRETS_ACCOUNT bit in userAccountControl requires the DS-Install-Replica right that was not given on the Domain object" }, { + .uac = UF_WORKSTATION_TRUST_ACCOUNT, + .priv_to_change_from = UF_NORMAL_ACCOUNT, + .error_string = "Swapping UF_NORMAL_ACCOUNT to UF_WORKSTATION_TRUST_ACCOUNT requires the user to be a member of the domain admins group" + }, + { + .uac = UF_NORMAL_ACCOUNT, + .priv_to_change_from = UF_WORKSTATION_TRUST_ACCOUNT, + .error_string = "Swapping UF_WORKSTATION_TRUST_ACCOUNT to UF_NORMAL_ACCOUNT requires the user to be a member of the domain admins group" + }, + { .uac = UF_INTERDOMAIN_TRUST_ACCOUNT, .oid = DSDB_CONTROL_PERMIT_INTERDOMAIN_TRUST_UAC_OID, .error_string = "Updating the UF_INTERDOMAIN_TRUST_ACCOUNT bit in userAccountControl is not permitted over LDAP. This bit is restricted to the LSA CreateTrustedDomain interface", @@ -1643,7 +1656,7 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac, return ldb_module_operr(ac->module); } - ret = dsdb_get_sd_from_ldb_message(ldb_module_get_ctx(ac->module), + ret = dsdb_get_sd_from_ldb_message(ldb, ac, res->msgs[0], &domain_sd); if (ret != LDB_SUCCESS) { @@ -1670,12 +1683,19 @@ static int samldb_check_user_account_control_acl(struct samldb_ctx *ac, if (have_priv == false) { ret = LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS; } - } else { + } else if (map[i].priv_to_change_from & user_account_control_old) { + bool is_admin = security_token_has_builtin_administrators(user_token); + if (is_admin == false) { + ret = LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS; + } + } else if (map[i].guid) { ret = acl_check_extended_right(ac, domain_sd, user_token, map[i].guid, SEC_ADS_CONTROL_ACCESS, sid); + } else { + ret = LDB_SUCCESS; } if (ret != LDB_SUCCESS) { break; diff --git a/source4/dsdb/tests/python/user_account_control.py b/source4/dsdb/tests/python/user_account_control.py index 16c7f81..a53c4f9 100644 --- a/source4/dsdb/tests/python/user_account_control.py +++ b/source4/dsdb/tests/python/user_account_control.py @@ -240,6 +240,16 @@ class UserAccountControlTests(samba.tests.TestCase): m.dn = res[0].dn m["userAccountControl"] = ldb.MessageElement(str(samba.dsdb.UF_WORKSTATION_TRUST_ACCOUNT), ldb.FLAG_MOD_REPLACE, "userAccountControl") + try: + self.samdb.modify(m) + self.fail("Unexpectedly able to set userAccountControl to be an Workstation on %s" % m.dn) + except LdbError, (enum, estr): + self.assertEqual(ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS, enum) + + m = ldb.Message() + m.dn = res[0].dn + m["userAccountControl"] = ldb.MessageElement(str(samba.dsdb.UF_NORMAL_ACCOUNT), + ldb.FLAG_MOD_REPLACE, "userAccountControl") self.samdb.modify(m) m = ldb.Message() @@ -306,7 +316,12 @@ class UserAccountControlTests(samba.tests.TestCase): m.dn = res[0].dn m["userAccountControl"] = ldb.MessageElement(str(samba.dsdb.UF_WORKSTATION_TRUST_ACCOUNT), ldb.FLAG_MOD_REPLACE, "userAccountControl") - self.samdb.modify(m) + try: + self.samdb.modify(m) + self.fail("Unexpectedly able to set userAccountControl to be an Workstation on %s" % m.dn) + except LdbError, (enum, estr): + self.assertEqual(ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS, enum) + def test_admin_mod_uac(self): computername=self.computernames[0] @@ -382,9 +397,10 @@ class UserAccountControlTests(samba.tests.TestCase): priv_to_auth_users_bits = set([UF_PASSWD_NOTREQD, UF_ENCRYPTED_TEXT_PASSWORD_ALLOWED, UF_DONT_EXPIRE_PASSWD]) - # These bits really are privileged + # These bits really are privileged, or can't be changed from UF_NORMAL as a non-admin priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT, UF_SERVER_TRUST_ACCOUNT, - UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION]) + UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION, + UF_WORKSTATION_TRUST_ACCOUNT]) invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT, UF_PARTIAL_SECRETS_ACCOUNT]) @@ -446,7 +462,7 @@ class UserAccountControlTests(samba.tests.TestCase): int("0x10000000", 16), int("0x20000000", 16), int("0x40000000", 16), int("0x80000000", 16)]) super_priv_bits = set([UF_INTERDOMAIN_TRUST_ACCOUNT]) - priv_to_remove_bits = set([UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION]) + priv_to_remove_bits = set([UF_TRUSTED_FOR_DELEGATION, UF_TRUSTED_TO_AUTHENTICATE_FOR_DELEGATION, UF_WORKSTATION_TRUST_ACCOUNT]) for bit in bits: # Reset this to the initial position, just to be sure @@ -507,6 +523,31 @@ class UserAccountControlTests(samba.tests.TestCase): except LdbError, (enum, estr): self.fail("Unable to set userAccountControl bit 0x%08X on %s: %s" % (bit, m.dn, estr)) + res = self.admin_samdb.search("%s" % self.base_dn, + expression="(&(objectClass=computer)(samAccountName=%s$))" % computername, + scope=SCOPE_SUBTREE, + attrs=["userAccountControl"]) + + if bit in account_types: + self.assertEqual(int(res[0]["userAccountControl"][0]), + bit|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, + "bit 0X%08x should have been added (0X%08x vs 0X%08x)" + % (bit, int(res[0]["userAccountControl"][0]), + bit|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD)) + elif bit in ignored_bits: + self.assertEqual(int(res[0]["userAccountControl"][0]), + UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, + "bit 0X%08x should have been added (0X%08x vs 0X%08x)" + % (bit, int(res[0]["userAccountControl"][0]), + UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD)) + + else: + self.assertEqual(int(res[0]["userAccountControl"][0]), + bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, + "bit 0X%08x should have been added (0X%08x vs 0X%08x)" + % (bit, int(res[0]["userAccountControl"][0]), + bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD)) + try: m = ldb.Message() m.dn = res[0].dn @@ -520,7 +561,7 @@ class UserAccountControlTests(samba.tests.TestCase): if bit in priv_to_remove_bits: self.assertEqual(enum, ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS) else: - self.fail("Unexpectedly able to remove userAccountControl bit 0x%08X on %s: %s" % (bit, m.dn, estr)) + self.fail("Unexpectedly unable to remove userAccountControl bit 0x%08X on %s: %s" % (bit, m.dn, estr)) res = self.admin_samdb.search("%s" % self.base_dn, expression="(&(objectClass=computer)(samAccountName=%s$))" % computername, @@ -528,9 +569,14 @@ class UserAccountControlTests(samba.tests.TestCase): attrs=["userAccountControl"]) if bit in priv_to_remove_bits: - self.assertEqual(int(res[0]["userAccountControl"][0]), - bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, - "bit 0X%08x should not have been removed" % bit) + if bit in account_types: + self.assertEqual(int(res[0]["userAccountControl"][0]), + bit|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, + "bit 0X%08x should not have been removed" % bit) + else: + self.assertEqual(int(res[0]["userAccountControl"][0]), + bit|UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, + "bit 0X%08x should not have been removed" % bit) else: self.assertEqual(int(res[0]["userAccountControl"][0]), UF_NORMAL_ACCOUNT|UF_ACCOUNTDISABLE|UF_PASSWD_NOTREQD, @@ -553,7 +599,6 @@ class UserAccountControlTests(samba.tests.TestCase): self.sd_utils.dacl_add_ace("OU=test_computer_ou1," + self.base_dn, mod) invalid_bits = set([UF_TEMP_DUPLICATE_ACCOUNT, UF_PARTIAL_SECRETS_ACCOUNT]) - # These bits are privileged, but authenticated users have that CAR by default, so this is a pain to test priv_to_auth_users_bits = set([UF_PASSWD_NOTREQD, UF_ENCRYPTED_TEXT_PASSWORD_ALLOWED, UF_DONT_EXPIRE_PASSWD]) -- 1.9.1