Fixed memory deallocation/cleanup on error returns

For the credentials structures, established the convention that any of
the Kerberos routines that mutate the credentials structures shall
free substructure before replacing it.

git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@3320 dc483132-0cff-0310-8789-dd5450dbe970

diff --git a/src/lib/krb5/krb/copy_tick.c b/src/lib/krb5/krb/copy_tick.c
index 8871d1b088025ac5b50e4d02c20337499762e952..d10fa3bb29d9fe02fff0515fa4bc2e3292df9a8f 100644 (file)
--- a/src/lib/krb5/krb/copy_tick.c
+++ b/src/lib/krb5/krb/copy_tick.c
@@ -67,7 +67,7 @@ krb5_enc_tkt_part **partto;
            krb5_free_principal(tempto->client);
            krb5_free_keyblock(tempto->session);
            krb5_xfree(tempto);
-           return retval;
+           return ENOMEM;
        }
        memcpy((char *)tempto->transited.tr_contents.data,
               (char *)partfrom->transited.tr_contents.data,
@@ -111,8 +111,10 @@ krb5_ticket **pto;
        return ENOMEM;
     *tempto = *from;
     retval = krb5_copy_principal(from->server, &tempto->server);
-    if (retval)
+    if (retval) {
+       krb5_xfree(tempto);
        return retval;
+    }
     retval = krb5_copy_data(&from->enc_part.ciphertext, &scratch);
     if (retval) {
        krb5_free_principal(tempto->server);

diff --git a/src/lib/krb5/krb/gc_2tgt.c b/src/lib/krb5/krb/gc_2tgt.c
index 2aca318bcf6c6db262d5394dbfab53ce42c11a11..80b5bc9d85aea5f75be119835ee7b5e699bf7c10 100644 (file)
--- a/src/lib/krb5/krb/gc_2tgt.c
+++ b/src/lib/krb5/krb/gc_2tgt.c
@@ -119,72 +119,73 @@ register krb5_creds * cred;
     if (retval)
        return retval;
 
-#undef cleanup
-#define cleanup() {\
-       memset((char *)dec_rep->enc_part2->session->contents, 0,\
-             dec_rep->enc_part2->session->length);\
-                 krb5_free_kdc_rep(dec_rep); }
-
     if (dec_rep->msg_type != KRB5_TGS_REP) {
-       cleanup();
-       return KRB5KRB_AP_ERR_MSG_TYPE;
+       retval = KRB5KRB_AP_ERR_MSG_TYPE;
+       goto errout;
     }
     
     /* now it's decrypted and ready for prime time */
 
     if (!krb5_principal_compare(dec_rep->client, tgt->client)) {
-       cleanup();
-       return KRB5_KDCREP_MODIFIED;
+       retval = KRB5_KDCREP_MODIFIED;
+       goto errout;
     }
     /* put pieces into cred-> */
-    if (retval = krb5_copy_keyblock_contents(dec_rep->enc_part2->session,
-                                            &cred->keyblock)) {
-       cleanup();
-       return retval;
+    if (cred->keyblock.contents) {
+       memset(&cred->keyblock.contents, 0, cred->keyblock.length);
+       krb5_xfree(cred->keyblock.contents);
     }
-    memset((char *)dec_rep->enc_part2->session->contents, 0,
-         dec_rep->enc_part2->session->length);
-
-#undef cleanup
-#define cleanup() {\
-       memset((char *)cred->keyblock.contents, 0, cred->keyblock.length);\
-                 krb5_free_kdc_rep(dec_rep); }
+    if (retval = krb5_copy_keyblock_contents(dec_rep->enc_part2->session,
+                                            &cred->keyblock))
+       goto errout;
 
     /* Should verify that the ticket is what we asked for. */
     cred->times = dec_rep->enc_part2->times;
     cred->ticket_flags = dec_rep->enc_part2->flags;
     cred->is_skey = TRUE;
-    if (dec_rep->enc_part2->caddrs) {
-       if (retval = krb5_copy_addresses(dec_rep->enc_part2->caddrs,
-                                        &cred->addresses)) {
-           cleanup();
-           return retval;
-       }
-    } else {
+    if (cred->addresses)
+       krb5_free_addresses(cred->addresses);
+    if (dec_rep->enc_part2->caddrs)
+       retval = krb5_copy_addresses(dec_rep->enc_part2->caddrs,
+                                    &cred->addresses);
+    else
        /* no addresses in the list means we got what we had */
-       if (retval = krb5_copy_addresses(tgt->addresses,
-                                        &cred->addresses)) {
-           cleanup();
-           return retval;
-       }
-    }
+       retval = krb5_copy_addresses(tgt->addresses, &cred->addresses);
+    if (retval)
+           goto errout;
+    
+    if (cred->server)
+       krb5_free_principal(cred->server);
     if (retval = krb5_copy_principal(dec_rep->enc_part2->server,
-                                    &cred->server)) {
-       cleanup();
-       return retval;
-    }
+                                    &cred->server))
+       goto errout;
 
-    if (retval = encode_krb5_ticket(dec_rep->ticket, &scratch)) {
-       cleanup();
-       krb5_free_addresses(cred->addresses);
-       return retval;
-    }
+    if (retval = encode_krb5_ticket(dec_rep->ticket, &scratch))
+       goto errout;
 
     cred->ticket = *scratch;
     krb5_xfree(scratch);
 
+errout:
+    if (retval) {
+       if (cred->keyblock.contents) {
+           memset((char *)cred->keyblock.contents, 0, cred->keyblock.length);
+           krb5_xfree(cred->keyblock.contents);
+           cred->keyblock.contents = 0;
+       }
+       if (cred->addresses) {
+           krb5_free_addresses(cred->addresses);
+           cred->addresses = 0;
+       }
+       if (cred->server) {
+           krb5_free_principal(cred->server);
+           cred->server = 0;
+       }
+    }
+    memset((char *)dec_rep->enc_part2->session->contents, 0,
+          dec_rep->enc_part2->session->length);
     krb5_free_kdc_rep(dec_rep);
-    return 0;
+    return retval;
 }
 
 /*

diff --git a/src/lib/krb5/krb/gc_frm_kdc.c b/src/lib/krb5/krb/gc_frm_kdc.c
index 0110328c3d7960b198c778969a80ceaff958d7db..c825efa2fa43d6d69f5198a6f21f27dc58c561cd 100644 (file)
--- a/src/lib/krb5/krb/gc_frm_kdc.c
+++ b/src/lib/krb5/krb/gc_frm_kdc.c
@@ -73,7 +73,7 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 {
     krb5_creds tgt, tgtq;
     krb5_creds **ret_tgts = 0;
-    krb5_principal *tgs_list, *next_server;
+    krb5_principal *tgs_list = 0, *next_server;
     krb5_principal final_server;
     krb5_error_code retval;
     int nservers;
@@ -82,6 +82,9 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 
     /* in case we never get a TGT, zero the return */
     *tgts = 0;
+    
+    memset((char *)&tgtq, 0, sizeof(tgtq));
+    memset((char *)&tgt, 0, sizeof(tgt));
 
     /*
      * we know that the desired credentials aren't in the cache yet.
@@ -99,20 +102,14 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
      *  realm's KDC; so we use KRB5_TC_MATCH_SRV_NAMEONLY below)
      */
 
-    /*
-     * we're sharing some substructure here, which is dangerous.
-     * Be sure that if you muck with things here that tgtq.* doesn't share
-     * any substructure before you deallocate/clean up/whatever.
-     */
-    memset((char *)&tgtq, 0, sizeof(tgtq));
-    tgtq.client = cred->client;
-
+    if (retval = krb5_copy_principal(cred->client, &tgtq.client))
+       goto errout;
     if (retval = krb5_tgtname(krb5_princ_realm(cred->server),
                              krb5_princ_realm(cred->client), &final_server))
-       return retval;
-    tgtq.server = final_server;
+       goto errout;
+    if (retval = krb5_copy_principal(final_server, &tgtq.server))
+       goto errout;
 
-    /* try to fetch it directly */
     retval = krb5_cc_retrieve_cred (ccache,
                                    KRB5_TC_MATCH_SRV_NAMEONLY,
                                    &tgtq,
@@ -120,16 +117,16 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 
     if (retval != 0) {
        if (retval != KRB5_CC_NOTFOUND)
-           goto out;
+           goto errout;
        /* don't have the right TGT in the cred cache.  Time to iterate
           across realms to get the right TGT. */
 
        /* get a list of realms to consult */
-       retval = krb5_walk_realm_tree(krb5_princ_realm(cred->client),
-                                     krb5_princ_realm(cred->server),
-                                     &tgs_list, KRB5_REALM_BRANCH_CHAR);
-       if (retval)
-           goto out;
+       if (retval = krb5_walk_realm_tree(krb5_princ_realm(cred->client),
+                                         krb5_princ_realm(cred->server),
+                                         &tgs_list, KRB5_REALM_BRANCH_CHAR))
+           goto errout;
+
        /* walk the list BACKWARDS until we find a cached
           TGT, then move forward obtaining TGTs until we get the last
           TGT needed */
@@ -140,16 +137,16 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
 
        /* next_server now points to the last TGT */
        for (; next_server >= tgs_list; next_server--) {
-           tgtq.server = *next_server;
+           krb5_free_principal(tgtq.server);
+           if (retval = krb5_copy_principal(*next_server, &tgt.server))
+               goto errout;
            retval = krb5_cc_retrieve_cred (ccache,
                                            KRB5_TC_MATCH_SRV_NAMEONLY,
                                            &tgtq,
                                            &tgt);
            if (retval) {
-               if (retval != KRB5_CC_NOTFOUND) {
-                   krb5_free_realm_tree(tgs_list);
-                   goto out;
-               }
+               if (retval != KRB5_CC_NOTFOUND)
+                   goto errout;
                continue;
            }
            next_server++;
@@ -158,38 +155,38 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
        if (next_server < tgs_list) {
            /* didn't find any */
            retval = KRB5_NO_TKT_IN_RLM;
-           krb5_free_realm_tree(tgs_list);
-           goto out;
+           goto errout;
        }
        /* allocate storage for TGT pointers. */
        ret_tgts = (krb5_creds **)calloc(nservers+1, sizeof(krb5_creds));
        if (!ret_tgts) {
            retval = ENOMEM;
-           krb5_free_realm_tree(tgs_list);
-           goto out;
+           goto errout;
        }
        *tgts = ret_tgts;
        for (nservers = 0; *next_server; next_server++, nservers++) {
-
            krb5_data *tmpdata;
 
            if (!valid_keytype(tgt.keyblock.keytype)) {
                retval = KRB5_PROG_KEYTYPE_NOSUPP;
-               krb5_free_realm_tree(tgs_list);
-               goto out;
+               goto errout;
            }
            /* now get the TGTs */
+           krb5_free_cred_contents(&tgtq);
            memset((char *)&tgtq, 0, sizeof(tgtq));
            tgtq.times = tgt.times;
-           tgtq.client = tgt.client;
+           if (retval = krb5_copy_principal(tgt.client, &tgtq.client))
+               goto errout;
 
            /* ask each realm for a tgt to the end */
-           if (retval = krb5_copy_data(krb5_princ_realm(*next_server), &tmpdata)) {
-               krb5_free_realm_tree(tgs_list);
-               goto out;
+           if (retval = krb5_copy_data(krb5_princ_realm(*next_server),
+                                       &tmpdata)) {
+               goto errout;
            }
+           free(krb5_princ_realm(final_server)->data);
            krb5_princ_set_realm(final_server, tmpdata);
-           tgtq.server = final_server;
+           if (retval = krb5_copy_principal(final_server, &tgtq.server))
+               goto errout;
 
            tgtq.is_skey = FALSE;
            tgtq.ticket_flags = tgt.ticket_flags;
@@ -199,10 +196,9 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
                                               flags2options(tgtq.ticket_flags),
                                               etype,
                                               krb5_kdc_req_sumtype,
-                                              &tgtq)) {
-               krb5_free_realm_tree(tgs_list);
-               goto out;
-           }
+                                              &tgtq))
+               goto errout;
+
            /* make sure the returned ticket is somewhere in the remaining
               list, but we can tolerate different expected issuing realms */
            while (*++next_server &&
@@ -210,28 +206,21 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
                                           &(tgtq.server[1])));
            if (!next_server) {
                /* what we got back wasn't in the list! */
-               krb5_free_realm_tree(tgs_list);
                retval = KRB5_KDCREP_MODIFIED;
-               goto out;
+               goto errout;
            }
                                                           
            /* save tgt in return array */
-           if (retval = krb5_copy_creds(&tgtq, &ret_tgts[nservers])) {
-               krb5_free_realm_tree(tgs_list);
-               goto out;
-           }
+           if (retval = krb5_copy_creds(&tgtq, &ret_tgts[nservers]))
+               goto errout;
            tgt = *ret_tgts[nservers];
            returning_tgt = 1;          /* don't free it below... */
-           tgtq.client = 0;
-           tgtq.server = 0;
-           krb5_free_cred_contents(&tgtq);
        }
-       krb5_free_realm_tree(tgs_list);
     }
     /* got/finally have tgt! */
     if (!valid_keytype(tgt.keyblock.keytype)) {
        retval = KRB5_PROG_KEYTYPE_NOSUPP;
-       goto out;
+       goto errout;
     }
     etype = krb5_keytype_array[tgt.keyblock.keytype]->system->proto_enctype;
 
@@ -246,10 +235,13 @@ krb5_get_cred_from_kdc (ccache, cred, tgts)
                                       etype,
                                       krb5_kdc_req_sumtype,
                                       cred);
-    
+errout:
     if (!returning_tgt)
        krb5_free_cred_contents(&tgt);
-out:
-    krb5_free_principal(final_server);
+    if (final_server)
+           krb5_free_principal(final_server);
+    krb5_free_cred_contents(&tgtq);
+    if (tgs_list)
+       krb5_free_realm_tree(tgs_list);
     return retval;
 }

diff --git a/src/lib/krb5/krb/gc_via_tgt.c b/src/lib/krb5/krb/gc_via_tgt.c
index d6ef9119bfa176e4d07fec539d4f442894f8f5b9..23dc668f36ac8822ec6c5e814c5016aca0033b43 100644 (file)
--- a/src/lib/krb5/krb/gc_via_tgt.c
+++ b/src/lib/krb5/krb/gc_via_tgt.c
@@ -143,6 +143,10 @@ OLDDECLARG(krb5_creds *, cred)
        return KRB5_KDCREP_MODIFIED;
     }
     /* put pieces into cred-> */
+    if (cred->keyblock.contents) {
+       memset(&cred->keyblock.contents, 0, cred->keyblock.length);
+       krb5_xfree(cred->keyblock.contents);
+    }
     if (retval = krb5_copy_keyblock_contents(dec_rep->enc_part2->session,
                                             &cred->keyblock)) {
        cleanup();
@@ -188,6 +192,9 @@ OLDDECLARG(krb5_creds *, cred)
 
     cred->ticket_flags = dec_rep->enc_part2->flags;
     cred->is_skey = FALSE;
+    if (cred->addresses) {
+       krb5_free_addresses(cred->addresses);
+    }
     if (dec_rep->enc_part2->caddrs) {
        if (retval = krb5_copy_addresses(dec_rep->enc_part2->caddrs,
                                         &cred->addresses)) {
@@ -205,7 +212,8 @@ OLDDECLARG(krb5_creds *, cred)
     /*
      * Free cred->server before overwriting it.
      */
-    krb5_free_principal(cred->server);
+    if (cred->server)
+       krb5_free_principal(cred->server);
     if (retval = krb5_copy_principal(dec_rep->enc_part2->server,
                                     &cred->server)) {
        cleanup();

diff --git a/src/lib/krb5/krb/get_in_tkt.c b/src/lib/krb5/krb/get_in_tkt.c
index 320a3b2080742fb2f983b695edebec73c6cef003..28ff4739e2754f162b6b9b827d88cec3a9a9328f 100644 (file)
--- a/src/lib/krb5/krb/get_in_tkt.c
+++ b/src/lib/krb5/krb/get_in_tkt.c
@@ -320,6 +320,7 @@ OLDDECLARG(krb5_kdc_rep **, ret_as_reply)
     if (retval = krb5_copy_addresses(as_reply->enc_part2->caddrs,
                                     &creds->addresses)) {
        cleanup_key();
+       krb5_free_kdc_rep(as_reply);
        return retval;
     }
     creds->second_ticket.length = 0;
@@ -330,6 +331,7 @@ OLDDECLARG(krb5_kdc_rep **, ret_as_reply)
        krb5_free_kdc_rep(as_reply);
        krb5_free_addresses(creds->addresses);
        cleanup_key();
+       krb5_free_kdc_rep(as_reply);
        return retval;
     }  
     creds->ticket = *packet;
@@ -342,6 +344,7 @@ OLDDECLARG(krb5_kdc_rep **, ret_as_reply)
        krb5_xfree(creds->ticket.data);
        krb5_free_addresses(creds->addresses);
        cleanup_key();
+       krb5_free_kdc_rep(as_reply);
        return retval;
     }
     if (ret_as_reply)

diff --git a/src/lib/krb5/krb/mk_req.c b/src/lib/krb5/krb/mk_req.c
index d9afd1cbfe8aa38d6ef5a9996ea0db766a150b53..252ec976d1f8fc7e63df119ef9dc14207dd4ce5c 100644 (file)
--- a/src/lib/krb5/krb/mk_req.c
+++ b/src/lib/krb5/krb/mk_req.c
@@ -69,9 +69,10 @@ krb5_data *outbuf;
     /* obtain ticket & session key */
 
     memset((char *)&creds, 0, sizeof(creds));
-    creds.server = (krb5_principal) server;
+    if (retval = krb5_copy_principal(server, &creds.server))
+       goto errout;
     if (retval = krb5_cc_get_principal(ccache, &creds.client))
-       return(retval);
+       goto errout;
     /* creds.times.endtime = 0; -- memset 0 takes care of this
                                   zero means "as long as possible" */
     /* creds.keyblock.keytype = 0; -- as well as this.
@@ -81,7 +82,7 @@ krb5_data *outbuf;
     if (retval = krb5_get_credentials(krb5_kdc_default_options,
                                      ccache,
                                      &creds))
-       return(retval);
+       goto errout;
 
     retval = krb5_mk_req_extended(ap_req_options,
                                  checksum,
@@ -92,9 +93,8 @@ krb5_data *outbuf;
                                  &creds,
                                  0,    /* We don't need the authenticator */
                                  outbuf);
-    /* creds.server and the rest of the creds.* fields are filled in
-       by the ccache fetch or the kdc fetch, so we should allow it to be
-       freed */
+
+errout:
     krb5_free_cred_contents(&creds);
     return retval;
 }

diff --git a/src/lib/krb5/krb/rd_safe.c b/src/lib/krb5/krb/rd_safe.c
index 8e6cd992db2fd4f1a5780d41a5d3a71e188e8f46..b884e1660e44483e35babfd7de08de0090383066 100644 (file)
--- a/src/lib/krb5/krb/rd_safe.c
+++ b/src/lib/krb5/krb/rd_safe.c
@@ -78,11 +78,15 @@ krb5_data *outbuf;
 
 #define cleanup() krb5_free_safe(message)
 
-    if (!valid_cksumtype(message->checksum->checksum_type))
+    if (!valid_cksumtype(message->checksum->checksum_type)) {
+       cleanup();
        return KRB5_PROG_SUMTYPE_NOSUPP;
+    }
     if (!is_coll_proof_cksum(message->checksum->checksum_type) ||
-       !is_keyed_cksum(message->checksum->checksum_type))
+       !is_keyed_cksum(message->checksum->checksum_type)) {
+       cleanup();
        return KRB5KRB_AP_ERR_INAPP_CKSUM;
+    }
 
     if (!(safe_flags & KRB5_SAFE_NOTIME)) {
        krb5_donot_replay replay;