Ticket History #8415: Uninitialized read in krb5_sname_match

Hi, I've encountered an issue in libkrb5 while running MongoDB with Valgrind, and authenticating to it with GSSAPI. Here is a keytab file that I've made for reproducing an issue that was reported to me:

sajack@spencerjacksonDesktop /home/sajack/mongo  % klist -kt archTest.keytab    
Keytab name: FILE:archTest.keytab
KVNO Timestamp           Principal
---- ------------------- ------------------------------------------------------
   2 05/09/2016 16:14:02 mongodb@MONGOTEST.COM
   2 05/09/2016 16:14:02 mongodb/database.mongotest.com@MONGOTEST.COM
   2 05/09/2016 16:14:02 mongodb/database.mongotest.com@MONGOTEST.COM
   2 05/09/2016 16:14:02 mongodb/database.mongotest.com@MONGOTEST.COM
   2 05/09/2016 16:14:02 mongodb/database.mongotest.com@MONGOTEST.COM

While using this keytab, Valgrind reports the following warnings:
==31907== Thread 18:
==31907== Invalid read of size 4
==31907==    at 0x7EE68E0: data_eq (k5-int.h:2233)
==31907==    by 0x7EE68E0: krb5_sname_match (sname_match.c:49)
==31907==    by 0x55D07A0: check_keytab (acquire_cred.c:156)
==31907==    by 0x55D07A0: acquire_accept_cred (acquire_cred.c:225)
==31907==    by 0x55D07A0: acquire_cred_context.isra.10 (acquire_cred.c:797)
==31907==    by 0x55D0F99: krb5_gss_acquire_cred_from (acquire_cred.c:1224)
==31907==    by 0x55BEF4F: gss_add_cred_from (g_acquire_cred.c:455)
==31907==    by 0x55BF55A: gss_acquire_cred_from (g_acquire_cred.c:190)
==31907==    by 0x55BF715: gss_acquire_cred (g_acquire_cred.c:107)
==31907==    by 0xAA30120: gssapi_server_mech_authneg (gssapi.c:721)
==31907==    by 0xAA31803: gssapi_server_mech_step (gssapi.c:1267)
==31907==    by 0x53A5E72: sasl_server_step (server.c:1618)
==31907==    by 0x53A5BF2: sasl_server_start (server.c:1533)
==31907==    by 0x1960C5E: mongo::CyrusSaslAuthenticationSession::step(mongo::StringData, std::__1::basic_string<char, std::__1::char_trait$
<char>, std::__1::allocator<char> >*) (cyrus_sasl_authentication_session.cpp:353)
==31907==    by 0x13BCC1B: mongo::(anonymous namespace)::doSaslStep(mongo::ClientBasic const*, mongo::SaslAuthenticationSession*, mongo::BS$
NObj const&, mongo::BSONObjBuilder*) (sasl_commands.cpp:187)
==31907==  Address 0x117ea934 is 4 bytes after a block of size 16 alloc'd
==31907==    at 0x5184947: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31907==    by 0x7EBD4B2: krb5_ktfileint_internal_read_entry.isra.5 (kt_file.c:1243)
==31907==    by 0x7EBECEB: krb5_ktfileint_read_entry (kt_file.c:1407)
==31907==    by 0x7EBECEB: krb5_ktfile_get_next (kt_file.c:508)
==31907==    by 0x55D07C8: check_keytab (acquire_cred.c:155)
==31907==    by 0x55D07C8: acquire_accept_cred (acquire_cred.c:225)
==31907==    by 0x55D07C8: acquire_cred_context.isra.10 (acquire_cred.c:797)
==31907==    by 0x55D0F99: krb5_gss_acquire_cred_from (acquire_cred.c:1224)
==31907==    by 0x55BEF4F: gss_add_cred_from (g_acquire_cred.c:455)
==31907==    by 0x55BF55A: gss_acquire_cred_from (g_acquire_cred.c:190)                                                            [16/1973]
==31907==    by 0x55BF715: gss_acquire_cred (g_acquire_cred.c:107)
==31907==    by 0xAA30120: gssapi_server_mech_authneg (gssapi.c:721)
==31907==    by 0xAA31803: gssapi_server_mech_step (gssapi.c:1267)
==31907==    by 0x53A5E72: sasl_server_step (server.c:1618)
==31907==    by 0x53A5BF2: sasl_server_start (server.c:1533)
==31907==
==31907== Invalid read of size 4
==31907==    at 0x7EE68E0: data_eq (k5-int.h:2233)
==31907==    by 0x7EE68E0: krb5_sname_match (sname_match.c:49)
==31907==    by 0x7EDEF6E: decrypt_ticket (rd_req_dec.c:396)
==31907==    by 0x7EDEF6E: rd_req_decoded_opt (rd_req_dec.c:504)
==31907==    by 0x7EDFBAE: krb5_rd_req_decoded (rd_req_dec.c:808)
==31907==    by 0x55CDEF1: kg_accept_krb5 (accept_sec_context.c:642)
==31907==    by 0x55CF3B0: krb5_gss_accept_sec_context_ext (accept_sec_context.c:1303)
==31907==    by 0x55CF4E1: krb5_gss_accept_sec_context (accept_sec_context.c:1332)
==31907==    by 0x55BE681: gss_accept_sec_context (g_accept_sec_context.c:267)
==31907==    by 0xAA30243: gssapi_server_mech_authneg (gssapi.c:747)
==31907==    by 0xAA31803: gssapi_server_mech_step (gssapi.c:1267)
==31907==    by 0x53A5E72: sasl_server_step (server.c:1618)
==31907==    by 0x53A5BF2: sasl_server_start (server.c:1533)
==31907==    by 0x1960C5E: mongo::CyrusSaslAuthenticationSession::step(mongo::StringData, std::__1::basic_string<char, std::__1::char_traits
<char>, std::__1::allocator<char> >*) (cyrus_sasl_authentication_session.cpp:353)
==31907==  Address 0x11801304 is 4 bytes after a block of size 16 alloc'd
==31907==    at 0x5184947: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31907==    by 0x7EBD4B2: krb5_ktfileint_internal_read_entry.isra.5 (kt_file.c:1243)
==31907==    by 0x7EBECEB: krb5_ktfileint_read_entry (kt_file.c:1407)
==31907==    by 0x7EBECEB: krb5_ktfile_get_next (kt_file.c:508)
==31907==    by 0x7EDEF52: decrypt_ticket (rd_req_dec.c:394)
==31907==    by 0x7EDEF52: rd_req_decoded_opt (rd_req_dec.c:504)
==31907==    by 0x7EDFBAE: krb5_rd_req_decoded (rd_req_dec.c:808)
==31907==    by 0x55CDEF1: kg_accept_krb5 (accept_sec_context.c:642)
==31907==    by 0x55CF3B0: krb5_gss_accept_sec_context_ext (accept_sec_context.c:1303)
==31907==    by 0x55CF4E1: krb5_gss_accept_sec_context (accept_sec_context.c:1332)
==31907==    by 0x55BE681: gss_accept_sec_context (g_accept_sec_context.c:267)
==31907==    by 0xAA30243: gssapi_server_mech_authneg (gssapi.c:747)
==31907==    by 0xAA31803: gssapi_server_mech_step (gssapi.c:1267)
==31907==    by 0x53A5E72: sasl_server_step (server.c:1618)

