[PATCH] Add missing KDC "status" on some fail conditions

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Add missing KDC "status" on some fail conditions

Samuel Cabrero
The process_tgs_req function requires the status string to be set
for all fail conditions before jump to "cleanup" label, otherwise
it will cause an assertion failure.

Signed-off-by: Samuel Cabrero <[hidden email]>
---
 src/kdc/do_tgs_req.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/kdc/do_tgs_req.c b/src/kdc/do_tgs_req.c
index 547a41441..d13160c00 100644
--- a/src/kdc/do_tgs_req.c
+++ b/src/kdc/do_tgs_req.c
@@ -253,8 +253,10 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
 
     errcode = search_sprinc(kdc_active_realm, request, s_flags, &server,
                             &status);
-    if (errcode != 0)
+    if (errcode != 0) {
+        status = "SEARCH_SPRINC";
         goto cleanup;
+    }
     sprinc = server->princ;
 
     /* If we got a cross-realm TGS which is not the requested server, we are
@@ -304,8 +306,10 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
         au_state->s4u2self_user = NULL;
     }
 
-    if (errcode)
+    if (errcode) {
+        status = "PROCESS_S4U2SELF";
         goto cleanup;
+    }
     if (s4u_x509_user != NULL) {
         setflag(c_flags, KRB5_KDB_FLAG_PROTOCOL_TRANSITION);
         if (is_referral) {
@@ -320,8 +324,10 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
     /* Deal with user-to-user and constrained delegation */
     errcode = decrypt_2ndtkt(kdc_active_realm, request, c_flags,
                              &stkt_server, &status);
-    if (errcode)
+    if (errcode) {
+        status = "DECRYPT_2NDTKT";
         goto cleanup;
+    }
 
     if (isflagset(request->kdc_options, KDC_OPT_CNAME_IN_ADDL_TKT)) {
         /* Do constrained delegation protocol and authorization checks */
@@ -345,8 +351,10 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
             goto cleanup;
         }
         kau_s4u2proxy(kdc_context, errcode ? FALSE : TRUE, au_state);
-        if (errcode)
+        if (errcode) {
+            status = "KAU_S4U2PROXY";
             goto cleanup;
+        }
 
         setflag(c_flags, KRB5_KDB_FLAG_CONSTRAINED_DELEGATION);
 
@@ -365,8 +373,10 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
 
     errcode = gen_session_key(kdc_active_realm, request, server, &session_key,
                               &status);
