First of all, this is pretty good stuff to get it working. Looks pretty
good for first cut. I'm giving you conditional ack pending addressing
key issues below.
Christina
=========
APDU.java
*I don't see getCalculateSingleDesBeforeMac() or
setCalculateSingleDesBeforeMac() being used. calculateSingleDesBeforeMac
seems to be hard-coded in every APDU, and I am not seeing it used anywhere.
* it seems the only difference between the existing secureMessage() and
your secureMessageSCP02() is very miner:
if (rem == 0) {
+
+ padNeeded = 8;
instead of
padNeeded = 0;
Couldn't this be handled by modifying the existing function?
DeleteFileGP211APDU.java
* seems to be identical with DeleteFileAPDU.java, with the exception of
the unused variable mentioned above:
+ calculateSingleDesBeforeMac = true;
but maybe it's not a bad thing to have separate APDU files.
ExternalAuthenticateAPDUGP211.java
* did you intentionally leave out CMAC_RMAC in byteToSecurityLevel()?
* Please provide comment to new functions describing at least what the
params are for
Util.java
* computeEncEcbDes()
You should add a // TODO comment there just to be sure we don't ship
it with secret keys printed to the debug log.
+ TPSBuffer desDebug = new TPSBuffer(des.getEncoded());
+
+ CMS.debug("des key debug bytes: " + desDebug.toHexString());
+
TokenServlet.java
* processComputeSessionKeySCP02() is a copy of
processComputeSessionKey() with mods:
I am not in favor of making a copy of an existing function and make
changes to it. It is very difficult to look through it to visually find
diffs. Also, if we ever need to fix anything it needs to go to multiple
places. This seems to be the practice in this patch.
* I am not in favor of reformatting the entire existing file when you
modify it. It makes it really difficult to sift through for the real
changes. If you really wish to reformat it, try to make two separate
patches just to save your reviewer's time.
(I did what I could visually hunting for diffs due to this, it is
possible I missed review something..)
DeriveDESKeyFrom3DesKey
* the caller to DeriveDESKeyFrom3DesKey (Util.java:computeMACdes3des())
seems to hard code CKM_DES_CBC, where do you expect one to change the
alg? or is this for future?
SymKey.java
* why does this added line do?
+ transportKey = ReturnSymKey( slot, GetSharedSecretKeyName(NULL));
* why can't you just use CreateUnWrappedSymKeyOnToken() and you have to
write UnwrapWrappedSymKeyOnToken()? They look awfully similar.
On 02/23/2015 10:36 PM, John Magne wrote:
Subject: [PATCH] Ticket: TPS Rewrite: Implement Secure Channel
Protocol 02
(#883).
First cut of gp211 and secure channel protocol 02 for tokens.
Allow token operations using a GP211 token over secure channel protocol 02.
This patch supports the following:
1. Token operations with a GP211 card and SCP02 protocol, implementation 15.
2. Token still supports GP201 cards with SCP01.
3. SCP02 tested with SC650 gp211/scp02 card.
4. Formatting ,e nrollment, and pin reset tested to work.
5. Symmetric key changeover upgrade and downgrade tested to work.
6. APDU security methods of CMAC and Encryption supported, as well as the combination
of the two.
Things still to do:
1. Right now the SCP02 support has been tested with the current gp201 applet and
enrollment and formatting works just fine. We need to modify and compile the applet
against the GP211 spec and retest to see if any further changes are needed.
2. The nistSP800 key derivation stuff is not completed for the SCP02 protocol. Some
of the routines are self contained vs similar SCP01 ones. We have another ticket to
complete the nistSP800 support from end to end. This work will be done for that ticket.
3. One of the new scp02 deriviation functions can make use of a new NSS derive
mechanism.
As of now this work is done by simple encryption, this can be done later.
4. The security APDU level of "RMAC" is not supported because the card does not
support it.
It could have been done to the spec, but it having the card to test is more convenient
and there
were more crucial issues to this point.
5. Right now the security apdu level is set hard coded to the max supported level for
scp01 and 02.
Need to make that more configurable, although there is no reason to downgrade the apdu
security.
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel