Hi,
First of all, I have to say that Jack did a wonderful job on such
daunting task. The sheer amount of code and complexity does make the
review more challenging, but I dug through them with my teeth and claws
regardless ;-).
We discussed and think we should postpone the checkin to next release so
we can make sure it gets the kind of attention in details that it deserves.
For the first round of reviews, I sent him two separate sets of review
comments last week. One for JSS, and one for the rest.
The JSS patch was not attached to his original email request for
review. It is attached to the following ticket:
https://fedorahosted.org/pki/ticket/801
You can find my review comments attached to this email.
thanks,
Christina
On 04/15/2016 07:03 PM, John Magne wrote:
Subject: [PATCH] Port symkey JNI to Java classes. Ticket #801 : Merge
pki-symkey into jss
What is supported:
1. Everything that is needed to support Secure Channel Protocol 01.
2. Supports the nist sp800 kdf and the original kdf.
3. Supports key unwrapping used by TPS which was formerly in the symkey JNI.
Requires:
1. A new JSS that supports more advanced symkey operations such as key
derivation, more advanced key
unwrapping , and a way to list and identify a given symmetric key by name.
Version of new Jss will be forthcoming.
Still to do:
1. Port over the 2 or 3 SCP02 routines from Symkey to use this code.
2. The original symkey will remain in place until we can port over everything.
3. SCP03 support can be added later.
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
--- Begin Message ---
Sending this out to you just to get it off my back...
this is just the JSS part.
Some of these already communicated to you. I'm just listing them out to
track for the re-review.
==============
JSS
*PK11KeyWrapper.java
- The original unwrapSymmetric) assumes temporary true, and you want
to have a function that treats it as false.
You made a copy of the original and named it unwrapSymmetricPerm()
and set the temporary to false.
- I think it'd be better if you just add a "temporary" param to the
original function, and add the function with original signature to call
into the one with temporary true; Then you call into the new signature
with temporary false (instead of adding this extra unwrapSymmetricPerm()
* Java_org_mozilla_jss_pkcs11_PK11Store_putSymKeysInVector
This function gets a list of sym keys from a token and put them into a
vector. It seems more appropriate to be called
getSymKeysInVector() ("put" made me think initially that you are
putting keys into the token)
* PK11SymKey.c: JSS_PK11_wrapSymKey
- Are we sure that all sym keys have nicknames?
Should we maintain the original "no nickname" code path by checking
if nickname is null and call original calls?
* in Java_org_mozilla_jss_pkcs11_PK11SymKey_setNickNameNative
/* name the key */
status = PK11_SetSymKeyNickname( key, keyname );
if( status != SECSuccess ) {
JSS_throwMsgPrErr(env, TOKEN_EXCEPTION,
"Failed to name symmetric key");
- is there not an error code that you can get and throw back to make
it more useful?
I noticed this applies to many other areas as well. it's a good idea
to get errorcode to throw back
* setNickName
- you might want to check if nickname is null before passing it down to C
* might want to check if new JSS files created should bear the same
Netscape etc. license
*PK11SymmetricKeyDeriver.c : bestSlot =
PK11_GetBestSlot(deriveMechanism, NULL);
should test it out on hsm (already communicated to jack... item here
to track)
- suggest if issues found, revert to old code (or code similar to old
code) and create a separate ticket to tackle
- new code should test to work on both nethsm and lunasa
* the test SymKeyDeriving seems to fail at NSS init (Jack found the hard
coded lib): tracking here
--- End Message ---
--- Begin Message ---
=========
SecureChannelProtocol.java
** diversifyKey()
- I don't see KDD being checked for null when conext is being
assigned to it.
- in the 3 calls you made to standardKDF.getDiversificationData(),
first param you passed in is "KDD". Could you mean "context"? because
if nistSP800_108KdfUseCuidAsKdd is true, then your KDD is most
likely null, isn't it?
- I got so confused with these standardKDF.getDiversificationData() calls.
reason being that the getDiversificationData() is really not
specific to "standardKDF". It is used and used if you choose
NistSP800_108KDF too!!!
How about moving the getDiversificationData() method to the base KDF
class instead?
*computeSessionKey_SCP02()
note yet implemented, but your debug log says it's SCP01.
**computeMAC_SCP01()
- are you sure the cipher needs to be initialized inside the loop
instead of before the loop?
cipher.initEncrypt(symKey);
- also, oddly, I don't see any update() being called on the cipher so
how does it work?
(for the reference, the old symkey does a call to
PK11_CreateContextBySymKey() before the loop then PK11_CipherOp() within
the loop, then PK11_Finalize() after the loop... which is more what I am
used to see)
** computeDes3EcbEncryption() - this has similar symptom as the above.
It's missing the PK11_CipherOp() call .. and it doesn't even break the
input down into blocks. Maybe we can ask Relyea what this really means
if you do ECB without breaking it into blocks.
*sha256HMAC() please copy the comments for the params over from
NistSP800_108KDF.cpp: SHA256HMAC
they make reading the code so much easier.
* kdf_CM_SHA256_HMAC_L384 - method name in debug is wrong
** sha256HMAC() - going down initHMAC() to JNI c code is quite interesting.
In Java_org_mozilla_jss_pkcs11_PK11MessageDigest_initHMAC
This is called: newKey = PK11_CopySymKeyForSigning(origKey, mech);
So, the master key is copied somewhere as a new key? Is this even
allowed? and why?
I wonder if this is going to work in FIPS.
just FYI, old symkey code does not do that. It just calls
PK11_CreateContextBySymKey() directly.
** I think it's very good that you have one single
computeSessionKey_SCP01() instead of the original 3.
However, it is still called three times in TokenServlet, and each time
all three are calculated again.
May I suggest that you
1. turn the params of computeSessionKey_SCP01() into part of
SecureChannelProtocol.init() or constructor, passing in all three dev
key arrays.
2. computeSessionKey_SCP01() just needs to be called once; detect if
the 3 session keys were already calculated and not calculate again if
called again.
3. add getKekSessionKey(), getEncSessionKey(), and getMacSessionKey() to
SecureChannelProtocol class, which will just be simple return methods to
return previously calculated keys
** What's confusing to me is the following:
The old SessionKey.ComputeKekKey() call is replaced with
protocol.computeSessionKey_SCP01() in case of server side key generation.
But, the old SessionKey.ComputeKekKey() returns the actual kek key
handle, while the protocol.computeSessionKey_SCP01() returns a session
key derived based on the card keys (with host and card challenges). How
could server side key gen work with the new code?
I may be misreading the code though... need some investigation here.
* May I suggest that you rename SecureChannelProtocol.wrapSessionKey to
wrapSessionKeyWithSharedSecret()? Just to make it clear.
* suggestion to define cryptogram type instead of the original 0 and 1
** in computeCryptogram_SCP01(), authKey is used for computeMAC_SCP01(),
which is used directly for encryption,
but in old code:
DeriveKey() is called with authKey before ComputeMAC().
Did I miss something?
* Is there a reason to use a negative boolean? noDerive
* Could you do a "setSharedSecretKey(token, keyNickName)" so it can be
called in TokenServlet.java?
then in SecureChannelProtocol, you won' t have this weird
getSharedSecretKey(token) that takes just one param token
transportKey = getSharedSecretKey(token);
instead just do
transportKey = getSharedSecretKey(); which would just give you the
exact token:keyNickaname.
and do away with getSharedSecretKeyName()
That seems to make better sense.
** in getKeyName()
Are you sure this line will have the effect of e.g. #02#01? Is this
line tested?
keyName = "#" + keyVersion[0] + "#" + keyVersion[1];
* There are four versions of unwrapWrappedSymKeyOnToken(). Is that
really necessary? The original Symkey code is so much easier to read
and understand without being sent through so many layers of abstraction.
For example, the original code checks for transportKey early on and
bails out if not there. The new code goes into multiple layers of code
before it finds out it's not there. If you look closely at the first
copy of unwrapWrappedSymKeyOnToken() it calls, other than calling to
retrieve the transportKey, it pretty much does nothing other than
sending you down to next copy of unwrapWrappedSymKeyOnToken(). What is
even the purpose of having such abstraction?
- another suggestion is to put a comment there to let know that we
know what we are doing: in case when devKey = getSymKeyByName(token,
devKeyName); returns null for devKey, I think that means the master key
does not exist in the TKS token. Which is (unless it's misconfig) most
likely the scenario that it's using the default keySet coming from TKS
CS.cfg. Those are sym key bytes in the clear. But they are also WELL
KNOWN keys and are expected to be replaced with new master keys for
serious sites.
The transportKey is just used to unwrap onto the token so that the sym
keys could be used as permanent keys.
* in makeDes3KeyDerivedFromDes2, please add a comment to explain that
you are adding the first 8bytes of the key to the end of the original 16
byte key. Such comment exists in the original code and makes things
easier to understand.
* in computeSessionKey_SCP01, not sure why this has to be called again,
especially the return value isn't even caught:
+ // Create the auth with is the same as enc, might need it
later.
+ if (keyType.equals(encType)) {
+ returnDeveloperSymKey(token, authType, keySet,
devKeyArray);
+ }
* In general, you seem to lose all or most comments. not a good idea.
for example,
in old code, NistSP800_108KDF.cpp: KDF_CM_SHA256HMAC_L384
The line:
// hmac_data_input = i || label || 0x00 || context || L
tells me right away the format of the hmac data input.
You lost that line of comment in NistSP800_108KDF.java:
kdf_CM_SHA256_HMAC_L384
And in the immediate next function sha256HMAC, it's clear from original
code that it expects 32 byte output, but yours did not mention it nor
does it do any control/check of the size .
* On the output of old KDF_CM_SHA256HMAC_L384, it returns the 384 bits
(2 bytes each key),
while in your kdf_CM_SHA256_HMAC_L384, if I read it correctly, you
return the entire 512 bits. Of course only the needed bytes are
extracted later after return, but the name of the function
kdf_CM_SHA256_HMAC_L384() is now misleading.
- one other thing, the log says "bytes", it should be "bits" in
kdf_CM_SHA256_HMAC_L384()
* KDF.java: getDesParity does not check if the key length like the old
code NistSP800_108KDF.cpp: set_des_parity
old:
if(length != 2*8){
throw std::runtime_error("set_des_parity failed: wrong key size");
}
new:
if (key == null
|| (key.length != SecureChannelProtocol.DES2_LENGTH &&
key.length != SecureChannelProtocol.EIGHT_BYTES)) {
throw new EBaseException(method + " Incorrect input key !");
}
- Also, it seems the original function name seems to be more
appropriate, as the purpose of the function is to set parity on the key
I suggest to rename it setDesParity()
- don't print the key desKey to debug log
** need to zero out outputHMAC256 in kdf_CM_SHA256_HMAC_L384
and zero out kdf_output in computeCardKeys
* I'm not sure I see the need to split up into these two functions:
computeCardKeyOnSoftToken()
computeCardKeyOnToken()
seems could be easily handled in one single function (goes back to my
issue with the "little functions" that have very little reasons for
their existence)
--- End Message ---
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
