From crawdad@gungnir.fnal.gov Fri Mar 16 10:53:37 2001 Received: from pacific-carrier-annex.mit.edu (PACIFIC-CARRIER-ANNEX.MIT.EDU [18.7.21.83]) by rt-11.mit.edu (8.9.3/8.9.3) with ESMTP id KAA01680 for ; Fri, 16 Mar 2001 10:53:37 -0500 (EST) Received: from gungnir.fnal.gov (gungnir.fnal.gov [131.225.80.1]) by pacific-carrier-annex.mit.edu (8.9.2/8.9.2) with ESMTP id KAA00826; Fri, 16 Mar 2001 10:35:32 -0500 (EST) Received: (from crawdad@localhost) by gungnir.fnal.gov (8.9.1/8.9.1) id JAA03408; Fri, 16 Mar 2001 09:35:31 -0600 (CST) Message-Id: <200103161535.JAA03408@gungnir.fnal.gov> Date: Fri, 16 Mar 2001 09:35:31 -0600 (CST) From: crawdad@fnal.gov Reply-To: crawdad@gungnir.fnal.gov To: krb5-bugs@mit.edu Cc: krbdev@mit.edu Subject: kadmind potential core dump, versions 1.0.x through 1.2.2 X-Send-Pr-Version: 3.99 >Number: 929 >Category: krb5-admin >Synopsis: Lowering pol->pw_history_num opens a potential core dump in kadmind >Confidential: no >Severity: serious >Priority: low >Responsible: mitchb >State: closed >Class: sw-bug >Submitter-Id: unknown >Arrival-Date: Fri Mar 16 10:54:00 EST 2001 >Last-Modified: Tue Oct 16 06:29:36 EDT 2001 >Originator: Matt Crawford >Organization: Fermilab >Release: krb5-1.2.2 (and earlier) >Environment: Observed on Solaris 2.7, should affect all platforms System: SunOS gungnir.fnal.gov 5.5.1 Generic_103640-24 sun4u sparc SUNW,Ultra-1 Architecture: sun4 >Description: If a policy is modified to have a lower pw_history_num, or a user principal has its policy changed to one with a lower history length, and if a user affected by that change has not "filled" the old history list but has at least as many old_keys as the changed or new policy would record, then kadmind will crash at that user's next password change. >How-To-Repeat: Create a new principal with policy with a pw_history_num of 8. Change that principal's password three times. Now alter the policy to have pw_history_num = 3 (or 2 or 4), or modprinc the principal to have a different policy with that pw_history_num. Now change the principal's password once more. Boom. >Fix: The problem is in lib/kadm5/srv/svr_principal.c add_to_history(). The code assumes that the counter adb->old_key_next will never exceed adb->old_key_len-1. Essentially, it was written with the assumption that the policy under which the history list was last updated is still in force. Under the How-To-Repeat conditions it will start freeing key data one element beyond the end of the adb->old_keys array. This crashes with a stack trace like: krb5_free_key_data_contents(4de58,0,7,6,4,c2) + 30 add_to_history(4de58,ffbef2c0,ffbef2dc,ffbef21c,5,4ff78) + 20c kadm5_chpass_principal(5d6c8,60ae0,603f8,602f0,4fa90,ffffffff) + 70c chpass_principal_wrapper(5d6c8,60ae0,603f8,ffbef480,0,ffbef455) + 68 chpass_principal_1(ffbef480,ffbef9f4,ffbef480,fffffff8,0,ffbef4d9) + 1f0 kadm_1(ffbef9f4,5b0d0,ffbef9f0,0,0,493e1) + 49c sunrpc_seterr_reply(ffbefac8,ff03517c,ff03517c,500b8,d,5b0d0) + 19ac kadm_svc_run(0,4b8a8,52340,4aa5c,81010100,ff00) + 198 main(2,ffbefc84,ffbefc90,4a800,0,50620) + 1694 Possible solutions are to fix up the history for all affected principals when the policy changes (rather impractical!) or to make fewer assumptions in add_to_history() and fix the list when the next password change occurs. The following patch should do the latter. I have tried to trigger the same crash after applying this patch and found that the history list is cleanly trimmed, with the most recent keys retained. Index: src/lib/kadm5/srv/svr_principal.c =================================================================== RCS file: /cvs/cd/kerberos/src/lib/kadm5/srv/svr_principal.c,v retrieving revision 1.2 diff -c -r1.2 svr_principal.c *** svr_principal.c 1999/06/17 01:36:31 1.2 --- svr_principal.c 2001/03/16 01:43:07 *************** *** 1001,1006 **** --- 1001,1038 ---- memset(&adb->old_keys[adb->old_key_len],0,sizeof(osa_pw_hist_ent)); adb->old_key_len++; + } else if (adb->old_key_len > pol->pw_history_num-1) { + /* + * The policy must have changed! Shrink the array. + * Can't simply realloc() down, since it might be wrapped. + * To understand the arithmetic below, note that we are + * copying into new positions 0 .. N-1 from old positions + * old_key_next-N .. old_key_next-1, modulo old_key_len, + * where N = pw_history_num - 1 is the length of the + * shortened list. Matt Crawford, FNAL + */ + int j; + histp = (osa_pw_hist_ent *) + malloc((pol->pw_history_num - 1) * sizeof (osa_pw_hist_ent)); + if (histp) { + for (i = 0; i < pol->pw_history_num-1; i++) { + j = (i + adb->old_key_next - (pol->pw_history_num-1)) + % adb->old_key_len; + histp[i] = adb->old_keys[j]; + } + /* Now free the ones we don't keep (the oldest ones) */ + for (i = 0; i < adb->old_key_len - (pol->pw_history_num-1); i++) + for (j = 0; j < adb->old_keys[i].n_key_data; j++) + krb5_free_key_data_contents(context, + &adb->old_keys[i].key_data[j]); + free((void *)adb->old_keys); + adb->old_keys = histp; + adb->old_key_len = pol->pw_history_num - 1; + adb->old_key_next = 0; + } else { + /* All is not lost! (But I dare you to exercise this path.) */ + adb->old_key_next %= adb->old_key_len; + } } /* free the old pw history entry if it contains data */ >Audit-Trail: Verified and reproduced problem, reviewed patch, sent e-mail to Matt Crawford expressing concern that the keys this patch frees are only the right ones in the case where the array hasn't wrapped. --mitchb, 10/11/01 Response e-mail follows: Date: Thu, 11 Oct 2001 14:26:53 -0500 (15:26 EDT) From: Matt Crawford Subject: Re: kadmind potential core dump, versions 1.0.x through 1.2.2 To: Mitchell E Berger > ... > However immediately afterwards, you free the correct number of old keys from > the beginning of the old array. Aren't those only the right keys to free in > the case where the array hadn't wrapped? (If so and the array wrapped, I > haven't looked at the data structures, but you might even be destroying the > keys you just copied over to histp.) I think this needs some modular > arithmetic, but it's possible I'm missing something silly here. Urrr, no, I think you're right. The [i] in krb5_free_key_data_contents(context, &adb->old_keys[i].key_data[j]); needs to be [(i+adb->old_key_next) % adb->old_key_len] "Therre's egg enough for all faces" seems to be a law of nature. Matt Crawford Committed the patch with the following changes: - The arithmetic to calculate which keys needed to be freed was changed to use modular arithmetic, as we discussed in our last e-mail exchange. - Define and undefine a macro KADM_MOD(x) to add adb->old_key_next and modulate by adb->old_key_len since it's ugly and repeated. Undefine this macro after add_to_history. - Added adb->old_key_len to the number we're modulating by adb->old_key_len to get the indices for copying keys into histp because without this, if the array wraps, we can end up with a negative number, and ((-1) % 4) == -1, not 3, so this makes the number always positive and doesn't change the result we want. - Got rid of the else clause that handled histp failing to be malloced because adb->old_key_next %= adb->old_key_len is a noop... the problem in the first place was that adb->old_key_len was too large, and we know adb->old_key_next was smaller than it. This wouldn't avoid the crash. Replaced it with return(ENOMEM), which (amazingly enough for kadmind and clients) causes kpasswd to print the following in this extremely unlikely case: $ kpasswd Password for mitchb/crash@ZONE.MIT.EDU: Enter new password: Enter it again: Server error: Password not changed. Not enough space while trying to change password. --mitchb 10/16/01 Responsible-Changed-From-To: krb5-unassigned->mitchb Responsible-Changed-By: mitchb Responsible-Changed-When: Thu Oct 11 04:11:36 EDT 2001 Responsible-Changed-Why: mitchb is investigating the problem and patch State-Changed-From-To: open-closed State-Changed-By: mitchb State-Changed-When: Tue Oct 16 06:26:13 EDT 2001 State-Changed-Why: I've committed the patch, with some changes noted. >Unformatted: