Skip Menu |

Download (untitled) / with headers
text/plain 8.2KiB
From Fri Mar 16 10:53:37 2001
by (8.9.3/8.9.3) with ESMTP id KAA01680
for <>; Fri, 16 Mar 2001 10:53:37 -0500 (EST)
Received: from ( [])
by (8.9.2/8.9.2) with ESMTP id KAA00826;
Fri, 16 Mar 2001 10:35:32 -0500 (EST)
Received: (from crawdad@localhost)
by (8.9.1/8.9.1) id JAA03408;
Fri, 16 Mar 2001 09:35:31 -0600 (CST)
Message-Id: <>
Date: Fri, 16 Mar 2001 09:35:31 -0600 (CST)
Subject: kadmind potential core dump, versions 1.0.x through 1.2.2
X-Send-Pr-Version: 3.99

Show quoted text
>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
Show quoted text
>Release: krb5-1.2.2 (and earlier)
Observed on Solaris 2.7, should affect all platforms
System: SunOS 5.5.1 Generic_103640-24 sun4u sparc SUNW,Ultra-1
Architecture: sun4

Show quoted text
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.
Show quoted text
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.
Show quoted text

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 ----

+ } 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 */
Show quoted text
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 <mitchb@MIT.EDU>

Show quoted text
> ...
> 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


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
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
I've committed the patch, with some changes noted.

Show quoted text