Skip Menu |
 

Subject: Change in behaviour in gss_release_buffer() by mechtypes introduces memory leak
Download (untitled) / with headers
text/plain 1.1KiB
While running the dejagnu test suite gssftp w/ valgrind - when using the
aes keytypes - a memory leak is introduced by the gss_unseal() - in
particular k5sealv3.c line 399...

Internally - memory is allocated for the output message - and the length
filled in later. If message_buffer->length results in 0 (about line
425) - things get interesting...

The caller uses gss_release_buffer to free memory returned.
Before the mechglue layer - generic/rel_buffer.c would test only if
buffer->value was set - and if so - free the memory and set the length to 0.

In mechglue/g_rel_buffer.c both the buffer->length and value are tested.

The problem: Memory may be allocated in k5sealv3 which will not be freed.

The fundamental question is - if the length field is == 0 - could memory
have been allocated? If so - it should be freed by gss_release_buffer.
If not - then k5sealv3.c should be changed accordingly.

My reading of RFC2744 does not make it clear - but from the RFC 3.2:
"[the buffer] ...which consists of a length field that contains the
total number of bytes in the datum,..."

Which seems to imply that the length contains the bytes in the datum -
but does not indicate if 0 means no allocation...

Ezra
From: Sam Hartman <hartmans@mit.edu>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #5233] Change in behaviour in gss_release_buffer() by mechtypes introduces memory leak
Date: Fri, 29 Dec 2006 14:43:51 -0500
RT-Send-Cc:
Note that callers should not be releasing buffers that they allocated.
So I think we need only be consistent within our implementation and
within mechanisms that plug into our implementation.
Date: Fri, 29 Dec 2006 17:05:35 -0500
From: Ezra Peisach <epeisach@MIT.EDU>
To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #5233] Change in behaviour in gss_release_buffer() by mechtypes introduces memory leak
RT-Send-Cc:
Sam Hartman via RT wrote:
Show quoted text
> Note that callers should not be releasing buffers that they allocated.
> So I think we need only be consistent within our implementation and
> within mechanisms that plug into our implementation.
>
>
Exactly.... The mechanism interface has changed the implementation
requirements. The release_buffer is not passed down to the mech
specific handlers. So - self consistency in the implementation would
now require a change in the k5sealv3.c code...

Ezra
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #5233] Change in behaviour in gss_release_buffer() by mechtypes introduces memory leak
From: Tom Yu <tlyu@MIT.EDU>
Date: Fri, 29 Dec 2006 17:20:44 -0500
RT-Send-Cc:
Show quoted text
>>>>> "Ezra" == Ezra Peisach via RT <rt-comment@krbdev.mit.edu> writes:

Show quoted text
Ezra> Sam Hartman via RT wrote:
Show quoted text
>> Note that callers should not be releasing buffers that they allocated.
>> So I think we need only be consistent within our implementation and
>> within mechanisms that plug into our implementation.
>>
>>
Show quoted text
Ezra> Exactly.... The mechanism interface has changed the implementation
Ezra> requirements. The release_buffer is not passed down to the mech
Ezra> specific handlers. So - self consistency in the implementation would
Ezra> now require a change in the k5sealv3.c code...

I think some interpretations of the spec require that an application
be able to assume that a zero length buffer doesn't need to be
released. We should probably adjust k5sealv3.c to be consistent with
that.
Date: Fri, 29 Dec 2006 18:17:50 -0500
From: Ezra Peisach <epeisach@MIT.EDU>
To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #5233] Change in behaviour in gss_release_buffer() by mechtypes introduces memory leak
RT-Send-Cc:
Well - here is my patch.... Appears to work w/o complaint w/ gssftp....


Included is another memory leak patch - on error case....

Index: krb5/k5sealv3.c
===================================================================
--- krb5/k5sealv3.c (revision 19019)
+++ krb5/k5sealv3.c (working copy)
@@ -412,10 +412,16 @@
if (load_16_be(althdr) != 0x0504
|| althdr[2] != ptr[2]
|| althdr[3] != ptr[3]
- || memcmp(althdr+8, ptr+8, 8))
+ || memcmp(althdr+8, ptr+8, 8)) {
+ free(plain.data);
goto defective;
+ }
message_buffer->value = plain.data;
message_buffer->length = plain.length - ec - 16;
+ if(message_buffer->length == 0) {
+ free(message_buffer->value);
+ message_buffer->value = NULL;
+ }
} else {
/* no confidentiality */
if (conf_state)
From: epeisach@mit.edu
Subject: SVN Commit
If gss_krb5int_unseal_token_v3() unwraps a message of length 0 - free
memory and return in message_buffer a NULL pointer for value. This
is consistant with gss_release_buffer in the mechglue implementation in which
memory is only freed if the buffer length != 0.


Commit By: epeisach



Revision: 19022
Changed Files:
U trunk/src/lib/gssapi/krb5/k5sealv3.c
From: tlyu@mit.edu
Subject: SVN Commit
pull up r19022 from trunk

r19022@cathode-dark-space: epeisach | 2006-12-30 01:09:25 -0500
ticket: 5233
tags: pullup

If gss_krb5int_unseal_token_v3() unwraps a message of length 0 - free
memory and return in message_buffer a NULL pointer for value. This
is consistant with gss_release_buffer in the mechglue implementation in which
memory is only freed if the buffer length != 0.




Commit By: tlyu



Revision: 19074
Changed Files:
_U branches/krb5-1-6/
U branches/krb5-1-6/src/lib/gssapi/krb5/k5sealv3.c