From f9eb0b248da0689c82656f3e482161c45749afb6 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 16 Jan 2025 16:12:31 -0800 Subject: [PATCH] auth: Cleanup exit code paths in kerberos_decode_pac(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One more memory leak missed and now fixed. tmp_ctx must be freed once the pac data is talloc_move'd. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15782 Signed-off-by: Jeremy Allison Reviewed-by: Jennifer Sutton Reviewed-by: Christian Ambach Reviewed-by: Guenther Deschner Autobuild-User(master): Günther Deschner Autobuild-Date(master): Fri Jan 17 12:01:47 UTC 2025 on atb-devel-224 --- auth/kerberos/kerberos_pac.c | 88 ++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/auth/kerberos/kerberos_pac.c b/auth/kerberos/kerberos_pac.c index 1f7d3e7ef26..4c61cfe838f 100644 --- a/auth/kerberos/kerberos_pac.c +++ b/auth/kerberos/kerberos_pac.c @@ -137,7 +137,7 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, time_t tgs_authtime, struct PAC_DATA **pac_data_out) { - NTSTATUS status; + NTSTATUS status = NT_STATUS_NO_MEMORY; enum ndr_err_code ndr_err; krb5_error_code ret; DATA_BLOB modified_pac_blob; @@ -173,8 +173,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, kdc_sig_wipe = talloc(tmp_ctx, struct PAC_SIGNATURE_DATA); srv_sig_wipe = talloc(tmp_ctx, struct PAC_SIGNATURE_DATA); if (!pac_data_raw || !pac_data || !kdc_sig_wipe || !srv_sig_wipe) { - talloc_free(tmp_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto out; } ndr_err = ndr_pull_struct_blob(&pac_data_blob, pac_data, pac_data, @@ -183,15 +183,14 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't parse the PAC: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } if (pac_data->num_buffers < 4) { /* we need logon_info, service_key and kdc_key */ DEBUG(0,("less than 4 PAC buffers\n")); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } ndr_err = ndr_pull_struct_blob( @@ -201,15 +200,14 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't parse the PAC: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } if (pac_data_raw->num_buffers < 4) { /* we need logon_info, service_key and kdc_key */ DEBUG(0,("less than 4 PAC buffers\n")); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } if (pac_data->num_buffers != pac_data_raw->num_buffers) { @@ -217,8 +215,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, DEBUG(0, ("misparse! PAC_DATA has %d buffers while " "PAC_DATA_RAW has %d\n", pac_data->num_buffers, pac_data_raw->num_buffers)); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } for (i=0; i < pac_data->num_buffers; i++) { @@ -229,8 +227,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, DEBUG(0, ("misparse! PAC_DATA buffer %d has type " "%d while PAC_DATA_RAW has %d\n", i, data_buf->type, raw_buf->type)); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } switch (data_buf->type) { case PAC_TYPE_LOGON_INFO: @@ -263,26 +261,26 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, if (!logon_info) { DEBUG(0,("PAC no logon_info\n")); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } if (!logon_name) { DEBUG(0,("PAC no logon_name\n")); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } if (!srv_sig_ptr || !srv_sig_blob) { DEBUG(0,("PAC no srv_key\n")); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } if (!kdc_sig_ptr || !kdc_sig_blob) { DEBUG(0,("PAC no kdc_key\n")); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } /* Find and zero out the signatures, @@ -297,8 +295,7 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't parse the KDC signature: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } ndr_err = ndr_pull_struct_blob( @@ -308,8 +305,7 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't parse the SRV signature: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } /* Now zero the decoded structure */ @@ -326,8 +322,7 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't repack the KDC signature: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } ndr_err = ndr_push_struct_blob( srv_sig_blob, pac_data_raw, srv_sig_wipe, @@ -336,8 +331,7 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't repack the SRV signature: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } /* push out the whole structure, but now with zero'ed signatures */ @@ -348,8 +342,7 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, status = ndr_map_error2ntstatus(ndr_err); DEBUG(0,("can't repack the RAW PAC: %s\n", nt_errstr(status))); - talloc_free(tmp_ctx); - return status; + goto out; } if (service_keyblock) { @@ -360,8 +353,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, if (ret) { DEBUG(5, ("PAC Decode: Failed to verify the service " "signature: %s\n", error_message(ret))); - talloc_free(tmp_ctx); - return NT_STATUS_ACCESS_DENIED; + status = NT_STATUS_ACCESS_DENIED; + goto out; } if (krbtgt_keyblock) { @@ -371,8 +364,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, if (ret) { DEBUG(1, ("PAC Decode: Failed to verify the KDC signature: %s\n", smb_get_krb5_error_message(context, ret, tmp_ctx))); - talloc_free(tmp_ctx); - return NT_STATUS_ACCESS_DENIED; + status = NT_STATUS_ACCESS_DENIED; + goto out; } } } @@ -388,8 +381,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, nt_time_string(tmp_ctx, logon_name->logon_time))); DEBUG(2, ("PAC Decode: Ticket: %s\n", nt_time_string(tmp_ctx, tgs_authtime_nttime))); - talloc_free(tmp_ctx); - return NT_STATUS_ACCESS_DENIED; + status = NT_STATUS_ACCESS_DENIED; + goto out; } } @@ -401,8 +394,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, if (ret) { DEBUG(2, ("Could not unparse name from ticket to match with name from PAC: [%s]:%s\n", logon_name->account_name, error_message(ret))); - talloc_free(tmp_ctx); - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto out; } bool_ret = strcmp(client_principal_string, logon_name->account_name) == 0; @@ -413,8 +406,8 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, logon_name->account_name, client_principal_string)); SAFE_FREE(client_principal_string); - talloc_free(tmp_ctx); - return NT_STATUS_ACCESS_DENIED; + status = NT_STATUS_ACCESS_DENIED; + goto out; } SAFE_FREE(client_principal_string); @@ -435,10 +428,15 @@ NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx, } if (pac_data_out) { - *pac_data_out = talloc_steal(mem_ctx, pac_data); + *pac_data_out = talloc_move(mem_ctx, &pac_data); } - return NT_STATUS_OK; + status = NT_STATUS_OK; + + out: + + TALLOC_FREE(tmp_ctx); + return status; } NTSTATUS kerberos_pac_logon_info(TALLOC_CTX *mem_ctx,