Skip Menu |
 

Date: Fri, 3 Feb 2012 15:01:19 +0000
From: Ian Abbott <abbotti@mev.co.uk>
To: krb5-bugs@mit.edu
Subject: [BUG krb5-1.10] krb5_gss_get_name_attribute
Hi krb5 maintainers,

I think there is a bug in krb5_gss_get_name_attribute introduced in
release 1.10 around lines 389-394 of src/lib/gssapi/krb5/naming_exts.c:

if (display_value != NULL) {
if (code != 0)
code = data_to_gss(&kdisplay_value, display_value);
else
free(kdisplay_value.data);
}

I think the "if (code != 0)" test needs to be inverted, otherwise
*display_value is never set when the function returns 0 for success.


I found this when trying to figure out why Samba3's smbd was crapping
out on me. It called gss_get_name_attribute with display_value pointing
to an uninitialized gss_buffer_t variable on the stack and later passed
a pointer to the same variable to gss_release_buffer() which caused
glib's free() to abort the process because display_value->value was an
uninitialized pointer.

Best regards,
Ian Abbott.

--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
Date: Mon, 6 Feb 2012 11:38:38 +0000
From: Ian Abbott <abbotti@mev.co.uk>
To: "rt@krbdev.mit.edu" <rt@krbdev.mit.edu>
Subject: Re: [krbdev.mit.edu #7087] AutoReply: [BUG krb5-1.10] krb5_gss_get_name_attribute
RT-Send-Cc:
Download (untitled) / with headers
text/plain 1.1KiB
Show quoted text
> if (display_value != NULL) {
> if (code != 0)
> code = data_to_gss(&kdisplay_value, display_value);
> else
> free(kdisplay_value.data);
> }

Also, in the same region of code:

If the call data_to_gss(&kdisplay_value, display_value) fails with an
error (which can only happen #ifdef _WIN32), the preceding call
data_to_gss(&kvalue, value) has now passed responsibility for the buffer
value->value to the caller even though krb5_gss_get_name_attribute() is
returning a failure code. In this case, one solution would be to free
value->value, and since kvalue now has empty data, repeat the call
data_to_gss(&kvalue, value) which shouldn't fail as it shouldn't have to
allocate anything.

if (display_value != NULL) {
if (code == 0) {
code = data_to_gss(&kdisplay_value, display_value);
if (code != 0 && value != NULL) {
/* Cleanup. N.B. kvalue is empty_data() */
free(value->value);
data_to_gss(&kvalue, value);
}
}
else
free(kdisplay_value.data);
}
From: ghudson@mit.edu
Subject: SVN Commit

Set display_value in krb5_gss_get_name_attribute

A backwards conditional in r25358 caused krb5_gss_get_name_attribute
not to set display_value on success. Fix the sense of the
conditional.

We still don't quite correctly handle the cases where data_to_gss()
fails, but those should be rare and the problem in those cases isn't
severe, so it can be fixed separately.

Also, value and display_value should probably be initialized to null
buffers on failure, as is common with GSS interfaces.

https://github.com/krb5/krb5/commit/08d7bd5e79f8b895405ba375947c8b825976c837
Commit By: ghudson
Revision: 25674
Changed Files:
U trunk/src/lib/gssapi/krb5/naming_exts.c
This is marked for pullup with with status "open". We should have a new ticket for the remaining
issues if we want the commit pulled up as-is.
I've created issue #7089 (with fix) for the lack of buffer initialization
in the mechglue gss_get_name_attribute, and issue #7090 for the minor
cleanup issue if the second data_to_gss call fails. Changing the status
of this issue to review.
From: tlyu@mit.edu
Subject: SVN Commit

Pull up r25674 from trunk

------------------------------------------------------------------------
r25674 | ghudson | 2012-02-06 18:19:08 -0500 (Mon, 06 Feb 2012) | 18 lines

ticket: 7087
status: open
target_version: 1.10.1
tags: pullup

Set display_value in krb5_gss_get_name_attribute

A backwards conditional in r25358 caused krb5_gss_get_name_attribute
not to set display_value on success. Fix the sense of the
conditional.

We still don't quite correctly handle the cases where data_to_gss()
fails, but those should be rare and the problem in those cases isn't
severe, so it can be fixed separately.

Also, value and display_value should probably be initialized to null
buffers on failure, as is common with GSS interfaces.

https://github.com/krb5/krb5/commit/ee7187c23249334e66d810c9e0a388a0518e0729
Commit By: tlyu
Revision: 25707
Changed Files:
U branches/krb5-1-10/src/lib/gssapi/krb5/naming_exts.c
From: tlyu@mit.edu
Subject: SVN Commit

Ian Abbott reported the bug where krb5_gss_get_name_attribute fails to
set display_value.

https://github.com/krb5/krb5/commit/2e562c2befc03ac3f3aaa43e6fa04269583ef744
Commit By: tlyu
Revision: 25754
Changed Files:
U branches/krb5-1-10/README