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>

diff --git a/source4/rpc_server/samr/samr_password.c b/source4/rpc_server/samr/samr_password.c
index de2a67c7bdd4908976f64dd64b01b136ede6c524..ac7d473378a7188a2ec93704b26e16fe56fd6d17 100644 (file)
--- 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) {