CVE-2013-4496:Revert remainder of ce895609b04380bfc41e4f8fddc84bd2f9324340

Part of this was removed when ChangePasswordUser was unimplemented,
but remove the remainder of this flawed commit.  Fully check the
password first, as extract_pw_from_buffer() already does a partial
check of the password because it needs a correct old password to
correctly decrypt the length.

Andrew Bartlett

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10245

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>

1 files changed, 35 insertions, 34 deletions

diff --git a/source4/rpc_server/samr/samr_password.c b/source4/rpc_server/samr/samr_password.c
index 9d6c9212f47..685a8e7864a 100644
--- a/source4/rpc_server/samr/samr_password.c
+++ b/source4/rpc_server/samr/samr_password.c
@@ -142,6 +142,9 @@ NTSTATUS dcesrv_samr_OemChangePasswordUser2(struct dcesrv_call_state *dce_call,
 
 	E_deshash(new_pass, new_lm_hash);
 	E_old_pw_hash(new_lm_hash, lm_pwd->hash, lm_verifier.hash);
+	if (memcmp(lm_verifier.hash, r->in.hash->hash, 16) != 0) {
+		return NT_STATUS_WRONG_PASSWORD;
+	}
 
 	/* Connect to a SAMDB with user privileges for the password change */
 	sam_ctx = samdb_connect(mem_ctx, dce_call->event_ctx,
@@ -173,11 +176,6 @@ NTSTATUS dcesrv_samr_OemChangePasswordUser2(struct dcesrv_call_state *dce_call,
 		return status;
 	}
 
-	if (memcmp(lm_verifier.hash, r->in.hash->hash, 16) != 0) {
-		ldb_transaction_cancel(sam_ctx);
-		return NT_STATUS_WRONG_PASSWORD;
-	}
-
 	/* And this confirms it in a transaction commit */
 	ret = ldb_transaction_commit(sam_ctx);
 	if (ret != LDB_SUCCESS) {
@@ -267,33 +265,8 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call,
 		goto failed;
 	}
 
-	/* Connect to a SAMDB with user privileges for the password change */
-	sam_ctx = samdb_connect(mem_ctx, dce_call->event_ctx,
-				dce_call->conn->dce_ctx->lp_ctx,
-				dce_call->conn->auth_state.session_info, 0);
-	if (sam_ctx == NULL) {
-		return NT_STATUS_INVALID_SYSTEM_SERVICE;
-	}
-
-	ret = ldb_transaction_start(sam_ctx);
-	if (ret != LDB_SUCCESS) {
-		DEBUG(1, ("Failed to start transaction: %s\n", ldb_errstring(sam_ctx)));
-		return NT_STATUS_TRANSACTION_ABORTED;
-	}
-
-	/* Performs the password modification. We pass the old hashes read out
-	 * from the database since they were already checked against the user-
-	 * provided ones. */
-	status = samdb_set_password(sam_ctx, mem_ctx,
-				    user_dn, NULL,
-				    &new_password,
-				    NULL, NULL,
-				    lm_pwd, nt_pwd, /* this is a user password change */
-				    &reason,
-				    &dominfo);
-
-	if (!NT_STATUS_IS_OK(status)) {
-		ldb_transaction_cancel(sam_ctx);
+	if (r->in.nt_verifier == NULL) {
+		status = NT_STATUS_WRONG_PASSWORD;
 		goto failed;
 	}
 
@@ -302,7 +275,6 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call,
 
 	E_old_pw_hash(new_nt_hash, nt_pwd->hash, nt_verifier.hash);
 	if (memcmp(nt_verifier.hash, r->in.nt_verifier->hash, 16) != 0) {
-		ldb_transaction_cancel(sam_ctx);
 		status = NT_STATUS_WRONG_PASSWORD;
 		goto failed;
 	}
@@ -322,13 +294,42 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call,
 			E_deshash(new_pass, new_lm_hash);
 			E_old_pw_hash(new_nt_hash, lm_pwd->hash, lm_verifier.hash);
 			if (memcmp(lm_verifier.hash, r->in.lm_verifier->hash, 16) != 0) {
-				ldb_transaction_cancel(sam_ctx);
 				status = NT_STATUS_WRONG_PASSWORD;
 				goto failed;
 			}
 		}
 	}
 
+	/* Connect to a SAMDB with user privileges for the password change */
+	sam_ctx = samdb_connect(mem_ctx, dce_call->event_ctx,
+				dce_call->conn->dce_ctx->lp_ctx,
+				dce_call->conn->auth_state.session_info, 0);
+	if (sam_ctx == NULL) {
+		return NT_STATUS_INVALID_SYSTEM_SERVICE;
+	}
+
+	ret = ldb_transaction_start(sam_ctx);
+	if (ret != LDB_SUCCESS) {
+		DEBUG(1, ("Failed to start transaction: %s\n", ldb_errstring(sam_ctx)));
+		return NT_STATUS_TRANSACTION_ABORTED;
+	}
+
+	/* Performs the password modification. We pass the old hashes read out
+	 * from the database since they were already checked against the user-
+	 * provided ones. */
+	status = samdb_set_password(sam_ctx, mem_ctx,
+				    user_dn, NULL,
+				    &new_password,
+				    NULL, NULL,
+				    lm_pwd, nt_pwd, /* this is a user password change */
+				    &reason,
+				    &dominfo);
+
+	if (!NT_STATUS_IS_OK(status)) {
+		ldb_transaction_cancel(sam_ctx);
+		goto failed;
+	}
+
 	/* And this confirms it in a transaction commit */
 	ret = ldb_transaction_commit(sam_ctx);
 	if (ret != LDB_SUCCESS) {