[SSSD] [PATCH] libcrypto fully implemented

Stephen Gallagher sgallagh at redhat.com
Tue Jun 26 13:01:32 UTC 2012


On Mon, 2012-06-25 at 16:04 -0500, George McCollister wrote:
> 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;
> 

Take a look at test_murmurhash3_random() in src/tests/utils-tests.c

That would be the best approach. Then we can test that random binary
data of arbitrary random length encodes and decodes properly. In order
to reproduce the issue, we should have it print the seed it was using if
we fail.

As I said before, this can be added on later. The current tests are
sufficient for the moment, so it won't hold up the push.

> >
> > 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.

FWIW, I added these two lines to my ~/.vimrc:

highlight ExtraWhitespace ctermbg=red guibg=red
match ExtraWhitespace /\t\|\r\|\S.*\s\+$\|^\s\s\+$\|\%>79v.\+/

It lights up text in red if there are tabs, non-UNIX line-endings,
whitespace at the end of a line, lines containing nothing but whitespace
and lines exceeding 79 characters. Very helpful for sticking to the
coding style.

> > 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.
> >

Yup, looks good to me.

> >
> > 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? :)

It's a completely useless feature, especially since the sssd.conf cannot
be read by anyone but root.

> >
> > 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 :)

Sure, that file was written eons ago and we haven't had a chance to
convert it yet :)

Ok, so there's one or two minor whitespace issues and I fixed the
copyright headers (discussed with George on IRC to eliminate any
confusion).

Ack and pushed to master.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120626/5d6082e4/attachment.sig>


More information about the sssd-devel mailing list