Commit 2a5d0006 authored by Leonardo Arena's avatar Leonardo Arena
Browse files

main/samba: securiti fixes

CVE-2018-10858, CVE-2018-10918, CVE-2018-10919, CVE-2018-1139

Fixes #9251
parent 8c6e5428
From 02db55b4074e0ceebb87a75105e8ef79c3dcf032 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra@samba.org>
Date: Fri, 15 Jun 2018 15:07:17 -0700
Subject: [PATCH] CVE-2018-10858: libsmb: Ensure smbc_urlencode() can't
overwrite passed in buffer.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13453
CVE-2018-10858: Insufficient input validation on client directory
listing in libsmbclient.
Signed-off-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
---
source3/libsmb/libsmb_path.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/source3/libsmb/libsmb_path.c b/source3/libsmb/libsmb_path.c
index 01b0a61e483..ed70ab37550 100644
--- a/source3/libsmb/libsmb_path.c
+++ b/source3/libsmb/libsmb_path.c
@@ -173,8 +173,13 @@ smbc_urlencode(char *dest,
}
}
- *dest++ = '\0';
- max_dest_len--;
+ if (max_dest_len == 0) {
+ /* Ensure we return -1 if no null termination. */
+ return -1;
+ }
+
+ *dest++ = '\0';
+ max_dest_len--;
return max_dest_len;
}
--
2.18.0
From 49d940f8e335b8af6daf65ac6d3cce45db09ca8e Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Mon, 30 Jul 2018 14:00:18 +1200
Subject: [PATCH] CVE-2018-10918: cracknames: Fix DoS (NULL pointer de-ref)
when not servicePrincipalName is set on a user
This regression was introduced in Samba 4.7 by bug 12842 and in
master git commit eb2e77970e41c1cb62c041877565e939c78ff52d.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13552
CVE-2018-10918: Denial of Service Attack on AD DC DRSUAPI server.
Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
---
source4/dsdb/samdb/cracknames.c | 8 ++++-
source4/torture/drs/python/cracknames.py | 38 ++++++++++++++++++++++++
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/source4/dsdb/samdb/cracknames.c b/source4/dsdb/samdb/cracknames.c
index d43f510b949..3b215ac0ec9 100644
--- a/source4/dsdb/samdb/cracknames.c
+++ b/source4/dsdb/samdb/cracknames.c
@@ -1253,7 +1253,13 @@ static WERROR DsCrackNameOneFilter(struct ldb_context *sam_ctx, TALLOC_CTX *mem_
return WERR_OK;
}
case DRSUAPI_DS_NAME_FORMAT_SERVICE_PRINCIPAL: {
- if (result->elements[0].num_values > 1) {
+ struct ldb_message_element *el
+ = ldb_msg_find_element(result,
+ "servicePrincipalName");
+ if (el == NULL) {
+ info1->status = DRSUAPI_DS_NAME_STATUS_NOT_FOUND;
+ return WERR_OK;
+ } else if (el->num_values > 1) {
info1->status = DRSUAPI_DS_NAME_STATUS_NOT_UNIQUE;
return WERR_OK;
}
diff --git a/source4/torture/drs/python/cracknames.py b/source4/torture/drs/python/cracknames.py
index d8c8ae53d60..9bf90f9c997 100644
--- a/source4/torture/drs/python/cracknames.py
+++ b/source4/torture/drs/python/cracknames.py
@@ -149,6 +149,44 @@ class DrsCracknamesTestCase(drs_base.DrsBaseTestCase):
self.ldb_dc1.delete(user)
+ def test_NoSPNAttribute(self):
+ """
+ Verifies that, if we try and cracknames with the desired output
+ being an SPN, it returns
+ DRSUAPI_DS_NAME_STATUS_NOT_UNIQUE.
+ """
+ username = "Cracknames_no_SPN"
+ user = "cn=%s,%s" % (username, self.ou)
+
+ user_record = {
+ "dn": user,
+ "objectclass": "user",
+ "sAMAccountName" : username,
+ "userPrincipalName" : "test4@test.com",
+ "displayName" : "test4"}
+
+ self.ldb_dc1.add(user_record)
+
+ (result, ctr) = self._do_cracknames(user,
+ drsuapi.DRSUAPI_DS_NAME_FORMAT_FQDN_1779,
+ drsuapi.DRSUAPI_DS_NAME_FORMAT_GUID)
+
+ self.assertEquals(ctr.count, 1)
+ self.assertEquals(ctr.array[0].status,
+ drsuapi.DRSUAPI_DS_NAME_STATUS_OK)
+
+ user_guid = ctr.array[0].result_name
+
+ (result, ctr) = self._do_cracknames(user_guid,
+ drsuapi.DRSUAPI_DS_NAME_FORMAT_GUID,
+ drsuapi.DRSUAPI_DS_NAME_FORMAT_SERVICE_PRINCIPAL)
+
+ self.assertEquals(ctr.count, 1)
+ self.assertEquals(ctr.array[0].status,
+ drsuapi.DRSUAPI_DS_NAME_STATUS_NOT_FOUND)
+
+ self.ldb_dc1.delete(user)
+
def _do_cracknames(self, name, format_offered, format_desired):
req = drsuapi.DsNameRequest1()
names = drsuapi.DsNameString()
--
2.18.0
From 12f97f9f69d3ace751c9b49f739aecc4e452dd35 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale@catalyst.net.nz>
Date: Thu, 19 Jul 2018 16:03:36 +1200
Subject: [PATCH] CVE-2018-10919 security: Move object-specific access checks
into separate function
Object-specific access checks refer to a specific section of the
MS-ADTS, and the code closely matches the spec. We need to extend this
logic to properly handle the Control-Access Right (CR), so it makes
sense to split the logic out into its own function.
This patch just moves the code, and should not alter the logic (apart
from ading in the boolean grant_access return variable.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13434
Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
---
libcli/security/access_check.c | 86 +++++++++++++++++++++++-----------
1 file changed, 59 insertions(+), 27 deletions(-)
diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
index b4c850b613e..b4e62441542 100644
--- a/libcli/security/access_check.c
+++ b/libcli/security/access_check.c
@@ -374,6 +374,57 @@ static const struct GUID *get_ace_object_type(struct security_ace *ace)
return NULL;
}
+/**
+ * Evaluates access rights specified in a object-specific ACE for an AD object.
+ * This logic corresponds to MS-ADTS 5.1.3.3.3 Checking Object-Specific Access.
+ * @param[in] ace - the ACE being processed
+ * @param[in/out] tree - remaining_access gets updated for the tree
+ * @param[out] grant_access - set to true if the ACE grants sufficient access
+ * rights to the object/attribute
+ * @returns NT_STATUS_OK, unless access was denied
+ */
+static NTSTATUS check_object_specific_access(struct security_ace *ace,
+ struct object_tree *tree,
+ bool *grant_access)
+{
+ struct object_tree *node = NULL;
+ const struct GUID *type = NULL;
+
+ *grant_access = false;
+
+ /*
+ * check only in case we have provided a tree,
+ * the ACE has an object type and that type
+ * is in the tree
+ */
+ type = get_ace_object_type(ace);
+
+ if (!tree) {
+ return NT_STATUS_OK;
+ }
+
+ if (!type) {
+ node = tree;
+ } else {
+ if (!(node = get_object_tree_by_GUID(tree, type))) {
+ return NT_STATUS_OK;
+ }
+ }
+
+ if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
+ object_tree_modify_access(node, ace->access_mask);
+ if (node->remaining_access == 0) {
+ *grant_access = true;
+ return NT_STATUS_OK;
+ }
+ } else {
+ if (node->remaining_access & ace->access_mask){
+ return NT_STATUS_ACCESS_DENIED;
+ }
+ }
+ return NT_STATUS_OK;
+}
+
/**
* @brief Perform directoryservice (DS) related access checks for a given user
*
@@ -405,8 +456,6 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
{
uint32_t i;
uint32_t bits_remaining;
- struct object_tree *node;
- const struct GUID *type;
struct dom_sid self_sid;
dom_sid_parse(SID_NT_SELF, &self_sid);
@@ -456,6 +505,8 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
for (i=0; bits_remaining && i < sd->dacl->num_aces; i++) {
struct dom_sid *trustee;
struct security_ace *ace = &sd->dacl->aces[i];
+ NTSTATUS status;
+ bool grant_access = false;
if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY) {
continue;
@@ -486,34 +537,15 @@ NTSTATUS sec_access_check_ds(const struct security_descriptor *sd,
break;
case SEC_ACE_TYPE_ACCESS_DENIED_OBJECT:
case SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT:
- /*
- * check only in case we have provided a tree,
- * the ACE has an object type and that type
- * is in the tree
- */
- type = get_ace_object_type(ace);
-
- if (!tree) {
- continue;
- }
+ status = check_object_specific_access(ace, tree,
+ &grant_access);
- if (!type) {
- node = tree;
- } else {
- if (!(node = get_object_tree_by_GUID(tree, type))) {
- continue;
- }
+ if (!NT_STATUS_IS_OK(status)) {
+ return status;
}
- if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
- object_tree_modify_access(node, ace->access_mask);
- if (node->remaining_access == 0) {
- return NT_STATUS_OK;
- }
- } else {
- if (node->remaining_access & ace->access_mask){
- return NT_STATUS_ACCESS_DENIED;
- }
+ if (grant_access) {
+ return NT_STATUS_OK;
}
break;
default: /* Other ACE types not handled/supported */
--
2.18.0
From c25460ee1f1b10bf69eaaf1ac937da225854d1d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd@samba.org>
Date: Tue, 13 Mar 2018 16:56:20 +0100
Subject: [PATCH] CVE-2018-1139 libcli/auth: Do not allow ntlmv1 over SMB1 when
it is disabled via "ntlm auth".
This fixes a regression that came in via 00db3aba6cf9ebaafdf39ee2f9c7ba5ec2281ea0.
Found by Vivek Das <vdas@redhat.com> (Red Hat QE).
In order to demonstrate simply run:
smbclient //server/share -U user%password -mNT1 -c quit \
--option="client ntlmv2 auth"=no \
--option="client use spnego"=no
against a server that uses "ntlm auth = ntlmv2-only" (our default
setting).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13360
CVE-2018-1139: Weak authentication protocol allowed.
Guenther
Pair-Programmed-With: Stefan Metzmacher <metze@samba.org>
Signed-off-by: Guenther Deschner <gd@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
---
libcli/auth/ntlm_check.c | 2 +-
selftest/knownfail | 3 ++-
selftest/knownfail.d/ntlm | 2 --
3 files changed, 3 insertions(+), 4 deletions(-)
delete mode 100644 selftest/knownfail.d/ntlm
diff --git a/libcli/auth/ntlm_check.c b/libcli/auth/ntlm_check.c
index 1c6499bd210..b68e9c87888 100644
--- a/libcli/auth/ntlm_check.c
+++ b/libcli/auth/ntlm_check.c
@@ -572,7 +572,7 @@ NTSTATUS ntlm_password_check(TALLOC_CTX *mem_ctx,
- I think this is related to Win9X pass-though authentication
*/
DEBUG(4,("ntlm_password_check: Checking NT MD4 password in LM field\n"));
- if (ntlm_auth) {
+ if (ntlm_auth == NTLM_AUTH_ON) {
if (smb_pwd_check_ntlmv1(mem_ctx,
lm_response,
stored_nt->hash, challenge,
diff --git a/selftest/knownfail b/selftest/knownfail
index 21ef797ec0f..baf3d57a31a 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -304,8 +304,9 @@
^samba4.smb.signing.*disabled.*signing=off.*\(ad_dc\)
# fl2000dc doesn't support AES
^samba4.krb5.kdc.*as-req-aes.*fl2000dc
-# nt4_member and ad_member don't support ntlmv1
+# nt4_member and ad_member don't support ntlmv1 (not even over SMB1)
^samba3.blackbox.smbclient_auth.plain.*_member.*option=clientntlmv2auth=no.member.creds.*as.user
+^samba3.blackbox.smbclient_auth.plain.*_member.*option=clientntlmv2auth=no.*mNT1.member.creds.*as.user
#nt-vfs server blocks read with execute access
^samba4.smb2.read.access
#ntvfs server blocks copychunk with execute access on read handle
--
2.18.0
From 011d25d5f653246770fa58b7dcff26740369c6ef Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra@samba.org>
Date: Fri, 15 Jun 2018 15:08:17 -0700
Subject: [PATCH] CVE-2018-10858: libsmb: Harden smbc_readdir_internal()
against returns from malicious servers.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13453
CVE-2018-10858: Insufficient input validation on client directory
listing in libsmbclient.
Signed-off-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
---
source3/libsmb/libsmb_dir.c | 57 +++++++++++++++++++++++++++++++-----
source3/libsmb/libsmb_path.c | 2 +-
2 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/source3/libsmb/libsmb_dir.c b/source3/libsmb/libsmb_dir.c
index 72441c46736..54c2bcb3c73 100644
--- a/source3/libsmb/libsmb_dir.c
+++ b/source3/libsmb/libsmb_dir.c
@@ -943,27 +943,47 @@ SMBC_closedir_ctx(SMBCCTX *context,
}
-static void
+static int
smbc_readdir_internal(SMBCCTX * context,
struct smbc_dirent *dest,
struct smbc_dirent *src,
int max_namebuf_len)
{
if (smbc_getOptionUrlEncodeReaddirEntries(context)) {
+ int remaining_len;
/* url-encode the name. get back remaining buffer space */
- max_namebuf_len =
+ remaining_len =
smbc_urlencode(dest->name, src->name, max_namebuf_len);
+ /* -1 means no null termination. */
+ if (remaining_len < 0) {
+ return -1;
+ }
+
/* We now know the name length */
dest->namelen = strlen(dest->name);
+ if (dest->namelen + 1 < 1) {
+ /* Integer wrap. */
+ return -1;
+ }
+
+ if (dest->namelen + 1 >= max_namebuf_len) {
+ /* Out of space for comment. */
+ return -1;
+ }
+
/* Save the pointer to the beginning of the comment */
dest->comment = dest->name + dest->namelen + 1;
+ if (remaining_len < 1) {
+ /* No room for comment null termination. */
+ return -1;
+ }
+
/* Copy the comment */
- strncpy(dest->comment, src->comment, max_namebuf_len - 1);
- dest->comment[max_namebuf_len - 1] = '\0';
+ strlcpy(dest->comment, src->comment, remaining_len);
/* Save other fields */
dest->smbc_type = src->smbc_type;
@@ -973,10 +993,21 @@ smbc_readdir_internal(SMBCCTX * context,
} else {
/* No encoding. Just copy the entry as is. */
+ if (src->dirlen > max_namebuf_len) {
+ return -1;
+ }
memcpy(dest, src, src->dirlen);
+ if (src->namelen + 1 < 1) {
+ /* Integer wrap */
+ return -1;
+ }
+ if (src->namelen + 1 >= max_namebuf_len) {
+ /* Comment off the end. */
+ return -1;
+ }
dest->comment = (char *)(&dest->name + src->namelen + 1);
}
-
+ return 0;
}
/*
@@ -988,6 +1019,7 @@ SMBC_readdir_ctx(SMBCCTX *context,
SMBCFILE *dir)
{
int maxlen;
+ int ret;
struct smbc_dirent *dirp, *dirent;
TALLOC_CTX *frame = talloc_stackframe();
@@ -1037,7 +1069,12 @@ SMBC_readdir_ctx(SMBCCTX *context,
dirp = &context->internal->dirent;
maxlen = sizeof(context->internal->_dirent_name);
- smbc_readdir_internal(context, dirp, dirent, maxlen);
+ ret = smbc_readdir_internal(context, dirp, dirent, maxlen);
+ if (ret == -1) {
+ errno = EINVAL;
+ TALLOC_FREE(frame);
+ return NULL;
+ }
dir->dir_next = dir->dir_next->next;
@@ -1095,6 +1132,7 @@ SMBC_getdents_ctx(SMBCCTX *context,
*/
while ((dirlist = dir->dir_next)) {
+ int ret;
struct smbc_dirent *dirent;
struct smbc_dirent *currentEntry = (struct smbc_dirent *)ndir;
@@ -1109,8 +1147,13 @@ SMBC_getdents_ctx(SMBCCTX *context,
/* Do urlencoding of next entry, if so selected */
dirent = &context->internal->dirent;
maxlen = sizeof(context->internal->_dirent_name);
- smbc_readdir_internal(context, dirent,
+ ret = smbc_readdir_internal(context, dirent,
dirlist->dirent, maxlen);
+ if (ret == -1) {
+ errno = EINVAL;
+ TALLOC_FREE(frame);
+ return -1;
+ }
reqd = dirent->dirlen;
diff --git a/source3/libsmb/libsmb_path.c b/source3/libsmb/libsmb_path.c
index ed70ab37550..5b53b386a67 100644
--- a/source3/libsmb/libsmb_path.c
+++ b/source3/libsmb/libsmb_path.c
@@ -173,7 +173,7 @@ smbc_urlencode(char *dest,
}
}
- if (max_dest_len == 0) {
+ if (max_dest_len <= 0) {
/* Ensure we return -1 if no null termination. */
return -1;
}
--
2.18.0
From 81865e8584a0f597650a9df31d49bad3e7549d26 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale@catalyst.net.nz>
Date: Fri, 20 Jul 2018 13:13:50 +1200
Subject: [PATCH] CVE-2018-10919 security: Add more comments to the
object-specific access checks
Reading the spec and then reading the code makes sense, but we could
comment the code more so it makes sense on its own.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13434
Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
---
libcli/security/access_check.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
index b4e62441542..93eb85def91 100644
--- a/libcli/security/access_check.c
+++ b/libcli/security/access_check.c
@@ -392,32 +392,46 @@ static NTSTATUS check_object_specific_access(struct security_ace *ace,
*grant_access = false;
- /*
- * check only in case we have provided a tree,
- * the ACE has an object type and that type
- * is in the tree
- */
- type = get_ace_object_type(ace);
-
+ /* if no tree was supplied, we can't do object-specific access checks */
if (!tree) {
return NT_STATUS_OK;
}
+ /* Get the ObjectType GUID this ACE applies to */
+ type = get_ace_object_type(ace);
+
+ /*
+ * If the ACE doesn't have a type, then apply it to the whole tree, i.e.
+ * treat 'OA' ACEs as 'A' and 'OD' as 'D'
+ */
if (!type) {
node = tree;
} else {
- if (!(node = get_object_tree_by_GUID(tree, type))) {
+
+ /* skip it if the ACE's ObjectType GUID is not in the tree */
+ node = get_object_tree_by_GUID(tree, type);
+ if (!node) {
return NT_STATUS_OK;
}
}
if (ace->type == SEC_ACE_TYPE_ACCESS_ALLOWED_OBJECT) {
+
+ /* apply the access rights to this node, and any children */
object_tree_modify_access(node, ace->access_mask);
+
+ /*
+ * Currently all nodes in the tree request the same access mask,
+ * so we can use any node to check if processing this ACE now
+ * means the requested access has been granted
+ */
if (node->remaining_access == 0) {
*grant_access = true;
return NT_STATUS_OK;
}
} else {
+
+ /* this ACE denies access to the requested object/attribute */
if (node->remaining_access & ace->access_mask){
return NT_STATUS_ACCESS_DENIED;
}
--
2.18.0
From 1d89fe91a7336950b1ba84b8680f015e228047f5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd@samba.org>
Date: Wed, 14 Mar 2018 15:36:05 +0100
Subject: [PATCH 1/1] CVE-2018-1139 libcli/auth: fix debug messages in
hash_password_check()
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13360
CVE-2018-1139: Weak authentication protocol allowed.
Guenther
Signed-off-by: Guenther Deschner <gd@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>