gp_creds: fix use-after-free in gp_count_tickets() - #133
Conversation
| } | ||
|
|
||
| do { | ||
| for (;;) { |
There was a problem hiding this comment.
if we are going to switch to a for loop, then please use:
for (err = 0; err == 0;
err = krb5_cc_next_cred(context, ccache, &cursor, &creds)) {
There was a problem hiding this comment.
in fact we can actually write this as:
for (err = 0; err == 0;
err = krb5_cc_next_cred(context, ccache, &cursor, &creds)) {
if (err == 0) {
krb5_free_cred_contents(context, &creds);
(*ccsum)++;
}
}
if (err != KRB5_CC_END) {
ret_min = err;
ret_maj = GSS_S_FAILURE;
goto done;
}
^ This is my preferred and more understandable form.
aeaedb7 to
e45faca
Compare
|
Thanks Simo — applied exactly that form in e45faca (amended): the loop initializer/step drives |
|
Looks like this change still has some issues, please test locally |
e45faca to
ccf6ae7
Compare
|
Thanks for the second look — the remaining defects were real. Amend ccf6ae7 fixes both:
Local verification (no krb5kdc on this box, so
|
The original do-while pattern calls krb5_free_cred_contents() on the krb5_creds buffer even when krb5_cc_next_cred() reports KRB5_CC_END, i.e. after the iterator already returned without writing the buffer: the terminal iteration frees either freed or stale contents. Reshape as a while loop whose condition performs the iterator call so the body only runs on success, keep error handling after the loop, and terminate the cursor on the mid-iteration error path (the earlier goto-done skipped krb5_cc_end_seq_get, leaking the cursor). Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>
The counting loop in
gp_count_tickets()callskrb5_free_cred_contents(&creds)and increments the counter even whenkrb5_cc_next_cred()has already failed — includingKRB5_CC_END, where it then continues iterating.krb5_creds.*ccsumcomes out one too high.Only free and count after a successful fetch, and leave the loop at
KRB5_CC_END.Built clean with
--disable-public-librariesetc. default options (-Wall -Wextraquiet), and the counting behavior was traced for the empty-ccache, zero-ticket and multi-ticket cases against the MIT Kerberos semantics ofkrb5_cc_next_cred()(KRB5_CC_END is the normal end-of-cache signal and must not count nor free).