From 08fc826b72fabe021f299c8924f6b35216aedfa2 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 24 Sep 2013 10:12:24 -0700 Subject: [PATCH 1/4] TODO/REVIEW: dsdb: Set syntax of userParameters to binary string, not unicode string This means we continue to store the values as given on SAMR, assuming that the SAMR buffer is little endian. The syntax for this specific object is forced to be a binary blob, so that it is not converted on DRSUAPI. This commit does not fix existing databases, nor pdb_samba_dsdb (used by classicupgrade). Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=8077 Change-Id: I10bb6aaecc381194e3c0ce6b9163f961acbdcee1 Pair-Programmed-With: Stefan Metzmacher TODO Signed-off-by: Andrew Bartlett Signed-off-by: Stefan Metzmacher --- source4/dsdb/schema/schema.h | 1 + source4/dsdb/schema/schema_syntax.c | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/source4/dsdb/schema/schema.h b/source4/dsdb/schema/schema.h index cac6f98..457d986 100644 --- a/source4/dsdb/schema/schema.h +++ b/source4/dsdb/schema/schema.h @@ -74,6 +74,7 @@ struct dsdb_syntax { const struct dsdb_attribute *attr, const struct ldb_message_element *in); bool auto_normalise; + bool userParameters; /* Indicates the syntax userParameters should be forced to */ }; struct dsdb_attribute { diff --git a/source4/dsdb/schema/schema_syntax.c b/source4/dsdb/schema/schema_syntax.c index c2e0208..c9ff588 100644 --- a/source4/dsdb/schema/schema_syntax.c +++ b/source4/dsdb/schema/schema_syntax.c @@ -2395,6 +2395,7 @@ static const struct dsdb_syntax dsdb_syntaxes[] = { .validate_ldb = dsdb_syntax_DATA_BLOB_validate_ldb, .equality = "octetStringMatch", .comment = "Octet String", + .userParameters = true },{ .name = "String(Sid)", .ldap_oid = LDB_SYNTAX_OCTET_STRING, @@ -2665,6 +2666,16 @@ const struct dsdb_syntax *dsdb_syntax_for_attribute(const struct dsdb_attribute unsigned int i; for (i=0; i < ARRAY_SIZE(dsdb_syntaxes); i++) { + /* + * We must pretend that userParamters was declared + * binary string, so we can store the 'UTF16' (not + * really string) structure as given over SAMR to samba + */ + if (dsdb_syntaxes[i].userParameters && + (strcasecmp(attr->lDAPDisplayName, "userParameters") == 0)) + { + return &dsdb_syntaxes[i]; + } if (attr->oMSyntax != dsdb_syntaxes[i].oMSyntax) continue; if (attr->oMObjectClass.length != dsdb_syntaxes[i].oMObjectClass.length) continue; -- 1.9.1 From 7544ac286af45e71929ea7e410b57372c807a535 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 17 Jun 2014 16:03:22 +1200 Subject: [PATCH 2/4] TODO/REVIEW dsdb: Always store and return the userParameters as a array of LE 16-bit values This is not allowed to be odd length, as otherwise we can not send it over the SAMR transport correctly. Allocating one byte less memory than required causes malloc() heap corruption and then a crash or lockup of the SAMR server. Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=10130 Change-Id: I5c0c531c1d660141e07f884a4789ebe11c1716f6 Pair-Programmed-With: Stefan Metzmacher TODO Signed-off-by: Andrew Bartlett Signed-off-by: Stefan Metzmacher --- source3/passdb/pdb_samba_dsdb.c | 31 +++++++++++++++--- source4/dsdb/common/util.c | 59 ++++++++++++++++++++++++++--------- source4/rpc_server/samr/dcesrv_samr.c | 17 +++++++--- 3 files changed, 84 insertions(+), 23 deletions(-) diff --git a/source3/passdb/pdb_samba_dsdb.c b/source3/passdb/pdb_samba_dsdb.c index 7e7468d..b04e7b2 100644 --- a/source3/passdb/pdb_samba_dsdb.c +++ b/source3/passdb/pdb_samba_dsdb.c @@ -259,9 +259,13 @@ static NTSTATUS pdb_samba_dsdb_init_sam_from_priv(struct pdb_methods *m, pdb_set_workstations(sam, str, PDB_SET); } - str = ldb_msg_find_attr_as_string(msg, "userParameters", - NULL); - if (str != NULL) { + blob = ldb_msg_find_ldb_val(msg, "userParameters"); + if (blob != NULL) { + str = base64_encode_data_blob(frame, *blob); + if (str == NULL) { + DEBUG(0, ("base64_encode_data_blob() failed\n")); + goto fail; + } pdb_set_munged_dial(sam, str, PDB_SET); } @@ -555,8 +559,25 @@ static int pdb_samba_dsdb_replace_by_sam(struct pdb_samba_dsdb_state *state, /* This will need work, it is actually a UTF8 'string' with internal NULLs, to handle TS parameters */ if (need_update(sam, PDB_MUNGEDDIAL)) { - ret |= ldb_msg_add_string(msg, "userParameters", - pdb_get_munged_dial(sam)); + const char *base64_munged_dial = NULL; + + base64_munged_dial = pdb_get_munged_dial(sam); + if (base64_munged_dial != NULL && strlen(base64_munged_dial) > 0) { + struct ldb_val blob; + + blob = base64_decode_data_blob_talloc(msg, + base64_munged_dial); + if (blob.data == NULL) { + DEBUG(0, ("Failed to decode userParameters from " + "munged dialback string[%s] for %s\n", + base64_munged_dial, + ldb_dn_get_linearized(msg->dn))); + talloc_free(frame); + return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX; + } + ret |= ldb_msg_add_steal_value(msg, "userParameters", + &blob); + } } if (need_update(sam, PDB_COUNTRY_CODE)) { diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index 2aa6a6c..18f16f6 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -708,27 +708,42 @@ uint32_t samdb_result_acct_flags(struct ldb_message *msg, const char *attr) return acct_flags; } -struct lsa_BinaryString samdb_result_parameters(TALLOC_CTX *mem_ctx, - struct ldb_message *msg, - const char *attr) +NTSTATUS samdb_result_parameters(TALLOC_CTX *mem_ctx, + struct ldb_message *msg, + const char *attr, + struct lsa_BinaryString *s) { - struct lsa_BinaryString s; + int i; const struct ldb_val *val = ldb_msg_find_ldb_val(msg, attr); - ZERO_STRUCT(s); + ZERO_STRUCTP(s); if (!val) { - return s; + return NT_STATUS_OK; + } + + if ((val->length % 2) != 0) { + /* + * If the on-disk data is not even in length, we know + * it is corrupt, and can not be safely pushed. We + * would either truncate, send either a un-initilaised + * byte or send a forced zero byte + */ + return NT_STATUS_INTERNAL_DB_CORRUPTION; } - s.array = talloc_array(mem_ctx, uint16_t, val->length/2); - if (!s.array) { - return s; + s->array = talloc_array(mem_ctx, uint16_t, val->length/2); + if (!s->array) { + return NT_STATUS_NO_MEMORY; } - s.length = s.size = val->length; - memcpy(s.array, val->data, val->length); + s->length = s->size = val->length; - return s; + /* The on-disk format is the 'network' format, being UTF16LE (sort of) */ + for (i = 0; i < s->length / 2; i++) { + s->array[i] = SVAL(val->data, i * 2); + } + + return NT_STATUS_OK; } /* Find an attribute, with a particular value */ @@ -1036,10 +1051,26 @@ int samdb_msg_add_logon_hours(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, int samdb_msg_add_parameters(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg, const char *attr_name, struct lsa_BinaryString *parameters) { + int i; struct ldb_val val; + if ((parameters->length % 2) != 0) { + return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX; + } + + val.data = talloc_array(mem_ctx, uint8_t, parameters->length); + if (val.data == NULL) { + return LDB_ERR_OPERATIONS_ERROR; + } val.length = parameters->length; - val.data = (uint8_t *)parameters->array; - return ldb_msg_add_value(msg, attr_name, &val, NULL); + for (i = 0; i < parameters->length / 2; i++) { + /* + * The on-disk format needs to be in the 'network' + * format, parmeters->array is a uint16_t array of + * length parameters->length / 2 + */ + SSVAL(val.data, i * 2, parameters->array[i]); + } + return ldb_msg_add_steal_value(msg, attr_name, &val); } /* diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index eacbe7d..c0bec43 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -64,8 +64,6 @@ info->field = samdb_result_logon_hours(mem_ctx, msg, attr); #define QUERY_AFLAGS(msg, field, attr) \ info->field = samdb_result_acct_flags(msg, attr); -#define QUERY_PARAMETERS(msg, field, attr) \ - info->field = samdb_result_parameters(mem_ctx, msg, attr); /* these are used to make the Set[User|Group]Info code easier to follow */ @@ -2700,6 +2698,8 @@ static NTSTATUS dcesrv_samr_QueryUserInfo(struct dcesrv_call_state *dce_call, TA const char * const *attrs = NULL; union samr_UserInfo *info; + NTSTATUS status; + *r->out.info = NULL; DCESRV_PULL_HANDLE(h, r->in.user_handle, SAMR_HANDLE_USER); @@ -3048,7 +3048,11 @@ static NTSTATUS dcesrv_samr_QueryUserInfo(struct dcesrv_call_state *dce_call, TA break; case 20: - QUERY_PARAMETERS(msg, info20.parameters, "userParameters"); + status = samdb_result_parameters(mem_ctx, msg, "userParameters", &info->info20.parameters); + if (!NT_STATUS_IS_OK(status)) { + talloc_free(info); + return status; + } break; case 21: @@ -3067,7 +3071,12 @@ static NTSTATUS dcesrv_samr_QueryUserInfo(struct dcesrv_call_state *dce_call, TA QUERY_STRING(msg, info21.description, "description"); QUERY_STRING(msg, info21.workstations, "userWorkstations"); QUERY_STRING(msg, info21.comment, "comment"); - QUERY_PARAMETERS(msg, info21.parameters, "userParameters"); + status = samdb_result_parameters(mem_ctx, msg, "userParameters", &info->info21.parameters); + if (!NT_STATUS_IS_OK(status)) { + talloc_free(info); + return status; + } + QUERY_RID (msg, info21.rid, "objectSid"); QUERY_UINT (msg, info21.primary_gid, "primaryGroupID"); QUERY_AFLAGS(msg, info21.acct_flags, "msDS-User-Account-Control-Computed"); -- 1.9.1 From 848a48a7070bf0e69411c3bffad6c1abcde6ba2f Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 17 Jun 2014 16:00:57 +1200 Subject: [PATCH 3/4] dbcheck: Add check and test for various invalid userParameters values BUG: https://bugzilla.samba.org/show_bug.cgi?id=8077 Change-Id: I6f2f4169856ce78c62e3a7e74b48520cca9cb9ae Signed-off-by: Andrew Bartlett --- python/samba/dbchecker.py | 90 ++++++++++++++++++ testprogs/blackbox/dbcheck-oldrelease.sh | 154 +++++++++++++++++++++++++++++++ 2 files changed, 244 insertions(+) diff --git a/python/samba/dbchecker.py b/python/samba/dbchecker.py index c658610..74e9678 100644 --- a/python/samba/dbchecker.py +++ b/python/samba/dbchecker.py @@ -20,6 +20,7 @@ import ldb import samba import time +from base64 import b64decode from samba import dsdb from samba import common from samba.dcerpc import misc @@ -64,6 +65,9 @@ class dbcheck(object): self.fix_replmetadata_zero_invocationid = False self.fix_deleted_deleted_objects = False self.fix_dn = False + self.fix_base64_userparameters = False + self.fix_utf8_userparameters = False + self.fix_doubled_userparameters = False self.reset_well_known_acls = reset_well_known_acls self.reset_all_well_known_acls = False self.in_transaction = in_transaction @@ -522,6 +526,58 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) "Failed to correct missing instanceType on %s by setting instanceType=%d" % (obj.dn, calculated_instancetype)): self.report("Corrected instancetype on %s by setting instanceType=%d" % (obj.dn, calculated_instancetype)) + def err_short_userParameters(self, obj, attrname, value): + # This is a truncated userParameters due to a pre 4.1 replication bug + self.report("ERROR: incorrect userParameters value on object %s. If you have another working DC that does not give this warning, please run 'samba-tool drs replicate --full-sync --local %s'" % (obj.dn, self.samdb.get_nc_root(obj.dn))) + + def err_base64_userParameters(self, obj, attrname, value): + '''handle a wrong userParameters''' + self.report("ERROR: wrongly formatted userParameters %s on %s, should not be base64-encoded" % (value, obj.dn)) + if not self.confirm_all('Convert userParameters from base64 encoding on %s?' % (obj.dn), 'fix_base64_userparameters'): + self.report('Not changing userParameters from base64 encoding on %s' % (obj.dn)) + return + + m = ldb.Message() + m.dn = obj.dn + m['value'] = ldb.MessageElement(b64decode(obj[attrname][0]), ldb.FLAG_MOD_REPLACE, 'userParameters') + if self.do_modify(m, [], + "Failed to correct base64-encoded userParameters on %s by converting from base64" % (obj.dn)): + self.report("Corrected base64-encoded userParameters on %s by converting from base64" % (obj.dn)) + + def err_utf8_userParameters(self, obj, attrname, value): + '''handle a wrong userParameters''' + self.report("ERROR: wrongly formatted userParameters on %s, should not be psudo-UTF8 encoded" % (obj.dn)) + if not self.confirm_all('Convert userParameters from UTF8 encoding on %s?' % (obj.dn), 'fix_utf8_userparameters'): + self.report('Not changing userParameters from UTF8 encoding on %s' % (obj.dn)) + return + + m = ldb.Message() + m.dn = obj.dn + m['value'] = ldb.MessageElement(obj[attrname][0].decode('utf8').encode('utf-16-le'), + ldb.FLAG_MOD_REPLACE, 'userParameters') + if self.do_modify(m, [], + "Failed to correct psudo-UTF8 encoded userParameters on %s by converting from UTF8" % (obj.dn)): + self.report("Corrected psudo-UTF8 encoded userParameters on %s by converting from UTF8" % (obj.dn)) + + def err_doubled_userParameters(self, obj, attrname, value): + '''handle a wrong userParameters''' + self.report("ERROR: wrongly formatted userParameters on %s, should not be double UTF16 encoded" % (obj.dn)) + if not self.confirm_all('Convert userParameters from doubled UTF-16 encoding on %s?' % (obj.dn), 'fix_doubled_userparameters'): + self.report('Not changing userParameters from doubled UTF-16 encoding on %s' % (obj.dn)) + return + + m = ldb.Message() + m.dn = obj.dn + m['value'] = ldb.MessageElement(obj[attrname][0].decode('utf-16-le').decode('utf-16-le').encode('utf-16-le'), + ldb.FLAG_MOD_REPLACE, 'userParameters') + if self.do_modify(m, [], + "Failed to correct doubled-UTF16 encoded userParameters on %s by converting" % (obj.dn)): + self.report("Corrected doubled-UTF16 encoded userParameters on %s by converting" % (obj.dn)) + + def err_odd_userParameters(self, obj, attrname): + # This is a truncated userParameters due to a pre 4.1 replication bug + self.report("ERROR: incorrect userParameters value on object %s (odd length). If you have another working DC that does not give this warning, please run 'samba-tool drs replicate --full-sync --local %s'" % (obj.dn, self.samdb.get_nc_root(obj.dn))) + def find_revealed_link(self, dn, attrname, guid): '''return a revealed link in an object''' res = self.samdb.search(base=dn, scope=ldb.SCOPE_BASE, attrs=[attrname], @@ -1164,6 +1220,40 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) error_count += 1 continue + if str(attrname).lower() == 'userparameters': + if len(obj[attrname][0]) == 1 and obj[attrname][0][0] == '\x20': + error_count += 1 + self.err_short_userParameters(obj, attrname, obj[attrname]) + continue + + elif obj[attrname][0][:16] == '\x20\x00\x20\x00\x20\x00\x20\x00\x20\x00\x20\x00\x20\x00\x20\x00': + # This is the correct, normal prefix + continue + + elif obj[attrname][0][:20] == 'IAAgACAAIAAgACAAIAAg': + # this is the typical prefix from a windows migration + error_count += 1 + self.err_base64_userParameters(obj, attrname, obj[attrname]) + continue + + elif obj[attrname][0][1] != '\x00' and obj[attrname][0][3] != '\x00' and obj[attrname][0][5] != '\x00' and obj[attrname][0][7] != '\x00' and obj[attrname][0][9] != '\x00': + # This is a prefix that is not in UTF-16 format for the space or munged dialback prefix + error_count += 1 + self.err_utf8_userParameters(obj, attrname, obj[attrname]) + continue + + elif len(obj[attrname][0]) % 2 != 0: + # This is a value that isn't even in length + error_count += 1 + self.err_odd_userParameters(obj, attrname, obj[attrname]) + continue + + elif obj[attrname][0][1] == '\x00' and obj[attrname][0][2] == '\x00' and obj[attrname][0][3] == '\x00' and obj[attrname][0][4] != '\x00' and obj[attrname][0][5] == '\x00': + # This is a prefix that would happen if a SAMR-written value was replicated from a Samba 4.1 server to a working server + error_count += 1 + self.err_doubled_userParameters(obj, attrname, obj[attrname]) + continue + # check for empty attributes for val in obj[attrname]: if val == '': diff --git a/testprogs/blackbox/dbcheck-oldrelease.sh b/testprogs/blackbox/dbcheck-oldrelease.sh index fbc9e9b..d59c8a8 100755 --- a/testprogs/blackbox/dbcheck-oldrelease.sh +++ b/testprogs/blackbox/dbcheck-oldrelease.sh @@ -15,6 +15,11 @@ shift 2 release_dir=`dirname $0`/../../source4/selftest/provisions/$RELEASE +ldbmodify="ldbmodify" +if [ -x "$BINDIR/ldbmodify" ]; then + ldbmodify="$BINDIR/ldbmodify" +fi + undump() { if test -x $BINDIR/tdbrestore; then @@ -24,6 +29,109 @@ undump() { fi } +add_userparameters0() { + if [ x$RELEASE = x"release-4-1-0rc3" ]; then + $ldbmodify -H tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb < Date: Tue, 8 Jul 2014 16:19:09 +0200 Subject: [PATCH 4/4] TODO/REVIEW s4:dsdb/samldb: don't allow 'userParameters' to be modified over LDAP for now For now it's safer to reject setting 'userParameters' via LDAP, as we'll not provide the same behavior as a Windows Server. If someone requires that feature please report this in the following bug reports! Bug: https://bugzilla.samba.org/show_bug.cgi?id=8077 Bug: https://bugzilla.samba.org/show_bug.cgi?id=10130 Signed-off-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/samldb.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c index ad3d4da..944dca6 100644 --- a/source4/dsdb/samdb/ldb_modules/samldb.c +++ b/source4/dsdb/samdb/ldb_modules/samldb.c @@ -2341,6 +2341,15 @@ static int samldb_add(struct ldb_module *module, struct ldb_request *req) return ldb_next_request(module, req); } + el = ldb_msg_find_element(req->op.add.message, "userParameters"); + if (el != NULL && ldb_req_is_untrusted(req)) { + const char *reason = "samldb_add: " + "setting userParameter is not supported over LDAP, " + "see https://bugzilla.samba.org/show_bug.cgi?id=8077"; + ldb_debug(ldb, LDB_DEBUG_WARNING, "%s", reason); + return ldb_error(ldb, LDB_ERR_CONSTRAINT_VIOLATION, reason); + } + ac = samldb_ctx_init(module, req); if (ac == NULL) { return ldb_operr(ldb); @@ -2480,6 +2489,15 @@ static int samldb_modify(struct ldb_module *module, struct ldb_request *req) } } + el = ldb_msg_find_element(req->op.mod.message, "userParameters"); + if (el != NULL && ldb_req_is_untrusted(req)) { + const char *reason = "samldb: " + "setting userParameter is not supported over LDAP, " + "see https://bugzilla.samba.org/show_bug.cgi?id=8077"; + ldb_debug(ldb, LDB_DEBUG_WARNING, "%s", reason); + return ldb_error(ldb, LDB_ERR_CONSTRAINT_VIOLATION, reason); + } + ac = samldb_ctx_init(module, req); if (ac == NULL) { return ldb_operr(ldb); -- 1.9.1