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