[Pki-devel] [pki-devel][PATCH] 0024-Ticket-TPS-Rewrite-Implement-Secure-Channel-Protocol.patch

[Christina Fu]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20150224/2a748c02/attachment.htm>