Patch 5/9: pkinit_load_fs_cert_and_key() must not blindly overwrite pointer

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

Patch 5/9: pkinit_load_fs_cert_and_key() must not blindly overwrite pointer

Alexandr Nedvedicky
Hello,

I'm upgrading kerberos bundled with Solaris to krb5-1.16. Solaris currently
ships krb5-1.15.1. I've noticed there are some memory leaks, while running test
suite, which comes with krb-1.16 (e.g. running 'make check').  I don't think
those memory leaks are critical, though as kerberos newbie I can't be sure, so
I think I'm better to share my findings. All memory leaks were found using
'libumem', which can be found on Solaris (or its OSS sibbling illumos).
All patches are against krb5-1.16 release.

I'm sure the patch fixes the leak, however the change might be rather
fixing symptoms than a real problem. The story of this leak
is bit convoluted. The memory culprit has stack as follows:

f9c4f408 libc_hwcap1.so.1`__lwp_sigqueue+0x15(1, 6, f9c4f428, f6af9c7e)
f9c4f428 libc_hwcap1.so.1`raise+0x22(6, 0, f9c4f478, f6acb71e)
f9c4f478 libc_hwcap1.so.1`abort+0xe6(65737341, 6f697472, 6166206e, 64656c69, 6469203a, 7972635f)
f9c4f688 0xf6acc669(f5f805d0, f5f819c0, 1205, f5fb198c, 868bd30, 8692c08)
f9c4f6d8 pkinit.so`pkinit_load_fs_cert_and_key+0x188(8670dc8, 8677d08, 8692c88, 8692c08, 0, 8677d08)
f9c4f718 pkinit.so`pkinit_get_certs_fs+0xc1(8670dc8, 869bf88, 0, 87cff88, 8677d08, 0)
f9c4f768 pkinit.so`crypto_load_certs+0x59(8670dc8, 869bf88, 0, 87cff88, 8677d08, 0)
f9c4f7b8 pkinit.so`pkinit_identity_prompt+0xb7(8670dc8, 869bf88, 0, 87cff88, 8677d08, 0)
f9c4f808 pkinit.so`pkinit_server_plugin_init_realm+0x27a(8670dc8, 86698d0, f9c4f828, f5f8c3cb)
f9c4f858 pkinit.so`pkinit_server_plugin_init+0x169(8670dc8, f9c4f888, 8658868, f9c4f884)
f9c4f8c8 load_preauth_plugins+0x146(80c4a70)
f9c4f968 main+0x2e9(2, f9c4f998, f9c4f9a4)
f9c4f98c _start+0x72(2, f9c4fb2d, f9c4fb35, 0, f9c4fb38, f9c4fb3f)

As you can see the call stack comes from process, which tripped
my interim diagnostic assert here:

    4583 static krb5_error_code
    4584 pkinit_load_fs_cert_and_key(krb5_context context,
    4585                             pkinit_identity_crypto_context id_cryptoctx,
    4586                             char *certname,
    4587                             char *keyname,
    4588                             int cindex)
    4589 {
    4590     krb5_error_code retval;
    4591     X509 *x = NULL;
    4592     EVP_PKEY *y = NULL;
    4593     char *fsname = NULL;
    ....
    4608     /* Load the key. */
    4609     retval = get_key(context, id_cryptoctx, keyname, fsname, &y, password);
    4610     if (retval != 0 || y == NULL) {
    4611         retval = oerr(context, 0, _("Cannot read key file '%s'"), fsname);
    4612         goto cleanup;
    4613     }
    4614
    4615     assert(id_cryptoctx->creds[cindex] == NULL);
    4616    
    4617     id_cryptoctx->creds[cindex] = malloc(sizeof(struct _pkinit_cred_info));
    4618     if (id_cryptoctx->creds[cindex] == NULL) {
    4619         retval = ENOMEM;
    4620         goto cleanup;
    4621     }


The resulting core file gave me a chance to find out, where buffer, which gets
overwritten got allocated. The story starts at pkinit_server_plugin_init_realm()
    1386 static int
    1387 pkinit_server_plugin_init_realm(krb5_context context, const char *realmname,
    1388                                 pkinit_kdc_context *pplgctx)
    1389 {  
    ....
    1421     retval = pkinit_init_identity_opts(&plgctx->idopts);
    1422     if (retval)
    1423         goto errout;
    1424
    1425     retval = pkinit_init_kdc_profile(context, plgctx);
    1426     if (retval)
    1427         goto errout;
    1428
        /*
         * this is where we are going to call crypto_load_certs()
         * for the first time. The function will load certificates to
         * id_cryptoctx->creds[0]. See call to pkinit_load_fs_cert_and_key() in
         * pkinit_get_certs_fs(), which specifically asks to use slot 0.
         */
    1429     retval = pkinit_identity_initialize(context, plgctx->cryptoctx, NULL,
    1430                                         plgctx->idopts, plgctx->idctx,
    1431                                         NULL, NULL, NULL);
    1432     if (retval)
    1433         goto errout;
        /*
         * this is where we are going to call crypto_load_certs() for the
         * second time. The call essentially takes same path as earlier call to
         * pkinit_identity_initialize(). It will also load certificate to slot
         * 0.
         */
    1434     retval = pkinit_identity_prompt(context, plgctx->cryptoctx, NULL,
    1435                                     plgctx->idopts, plgctx->idctx,
    1436                                     NULL, NULL, 0, NULL);
    1437     if (retval)
    1438         goto errout;
    1439
    1440     pkiDebug("%s: returning context at %p for realm '%s'\n",
    1441              __FUNCTION__, plgctx, realmname);
    1442     *pplgctx = plgctx;
    1443     retval = 0;

I've tried to fix this bug by using slot 0 for certificate loaded
via pkinit_identity_initialize() and slot 1 for certificate,
loaded via pkinit_identity_prompt(). However such fix breaks
t_pkinit.py test:
krb5-1.16/src/tests/t_pkinit.py
*** Failure: /export/home/sashan/kerberos/components/krb5/build/i86/clients/kinit/kinit failed with code 1.
*** Last command (#2): /export/home/sashan/kerberos/components/krb5/build/i86/clients/kinit/kinit -X X509_user_identity=PKCS12:/export/home/sashan/kerberos/components/krb5/krb5-1.16/src/tests/dejagnu/pkinit-certs/user-upn2.p12 [hidden email]
*** Output of last command:
Password for [hidden email]:
kinit: Pre-authentication failed: Cannot read password while getting initial credentials
For details, see: /export/home/sashan/kerberos/components/krb5/build/i86/tests/testlog
Or re-run this test script with the -v flag
    cd /export/home/sashan/kerberos/components/krb5/build/i86/tests
    PYTHONPATH=/export/home/sashan/kerberos/components/krb5/krb5-1.16/src/util /usr/bin/python /export/home/sashan/kerberos/components/krb5/krb5-1.16/src/tests/t_pkinit.py -v
Use --debug=NUM to run a command under a debugger.  Use
--stop-after=NUM to stop after a daemon is started in order to
attach to it with a debugger.  Use --help to see other options.
make[2]: *** [Makefile:761: check-pytests] Error 1
make[2]: Leaving directory '/export/home/sashan/kerberos/components/krb5/build/i86/tests'
make[1]: *** [Makefile:1525: check-recurse] Error 1
make[1]: Leaving directory '/export/home/sashan/kerberos/components/krb5/build/i86'
gmake: *** [/export/home/sashan/kerberos/make-rules/configure.mk:220: /export/home/sashan/kerberos/components/krb5/build/i86/.tested] Error 2

I'm sure this patch needs an kerberos export eyes.

thanks and
regards
sasha

--------8<---------------8<---------------8<------------------8<--------
diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
index d3b4ad5d8..19bf1c359 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
+++ b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
@@ -4609,6 +4609,11 @@ pkinit_load_fs_cert_and_key(krb5_context context,
         goto cleanup;
     }
 
+    if (id_cryptoctx->creds[cindex] != NULL) {
+ pkinit_free_cred(id_cryptoctx->creds[cindex]);
+ id_cryptoctx->creds[cindex] = NULL;
+    }
+
     id_cryptoctx->creds[cindex] = malloc(sizeof(struct _pkinit_cred_info));
     if (id_cryptoctx->creds[cindex] == NULL) {
         retval = ENOMEM;
@@ -4658,8 +4663,9 @@ pkinit_get_certs_fs(krb5_context context,
     }
 
     retval = pkinit_load_fs_cert_and_key(context, id_cryptoctx,
-                                         idopts->cert_filename,
-                                         idopts->key_filename, 0);
+ idopts->cert_filename,
+ idopts->key_filename, 0);
+
 cleanup:
     return retval;
 }
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev