Krishna, Thanks for your review. I updated the webrev with your review comments.
http://cr.opensolaris.org/~haimay/CR6703956-v2/ Krishna Yenduri wrote: > Hai-May Chao wrote: >> >> I updated my webrev with review comments from Dan and Ferenc. >> The updated webrev is at: >> >> http://cr.opensolaris.org/~haimay/CR6703956-v1/ >> > > usr/src/common/crypto/rng/fips_random.c > > KY-0 lines 68-137 > Please file an RFE to implement the revised version > of the FIPS algorithm once this code is integrated. > ok. > I have looked at the revised algorithm and it can be > easily implemented with a couple of changes to this > code. > > KY-1 T3 lines 33-34, 106, 135 > fips_add160() expects val1 and val2 to be in big-endian > format. I assume the correctness of the operation depends > on that. > > If so, you need to do some conversion for little-endian > case, on lines 106 and 135. > No changes made per email exchange. > > usr/src/common/crypto/rng/fips_random.h > > KY-2 E3 line 36 > Nit: make this (SHA1BLOCKBITS >> 3) > Fixed. > > usr/src/uts/common/crypto/io/swrand.c > > KY-3 T3 line 387 > Why not use digest directly and get rid of digest32 > since they are of the same size? You can then > remove the bcopy() here. > Originally I coded just like you indicated and got lint warning due to pointer cast. So I changed it to as it is now. > > KY-4 T3 line 388 > fips_random_inner() requires the caller (per the comments) > to bzero the XSEED_j buffer after returning. So, need to > bzero digest32 after this call. > Fixed. > KY-5 T3 lines 406-407 > I assume FIPS compliance would require you to stop using > the provider in this case (or fail all the requests from then on). > I see that n2rng unregisters from the framework > in error cases. > > We can have the default behavior as now ("log a warning") > and add an option (a kernel tunable) to stop using swrand when we hit > this error. > If we have swrand and one or more hardware random number providers, then I think it'd be okay to disable the provider that fails. If we only have swrand, i.e. disabling the last remaining provider doesn't seem like a good idea. The kernel tunable option could be added later as part of FIPS 140-2 Self-Tests work. I'd like to suggest we provide logging a warning at this putback as we're getting the FIPS 186-2 RNG into the framework. > > usr/src/uts/common/crypto/api/kcf_random.c > > KY-6 E4 line 590 > s/FIP XKEY/FIPS XKEY/ > Fixed. > > KY-7 T3 lines 667-668 > I believe it is much more secure to have a different XSEED for > every call. > > I would recommend XOR'ing the seed value with a high > resolution timer output (gethrtime()) for this call. > Fixed. > > KY-8 T3 lines 687-688 > Same issue as KY-5 > Please see KY-5. > > KY-9 T4 lines 817-818 > Recommend changing rmp->rm_mag.rm_key to a local > buffer (say discard_buf) to make it clear that > we are just discarding the output. Also, add a > comment on why we are discarding it. > Fixed. Thanks, Hai-May
