[SSSD] [PATCH] libcrypto fully implemented

George McCollister george.mccollister at gmail.com
Mon Jun 25 21:04:08 UTC 2012


On 06/25/2012 07:08 AM, Stephen Gallagher wrote:
> On Tue, 2012-06-19 at 13:28 -0500, George McCollister wrote:
>> I implemented missing libcrypto functionality.
>
> Thank you very much for this patch, George!
>
> This isn't a blocker for this patch, but would you mind expanding the
> base64 encode/decode tests? I'd like to see a situation where you read a
> dozen or so random bytes and then pass it through both encode and decode
> to confirm that it returns the original byte array. If we do this with
> random data, it could conceivably discover issues that are not present
> when managing only a known ASCII string. As I said, I'm not nacking the
> patch based on this, but it would be a nice future enhancement to the
> tests.
What facility would be used to generate the random bytes (srand/rand, 
/dev/urandom or some SSSD API)? Is it a good idea to use random data in 
the test generated at the time of test? If a certain random string did 
generate a failure it might be difficult to reproduce, leading to 
confusion.

We could also just do something simple to catch all possible characters:
int i;
unsigned char data[256];
for(i=0;i<256;i++) data[i]=i;

>
> Ok, so a few NACKs:
>
> Minor: you have non-zero-length lines containing only whitespace. This
> is a style violation (and shows up bright red in my VIM).
Doesn't show up in red in my VIM (that would be helpful); I think I was 
able to find all of the cases using grep and a regular expression.
> In sss_base64_encode(), you return NULL if
> bmem = BIO_new(BIO_s_mem());
> fails, but this leaks the memory of b64.
Good catch, fixed.
>
> Please use more whitespace to logically separate your actions in
> sss_base64_encode() and sss_base64_decode(). They're bunched together
> and it's difficult to see what actually goes together.
Done
>
> In sss_base64_decode(), please have tmp_ctx be a child of NULL, not
> mem_ctx. This makes it possible for valgrind to detect memory leaks.
Done
>
> When you do
> in_dup = talloc_size(mem_ctx, inlen+1);
> this should be parented to tmp_ctx, not mem_ctx. You're leaking this.
Good catch, fixed
> If you 'goto done' in multiple places, you are leaking memory created by
> BIO_new(). Please make sure that you clean up after yourself on error.
I'm 99 percent sure I have this correct now. Looks okay in valgrind.
>
>
> Other than a blank line with whitespace, the HMAC-SHA1 portion looks
> good.
>
> The configure script warns that building with libcrypto will not support
> password obfuscation, but your patch implements it. One of these are
> lying :)
I removed the warning in configure.ac
> (For the record, if you opted not to implement this misfeature,
> I wouldn't have shed a tear).
What? You're not a fan of security through obscurity? :)
>
> As above, please allocate tmp_ctx on NULL for improved memory-leak
> detection. The only time mem_ctx should ever be used is when all data is
> finalized and ready to be returned.
Done
>
> Please use SSSDBG_* macros when adding DEBUG messages. They are more
> readable (and help us actually stick to reasonable meanings for the
> levels). In sss_password_encrypt() they should be SSSDBG_CRIT_FAILURE
> and SSSDBG_TRACE_FUNC
Fixed. In my defense I was following a bad example set by 
src/util/crypto/nss/nss_obfuscate.c :)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libcrypto-fully-implemented.patch
Type: text/x-patch
Size: 19939 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120625/ccb04b86/attachment.bin>


More information about the sssd-devel mailing list