-    if (errcode)
+    if (errcode) {
+        status = "GEN_SESSION_KEY";
         goto cleanup;
+    }
 
     /*
      * subject_tkt will refer to the evidence ticket (for constrained
@@ -745,8 +755,10 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
             au_state->status = status;
         }
         kau_s4u2self(kdc_context, errcode ? FALSE : TRUE, au_state);
-        if (errcode)
+        if (errcode) {
+            status = "KAU_S4U2SELF";
             goto cleanup;
+        }
     }
 
     reply.client = enc_tkt_reply.client;
--
2.13.2

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Add missing KDC "status" on some fail conditions

Greg Hudson
On 07/11/2017 01:48 PM, Samuel Cabrero wrote:
> The process_tgs_req function requires the status string to be set
> for all fail conditions before jump to "cleanup" label, otherwise
> it will cause an assertion failure.

In all of these cases, the status string is supposed to be set by the
subsidiary function, and as far as I know always is.  Did you run into a
case where the assertion failure triggered?
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Add missing KDC "status" on some fail conditions

Samuel Cabrero
Hi Greg,

yes, I triggered the assertion by testing samba 4.7rc1 built to use MIT
instead bundled Heimdal.

Once a Windows 7 machine is joined to the domain, every time it powers
up sends a TGS-REQ which triggers it. This is the backtrace:

#0  __GI_raise (sig=sig@entry=6) at
../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f16a602d0c6 in __GI_abort () at abort.c:78
#2  0x00007f16a602471a in __assert_fail_base (fmt=0x7f16a6163e80
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
    assertion=assertion@entry=0x55a129c524f6 "status != NULL",
file=file@entry=0x55a129c523b8 "do_tgs_req.c",
    line=line@entry=826, function=function@entry=0x55a129c525d0
<__PRETTY_FUNCTION__.13702> "process_tgs_req")
    at assert.c:92
#3  0x00007f16a6024792 in __GI___assert_fail
(assertion=assertion@entry=0x55a129c524f6 "status != NULL",
    file=file@entry=0x55a129c523b8 "do_tgs_req.c", line=line@entry=826,
    function=function@entry=0x55a129c525d0 <__PRETTY_FUNCTION__.13702>
"process_tgs_req") at assert.c:101
#4  0x000055a129c41212 in process_tgs_req
(handle=handle@entry=0x55a129e592c0 <shandle>,
    pkt=pkt@entry=0x55a12b325b38, from=from@entry=0x55a12be8a728,
response=response@entry=0x7ffe74820f18)
    at do_tgs_req.c:826
#5  0x000055a129c3eb5e in dispatch (cb=0x55a129e592c0 <shandle>,
local_saddr=<optimized out>,
    from=from@entry=0x55a12be8a728, pkt=pkt@entry=0x55a12b325b38,
is_tcp=is_tcp@entry=1,
    vctx=vctx@entry=0x55a12b0307e0, respond=0x55a129c4fd60
<process_tcp_response>, arg=0x55a12b325ab0)
    at dispatch.c:180
#6  0x000055a129c50019 in process_tcp_connection_read
(ctx=0x55a12b0307e0, ev=0x55a12bc59cd0) at net-server.c:1409
#7  0x00007f16a639fcb8 in verto_fire () from /usr/lib64/libverto.so.1
#8  0x00007f168e472736 in ev_invoke_pending (loop=0x7f168e67ca40
<default_loop_struct>) at ev.c:3288
#9  0x00007f168e475d48 in ev_run (loop=0x7f168e67ca40
<default_loop_struct>, flags=0) at ev.c:3688
#10 0x000055a129c3d98f in main (argc=2, argv=0x7ffe74821288) at
main.c:1065

And the involved data structures:

(gdb) p *request->server
$10 = {magic = 0, realm = {magic = 0, length = 7, data = 0x55a12afa88f0
"SUSE.AD"}, data = 0x55a12afa8770,
  length = 1, type = 10}
(gdb) p *header_ticket->enc_part2->client
$11 = {magic = 0, realm = {magic = 0, length = 7, data = 0x55a12b3ab880
"SUSE.AD"}, data = 0x55a12be0ab40,
  length = 1, type = 0}
(gdb) p *header_ticket->enc_part2->client->data
$12 = {magic = 0, length = 5, data = 0x55a12afaa970 "WIN7$U"}
(gdb) p *request->server->data
$13 = {magic = 0, length = 13, data = 0x55a12afa8a90 "win7$@SUSE.ADU"}
(gdb) printf "%x", request->kdc_options
40830000
(gdb) print *request
$14 = {magic = 0, msg_type = 12, padata = 0x55a12afadc00, kdc_options =
1082327040, client = 0x0,
  server = 0x55a12b7eb890, from = 0, till = 1499852047, rtime = 0,
nonce = 1809206369, nktypes = 5,
  ktype = 0x55a12afa8cd0, addresses = 0x0, authorization_data = {magic
= 0, enctype = 0, kvno = 0, ciphertext = {
      magic = 0, length = 0, data = 0x0}}, unenc_authdata = 0x0,
second_ticket = 0x55a12afa8b50}

I have traced it down and the function kdc_process_s4u2proxy_req
returns KRB5KDC_ERR_SERVER_NOMATCH without setting the status.

After your comment, this second version version of the patch is more
convenient as it respect the status if already set.

Also, just for reference, I have attached network captures with and
without patch.

Samuel Cabrero / SUSE Labs Samba Team
GPG: D7D6 E259 F91C F0B3 2E61 1239 3655 6EC9 7051 0856
[hidden email]
[hidden email]

On mar, jul 11, 2017 at 9:20 , Greg Hudson <[hidden email]> wrote:
> On 07/11/2017 01:48 PM, Samuel Cabrero wrote:
>>  The process_tgs_req function requires the status string to be set
>>  for all fail conditions before jump to "cleanup" label, otherwise
>>  it will cause an assertion failure.
>
> In all of these cases, the status string is supposed to be set by the
> subsidiary function, and as far as I know always is.  Did you run
> into a
> case where the assertion failure triggered?

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

0001-Add-missing-KDC-status-on-some-fail-conditions.patch (4K) Download Attachment