My understanding of these warnings is that krb5_sname_match is called against all principals stored in a keytab to find which entry it should accept tickets with. However it does not check if the keytab's principal actually has a hostname, before it is dereferenced and compared to the acceptor hostname. This leads to uninitialized memory being read. In rare situations, if the uninitialized value read out of princ->data[1].length is equal to the length stored in matching->data[1].length, data_eq in k5-int.h could attempt to call memcmp with invalid pointers causing a segfault.

The below patch prevents Valgrind from emitting these warnings.




From a46ee6666b2ea7ba9c864a614a61adf7aee0ffd6 Mon Sep 17 00:00:00 2001
From: Spencer Jackson <spencer.jackson@mongodb.com>
Date: Mon, 9 May 2016 18:23:16 -0400
Subject: [PATCH] Prevent uninitialized read in krb5_sname_match

---
 src/lib/krb5/krb/sname_match.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/lib/krb5/krb/sname_match.c b/src/lib/krb5/krb/sname_match.c
index 0c7bd39..61f1de8 100644
--- a/src/lib/krb5/krb/sname_match.c
+++ b/src/lib/krb5/krb/sname_match.c
@@ -41,12 +41,13 @@ krb5_sname_match(krb5_context context, krb5_const_principal matching,
         return FALSE;
 
     /* Check the service name (must be present in matching). */
-    if (!data_eq(matching->data[0], princ->data[0]))
+    if (princ->length == 0 || !data_eq(matching->data[0], princ->data[0]))
         return FALSE;
 
-    /* Check the hostname if present in matching and not ignored. */
-    if (matching->data[1].length != 0 && !context->ignore_acceptor_hostname &&
-        !data_eq(matching->data[1], princ->data[1]))
+    /* Check the hostname if present in both matching and princ, and not ignored.
+     * matching->length == 2, so princ must have greater or equal length to be comparable. */
+    if (!context->ignore_acceptor_hostname && matching->data[1].length != 0 &&
+        (princ->length < 2 || !data_eq(matching->data[1], princ->data[1])))
         return FALSE;
 
     /* All elements match. */
--
2.8.2