Alan Cronin (alcronin) wrote:
Hi Howard,
I can change the code so that it overwrites the memory from
ldap_free_request_int() instead. This will cover the execution using a bind (both synchronous and asynchronous). Is there any other places where the password would be in the buffer that needs to be cleared? The reason for inserting it into ber_free_buf() was that it would clear all places.
I see, you're looking at the client side. What about the slapd side?
There are 2 main places where passwords occur - Bind, and PasswordModify exop. They might also occur in Add and Modify requests. Add is not so unusual, Modify should not happen since clients should use PasswordModify instead.
Some compilers will optimize calls to memset() so that only the first
character will be changed. Leaving the rest of the string intact.
That's clearly a compiler bug then. If you have examples of such situations you should report that to the respective compiler maintainers.
This is why
the iteration is in place.
Thanks for looking into this patch.
Alan
On 05/08/2016, 19:50, "Howard Chu" hyc@symas.com wrote:
alcronin@cisco.com wrote: > Full_Name: Alan Cronin > Version: 2.4.44 > OS: Windows 8.1 > URL: https://dl.dropboxusercontent.com/u/82343475/SecurelyEraseBuffer.diff > Submission from: (NULL) (2001:420:4041:2003:f942:59a5:545f:b334) > > > The following patch is a modification to the OpenLDAP BerElement buffer. The > buffer can be used to contain the LDAP request including the password for > authentication. While this is free'd when it is no longer needed, the contents > of the buffer is not overwritten from memory. This can lead to someone reading > the memory of the process and determining what the password is. The change > included in this patch will iterate over the memory and clear it. This will > remove any trace of the password by the time execution is handed back to the > caller. Why would you insert a performance pessimization into every use of the LBER library, rather than just erasing the password from a Bind request? Why would you use an explicitly coded loop setting one character at a time, instead of using libc's memset() which has probably been well optimized? > The attached patch file is derived from OpenLDAP Software. All of the > modifications to OpenLDAP Software represented in the following patch(es) were > developed by Alan Cronin alcronin@cisco.com. I have not assigned rights and/or > interest in this work to any party. > The attached file is derived from OpenLDAP Software. All of the modifications to > OpenLDAP Software represented in the following patch(es) were developed by Cisco > Systems, Inc.. Cisco Systems, Inc. has not assigned rights and/or interest in > this work to any party. I, Alan Cronin am authorized by Cisco Systems, Inc., my > employer, to release this work under the following terms. > Cisco Systems, Inc. hereby place the following modifications to OpenLDAP > Software (and only these modifications) into the public domain. Hence, these > modifications may be freely used and/or redistributed for any purpose with or > without attribution and/or other notice. > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/