From 06b5a878c5b170d86ba1c7841b7155da21b8e459 Mon Sep 17 00:00:00 2001 From: Nikita Mikhailov Date: Thu, 18 Aug 2016 00:12:19 +0700 Subject: [PATCH 01/10] SecurityToken: more robust voltage selection --- .../securitytoken/usb/CcidDescription.java | 143 ++++++++++++++++++ .../securitytoken/usb/CcidTransceiver.java | 51 ++++++- .../securitytoken/usb/UsbTransport.java | 63 +------- .../usb/UsbTransportException.java | 9 +- 4 files changed, 193 insertions(+), 73 deletions(-) create mode 100644 OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java new file mode 100644 index 000000000..5a30c830f --- /dev/null +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java @@ -0,0 +1,143 @@ +/* + * Copyright (C) 2016 Nikita Mikhailov + * Copyright (C) 2017 Vincent Breitmoser + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +package org.sufficientlysecure.keychain.securitytoken.usb; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.ArrayList; + +import android.support.annotation.NonNull; + +import com.google.auto.value.AutoValue; +import org.sufficientlysecure.keychain.securitytoken.usb.tpdu.T1ShortApduProtocol; +import org.sufficientlysecure.keychain.securitytoken.usb.tpdu.T1TpduProtocol; + + +@AutoValue +abstract class CcidDescription { + private static final int DESCRIPTOR_LENGTH = 0x36; + private static final int DESCRIPTOR_TYPE = 0x21; + + // dwFeatures Masks + private static final int FEATURE_AUTOMATIC_VOLTAGE = 0x00008; + private static final int FEATURE_EXCHANGE_LEVEL_TPDU = 0x10000; + private static final int FEATURE_EXCHAGE_LEVEL_SHORT_APDU = 0x20000; + private static final int FEATURE_EXCHAGE_LEVEL_EXTENDED_APDU = 0x40000; + + // bVoltageSupport Masks + private static final byte VOLTAGE_5V = 1; + private static final byte VOLTAGE_3V = 2; + private static final byte VOLTAGE_1_8V = 4; + + private static final int SLOT_OFFSET = 4; + private static final int FEATURES_OFFSET = 40; + private static final short MASK_T1_PROTO = 2; + + public abstract byte getMaxSlotIndex(); + public abstract byte getVoltageSupport(); + public abstract int getProtocols(); + public abstract int getFeatures(); + + @NonNull + static CcidDescription fromRawDescriptors(byte[] desc) throws UsbTransportException { + int dwProtocols = 0, dwFeatures = 0; + byte bMaxSlotIndex = 0, bVoltageSupport = 0; + + boolean hasCcidDescriptor = false; + + ByteBuffer byteBuffer = ByteBuffer.wrap(desc).order(ByteOrder.LITTLE_ENDIAN); + + while (byteBuffer.hasRemaining()) { + byteBuffer.mark(); + byte len = byteBuffer.get(), type = byteBuffer.get(); + + if (type == DESCRIPTOR_TYPE && len == DESCRIPTOR_LENGTH) { + byteBuffer.reset(); + + byteBuffer.position(byteBuffer.position() + SLOT_OFFSET); + bMaxSlotIndex = byteBuffer.get(); + bVoltageSupport = byteBuffer.get(); + dwProtocols = byteBuffer.getInt(); + + byteBuffer.reset(); + + byteBuffer.position(byteBuffer.position() + FEATURES_OFFSET); + dwFeatures = byteBuffer.getInt(); + hasCcidDescriptor = true; + break; + } else { + byteBuffer.position(byteBuffer.position() + len - 2); + } + } + + if (!hasCcidDescriptor) { + throw new UsbTransportException("CCID descriptor not found"); + } + + return new AutoValue_CcidDescription(bMaxSlotIndex, bVoltageSupport, dwProtocols, dwFeatures); + } + + Voltage[] getVoltages() { + ArrayList voltages = new ArrayList<>(); + + if (hasFeature(FEATURE_AUTOMATIC_VOLTAGE)) { + voltages.add(Voltage.AUTO); + } else { + for (Voltage v : Voltage.values()) { + if ((v.mask & getVoltageSupport()) != 0) { + voltages.add(v); + } + } + } + + return voltages.toArray(new Voltage[voltages.size()]); + } + + CcidTransportProtocol getSuitableTransportProtocol() throws UsbTransportException { + boolean hasT1Protocol = (getProtocols() & MASK_T1_PROTO) != 0; + if (!hasT1Protocol) { + throw new UsbTransportException("T=0 protocol is not supported!"); + } + + if (hasFeature(CcidDescription.FEATURE_EXCHANGE_LEVEL_TPDU)) { + return new T1TpduProtocol(); + } else if (hasFeature(CcidDescription.FEATURE_EXCHAGE_LEVEL_SHORT_APDU) || + hasFeature(CcidDescription.FEATURE_EXCHAGE_LEVEL_EXTENDED_APDU)) { + return new T1ShortApduProtocol(); + } else { + throw new UsbTransportException("Character level exchange is not supported"); + } + } + + private boolean hasFeature(int feature) { + return (getFeatures() & feature) != 0; + } + + enum Voltage { + AUTO(0, 0), _5V(1, VOLTAGE_5V), _3V(2, VOLTAGE_3V), _1_8V(3, VOLTAGE_1_8V); + + final byte mask; + final byte powerOnValue; + + Voltage(int powerOnValue, int mask) { + this.powerOnValue = (byte) powerOnValue; + this.mask = (byte) mask; + } + } +} diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java index 471cefbbf..18912da4a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java @@ -34,6 +34,7 @@ import org.bouncycastle.util.Arrays; import org.bouncycastle.util.encoders.Hex; import org.sufficientlysecure.keychain.Constants; + public class CcidTransceiver { private static final int CCID_HEADER_LENGTH = 10; @@ -55,15 +56,18 @@ public class CcidTransceiver { private final UsbDeviceConnection usbConnection; private final UsbEndpoint usbBulkIn; private final UsbEndpoint usbBulkOut; + private final CcidDescription usbCcidDescription; private final byte[] inputBuffer; private byte currentSequenceNumber; - CcidTransceiver(UsbDeviceConnection connection, UsbEndpoint bulkIn, UsbEndpoint bulkOut) { + CcidTransceiver(UsbDeviceConnection connection, UsbEndpoint bulkIn, UsbEndpoint bulkOut, + CcidDescription ccidDescription) { usbConnection = connection; usbBulkIn = bulkIn; usbBulkOut = bulkOut; + usbCcidDescription = ccidDescription; inputBuffer = new byte[usbBulkIn.getMaxPacketSize()]; } @@ -79,25 +83,58 @@ public class CcidTransceiver { skipAvailableInput(); + CcidDataBlock response = null; + for (CcidDescription.Voltage v : usbCcidDescription.getVoltages()) { + Log.v(Constants.TAG, "CCID: attempting to power on with voltage " + v.toString()); + response = iccPowerOnVoltage(v.powerOnValue); + + if (response.getStatus() == 1 && response.getError() == 7) { // Power select error + Log.v(Constants.TAG, "CCID: failed to power on with voltage " + v.toString()); + iccPowerOff(); + Log.v(Constants.TAG, "CCID: powered off"); + } else { + break; + } + } + if (response == null) { + throw new UsbTransportException("Couldn't power up ICC2"); + } + + long elapsedTime = SystemClock.elapsedRealtime() - startTime; + + Log.d(Constants.TAG, "Usb transport connected, took " + elapsedTime + "ms, ATR=" + + Hex.toHexString(response.getData())); + + return response; + } + + private CcidDataBlock iccPowerOnVoltage(byte voltage) throws UsbTransportException { byte sequenceNumber = currentSequenceNumber++; final byte[] iccPowerCommand = { MESSAGE_TYPE_PC_TO_RDR_ICC_POWER_ON, 0x00, 0x00, 0x00, 0x00, SLOT_NUMBER, sequenceNumber, - 0x00, // voltage select = auto + voltage, 0x00, 0x00 // reserved for future use }; sendRaw(iccPowerCommand, 0, iccPowerCommand.length); - CcidDataBlock response = receiveDataBlock(sequenceNumber); - long elapsedTime = SystemClock.elapsedRealtime() - startTime; + return receiveDataBlock(sequenceNumber); + } - Log.d(Constants.TAG, "Usb transport connected T1/TPDU, took " + elapsedTime + "ms, ATR=" + - Hex.toHexString(response.getData())); + private void iccPowerOff() throws UsbTransportException { + byte sequenceNumber = currentSequenceNumber++; + final byte[] iccPowerCommand = { + 0x63, + 0x00, 0x00, 0x00, 0x00, + 0x00, + sequenceNumber, + 0x00 + }; - return response; + sendRaw(iccPowerCommand, 0, iccPowerCommand.length); } /** diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java index 707c6a076..0ff4e9f02 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java @@ -33,13 +33,9 @@ import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.Transport import org.sufficientlysecure.keychain.securitytoken.Transport; import org.sufficientlysecure.keychain.securitytoken.CommandApdu; import org.sufficientlysecure.keychain.securitytoken.ResponseApdu; -import org.sufficientlysecure.keychain.securitytoken.usb.tpdu.T1ShortApduProtocol; -import org.sufficientlysecure.keychain.securitytoken.usb.tpdu.T1TpduProtocol; import org.sufficientlysecure.keychain.util.Log; import java.io.IOException; -import java.nio.ByteBuffer; -import java.nio.ByteOrder; /** * Based on USB CCID Specification rev. 1.1 @@ -47,15 +43,6 @@ import java.nio.ByteOrder; * Implements small subset of these features */ public class UsbTransport implements Transport { - private static final int PROTOCOLS_OFFSET = 6; - private static final int FEATURES_OFFSET = 40; - private static final short MASK_T1_PROTO = 2; - - // dwFeatures Masks - private static final int MASK_TPDU = 0x10000; - private static final int MASK_SHORT_APDU = 0x20000; - private static final int MASK_EXTENDED_APDU = 0x40000; - // https://github.com/Yubico/yubikey-personalization/blob/master/ykcore/ykdef.h private static final int VENDOR_YUBICO = 4176; private static final int PRODUCT_YUBIKEY_NEO_OTP_CCID = 273; @@ -148,57 +135,13 @@ public class UsbTransport implements Transport { throw new UsbTransportException("USB error: failed to claim interface"); } - byte[] rawDescriptors = usbConnection.getRawDescriptors(); - ccidTransportProtocol = getCcidTransportProtocolForRawDescriptors(rawDescriptors); + CcidDescription ccidDescription = CcidDescription.fromRawDescriptors(usbConnection.getRawDescriptors()); + CcidTransceiver transceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, ccidDescription); - CcidTransceiver transceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut); + ccidTransportProtocol = ccidDescription.getSuitableTransportProtocol(); ccidTransportProtocol.connect(transceiver); } - private CcidTransportProtocol getCcidTransportProtocolForRawDescriptors(byte[] desc) throws UsbTransportException { - int dwProtocols = 0, dwFeatures = 0; - boolean hasCcidDescriptor = false; - - ByteBuffer byteBuffer = ByteBuffer.wrap(desc).order(ByteOrder.LITTLE_ENDIAN); - - while (byteBuffer.hasRemaining()) { - byteBuffer.mark(); - byte len = byteBuffer.get(), type = byteBuffer.get(); - - if (type == 0x21 && len == 0x36) { - byteBuffer.reset(); - - byteBuffer.position(byteBuffer.position() + PROTOCOLS_OFFSET); - dwProtocols = byteBuffer.getInt(); - - byteBuffer.reset(); - - byteBuffer.position(byteBuffer.position() + FEATURES_OFFSET); - dwFeatures = byteBuffer.getInt(); - hasCcidDescriptor = true; - break; - } else { - byteBuffer.position(byteBuffer.position() + len - 2); - } - } - - if (!hasCcidDescriptor) { - throw new UsbTransportException("CCID descriptor not found"); - } - - if ((dwProtocols & MASK_T1_PROTO) == 0) { - throw new UsbTransportException("T=0 protocol is not supported"); - } - - if ((dwFeatures & MASK_TPDU) != 0) { - return new T1TpduProtocol(); - } else if (((dwFeatures & MASK_SHORT_APDU) != 0) || ((dwFeatures & MASK_EXTENDED_APDU) != 0)) { - return new T1ShortApduProtocol(); - } else { - throw new UsbTransportException("Character level exchange is not supported"); - } - } - /** * Transmit and receive data * diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java index 13e86c723..8c9d618b9 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java @@ -20,18 +20,15 @@ package org.sufficientlysecure.keychain.securitytoken.usb; import java.io.IOException; public class UsbTransportException extends IOException { - public UsbTransportException() { - } - - public UsbTransportException(final String detailMessage) { + public UsbTransportException(String detailMessage) { super(detailMessage); } - public UsbTransportException(final String message, final Throwable cause) { + public UsbTransportException(String message, final Throwable cause) { super(message, cause); } - public UsbTransportException(final Throwable cause) { + public UsbTransportException(Throwable cause) { super(cause); } } From 19dc0db89b2ecfedc7b1ea1ba494645f558c0477 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 28 Oct 2017 23:32:03 +0200 Subject: [PATCH 02/10] add gnuk to supported tokens --- .../keychain/securitytoken/SecurityTokenInfo.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 08d0d194a..2547830bb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -97,7 +97,8 @@ public abstract class SecurityTokenInfo implements Parcelable { TokenType.YUBIKEY_NEO, TokenType.YUBIKEY_4, TokenType.NITROKEY_PRO, - TokenType.NITROKEY_STORAGE + TokenType.NITROKEY_STORAGE, + TokenType.GNUK )); private static final HashSet SUPPORTED_USB_PUT_KEY = new HashSet<>(Arrays.asList( From 8b07428ec06a3d380c2d594baf57ccd1eee4932a Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 28 Oct 2017 22:24:33 +0200 Subject: [PATCH 03/10] document T1TpduProtocol slighly better --- .../keychain/securitytoken/usb/tpdu/T1TpduProtocol.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/tpdu/T1TpduProtocol.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/tpdu/T1TpduProtocol.java index a2de18245..6b0e9781c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/tpdu/T1TpduProtocol.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/tpdu/T1TpduProtocol.java @@ -28,9 +28,14 @@ import org.sufficientlysecure.keychain.securitytoken.usb.CcidTransportProtocol; import org.sufficientlysecure.keychain.securitytoken.usb.UsbTransportException; import org.sufficientlysecure.keychain.util.Log; +/* T=1 Protocol, see http://www.icedev.se/proxmark3/docs/ISO-7816.pdf, Part 11 */ public class T1TpduProtocol implements CcidTransportProtocol { private final static int MAX_FRAME_LEN = 254; + private static final byte PPS_PPPSS = (byte) 0xFF; + private static final byte PPS_PPS0_T1 = 1; + @SuppressWarnings("PointlessBitwiseExpression") // constructed per spec + private static final byte PPS_PCK = (byte) (PPS_PPPSS ^ PPS_PPS0_T1); private CcidTransceiver ccidTransceiver; private T1TpduBlockFactory blockFactory; @@ -53,7 +58,8 @@ public class T1TpduProtocol implements CcidTransportProtocol { } private void performPpsExchange() throws UsbTransportException { - byte[] pps = { (byte) 0xFF, 1, (byte) (0xFF ^ 1) }; + // Perform PPS, see ISO-7816, Part 9 + byte[] pps = { PPS_PPPSS, PPS_PPS0_T1, PPS_PCK }; CcidDataBlock response = ccidTransceiver.sendXfrBlock(pps); From 0021c1f15f9f40361d65f9d6c6b5cc33dbe99457 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sun, 29 Oct 2017 02:40:24 +0200 Subject: [PATCH 04/10] add tests for CcidTransceiver --- .../securitytoken/usb/CcidDescription.java | 6 + .../securitytoken/usb/CcidTransceiver.java | 27 +- .../securitytoken/usb/UsbTransport.java | 10 +- .../usb/UsbTransportException.java | 16 + .../usb/CcidTransceiverTest.java | 313 ++++++++++++++++++ 5 files changed, 358 insertions(+), 14 deletions(-) create mode 100644 OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiverTest.java diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java index 5a30c830f..bdf3cdc39 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidDescription.java @@ -23,6 +23,7 @@ import java.nio.ByteOrder; import java.util.ArrayList; import android.support.annotation.NonNull; +import android.support.annotation.VisibleForTesting; import com.google.auto.value.AutoValue; import org.sufficientlysecure.keychain.securitytoken.usb.tpdu.T1ShortApduProtocol; @@ -54,6 +55,11 @@ abstract class CcidDescription { public abstract int getProtocols(); public abstract int getFeatures(); + @VisibleForTesting + static CcidDescription fromValues(byte maxSlotIndex, byte voltageSupport, int protocols, int features) { + return new AutoValue_CcidDescription(maxSlotIndex, voltageSupport, protocols, features); + } + @NonNull static CcidDescription fromRawDescriptors(byte[] desc) throws UsbTransportException { int dwProtocols = 0, dwFeatures = 0; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java index 18912da4a..72b2a1fb2 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java @@ -33,6 +33,7 @@ import com.google.auto.value.AutoValue; import org.bouncycastle.util.Arrays; import org.bouncycastle.util.encoders.Hex; import org.sufficientlysecure.keychain.Constants; +import org.sufficientlysecure.keychain.securitytoken.usb.UsbTransportException.UsbCcidErrorException; public class CcidTransceiver { @@ -40,6 +41,7 @@ public class CcidTransceiver { private static final int MESSAGE_TYPE_RDR_TO_PC_DATA_BLOCK = 0x80; private static final int MESSAGE_TYPE_PC_TO_RDR_ICC_POWER_ON = 0x62; + private static final int MESSAGE_TYPE_PC_TO_RDR_ICC_POWER_OFF = 0x63; private static final int MESSAGE_TYPE_PC_TO_RDR_XFR_BLOCK = 0x6f; private static final int COMMAND_STATUS_SUCCESS = 0; @@ -86,15 +88,20 @@ public class CcidTransceiver { CcidDataBlock response = null; for (CcidDescription.Voltage v : usbCcidDescription.getVoltages()) { Log.v(Constants.TAG, "CCID: attempting to power on with voltage " + v.toString()); - response = iccPowerOnVoltage(v.powerOnValue); + try { + response = iccPowerOnVoltage(v.powerOnValue); + } catch (UsbCcidErrorException e) { + if (e.getErrorResponse().getError() == 7) { // Power select error + Log.v(Constants.TAG, "CCID: failed to power on with voltage " + v.toString()); + iccPowerOff(); + Log.v(Constants.TAG, "CCID: powered off"); + continue; + } - if (response.getStatus() == 1 && response.getError() == 7) { // Power select error - Log.v(Constants.TAG, "CCID: failed to power on with voltage " + v.toString()); - iccPowerOff(); - Log.v(Constants.TAG, "CCID: powered off"); - } else { - break; + throw e; } + + break; } if (response == null) { throw new UsbTransportException("Couldn't power up ICC2"); @@ -127,7 +134,7 @@ public class CcidTransceiver { private void iccPowerOff() throws UsbTransportException { byte sequenceNumber = currentSequenceNumber++; final byte[] iccPowerCommand = { - 0x63, + MESSAGE_TYPE_PC_TO_RDR_ICC_POWER_OFF, 0x00, 0x00, 0x00, 0x00, 0x00, sequenceNumber, @@ -193,7 +200,7 @@ public class CcidTransceiver { } while (response.isStatusTimeoutExtensionRequest()); if (!response.isStatusSuccess()) { - throw new UsbTransportException("USB-CCID error: " + response); + throw new UsbCcidErrorException("USB-CCID error!", response); } return response; @@ -213,7 +220,6 @@ public class CcidTransceiver { throw new UsbTransportException("USB-CCID error - bad CCID header type " + inputBuffer[0]); } - CcidDataBlock result = CcidDataBlock.parseHeaderFromBytes(inputBuffer); if (expectedSequenceNumber != result.getSeq()) { @@ -235,6 +241,7 @@ public class CcidTransceiver { } result = result.withData(dataBuffer); + return result; } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java index 0ff4e9f02..7c52994e2 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java @@ -17,6 +17,9 @@ package org.sufficientlysecure.keychain.securitytoken.usb; + +import java.io.IOException; + import android.hardware.usb.UsbConstants; import android.hardware.usb.UsbDevice; import android.hardware.usb.UsbDeviceConnection; @@ -28,15 +31,13 @@ import android.support.annotation.Nullable; import android.util.Pair; import org.sufficientlysecure.keychain.Constants; +import org.sufficientlysecure.keychain.securitytoken.CommandApdu; +import org.sufficientlysecure.keychain.securitytoken.ResponseApdu; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TokenType; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TransportType; import org.sufficientlysecure.keychain.securitytoken.Transport; -import org.sufficientlysecure.keychain.securitytoken.CommandApdu; -import org.sufficientlysecure.keychain.securitytoken.ResponseApdu; import org.sufficientlysecure.keychain.util.Log; -import java.io.IOException; - /** * Based on USB CCID Specification rev. 1.1 * http://www.usb.org/developers/docs/devclass_docs/DWG_Smart-Card_CCID_Rev110.pdf @@ -136,6 +137,7 @@ public class UsbTransport implements Transport { } CcidDescription ccidDescription = CcidDescription.fromRawDescriptors(usbConnection.getRawDescriptors()); + Log.d(Constants.TAG, "CCID Description: " + ccidDescription); CcidTransceiver transceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, ccidDescription); ccidTransportProtocol = ccidDescription.getSuitableTransportProtocol(); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java index 8c9d618b9..43c22afb6 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransportException.java @@ -19,6 +19,9 @@ package org.sufficientlysecure.keychain.securitytoken.usb; import java.io.IOException; +import org.sufficientlysecure.keychain.securitytoken.usb.CcidTransceiver.CcidDataBlock; + + public class UsbTransportException extends IOException { public UsbTransportException(String detailMessage) { super(detailMessage); @@ -31,4 +34,17 @@ public class UsbTransportException extends IOException { public UsbTransportException(Throwable cause) { super(cause); } + + static class UsbCcidErrorException extends UsbTransportException { + private CcidDataBlock errorResponse; + + UsbCcidErrorException(String detailMessage, CcidDataBlock errorResponse) { + super(detailMessage + " " + errorResponse); + this.errorResponse = errorResponse; + } + + CcidDataBlock getErrorResponse() { + return errorResponse; + } + } } diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiverTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiverTest.java new file mode 100644 index 000000000..626c680ae --- /dev/null +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiverTest.java @@ -0,0 +1,313 @@ +package org.sufficientlysecure.keychain.securitytoken.usb; + + +import java.util.LinkedList; + +import android.annotation.TargetApi; +import android.hardware.usb.UsbDeviceConnection; +import android.hardware.usb.UsbEndpoint; +import android.os.Build.VERSION_CODES; + +import org.bouncycastle.util.Arrays; +import org.bouncycastle.util.encoders.Hex; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.sufficientlysecure.keychain.KeychainTestRunner; +import org.sufficientlysecure.keychain.securitytoken.usb.CcidTransceiver.CcidDataBlock; +import org.sufficientlysecure.keychain.securitytoken.usb.UsbTransportException.UsbCcidErrorException; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.AdditionalMatchers.aryEq; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; +import static org.mockito.Matchers.same; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + + +@SuppressWarnings("WeakerAccess") +@RunWith(KeychainTestRunner.class) +@TargetApi(VERSION_CODES.JELLY_BEAN_MR2) +public class CcidTransceiverTest { + static final String ATR = "3bda11ff81b1fe551f0300318473800180009000e4"; + static final int MAX_PACKET_LENGTH_IN = 61; + static final int MAX_PACKET_LENGTH_OUT = 63; + + UsbDeviceConnection usbConnection; + UsbEndpoint usbBulkIn; + UsbEndpoint usbBulkOut; + + LinkedList expectReplies; + LinkedList expectRepliesVerify; + + @Before + public void setUp() throws Exception { + usbConnection = mock(UsbDeviceConnection.class); + usbBulkIn = mock(UsbEndpoint.class); + when(usbBulkIn.getMaxPacketSize()).thenReturn(MAX_PACKET_LENGTH_IN); + usbBulkOut = mock(UsbEndpoint.class); + when(usbBulkOut.getMaxPacketSize()).thenReturn(MAX_PACKET_LENGTH_OUT); + + expectReplies = new LinkedList<>(); + expectRepliesVerify = new LinkedList<>(); + when(usbConnection.bulkTransfer(same(usbBulkIn), any(byte[].class), any(Integer.class), any(Integer.class))) + .thenAnswer( + new Answer() { + @Override + public Integer answer(InvocationOnMock invocation) throws Throwable { + byte[] reply = expectReplies.poll(); + if (reply == null) { + return -1; + } + + byte[] buf = invocation.getArgumentAt(1, byte[].class); + assertEquals(buf.length, MAX_PACKET_LENGTH_IN); + + int len = Math.min(buf.length, reply.length); + System.arraycopy(reply, 0, buf, 0, len); + + if (len < reply.length) { + byte[] rest = Arrays.copyOfRange(reply, len, reply.length); + expectReplies.addFirst(rest); + } + + return len; + } + }); + + } + + @Test + public void testAutoVoltageSelection() throws Exception { + CcidDescription description = CcidDescription.fromValues((byte) 0, (byte) 1, 2, 132218); + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, description); + + byte[] iccPowerOnVoltageAutoCommand = Hex.decode("62000000000000000000"); + byte[] iccPowerOnReply = Hex.decode("80150000000000000000" + ATR); + expectReadPreamble(); + expect(iccPowerOnVoltageAutoCommand, iccPowerOnReply); + + + CcidDataBlock ccidDataBlock = ccidTransceiver.iccPowerOn(); + + + verifyDialog(); + assertArrayEquals(Hex.decode(ATR), ccidDataBlock.getData()); + } + + @Test + public void testManualVoltageSelection() throws Exception { + CcidDescription description = CcidDescription.fromValues((byte) 0, (byte) 1, 2, 132210); + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, description); + + byte[] iccPowerOnVoltage5VCommand = Hex.decode("62000000000000010000"); + byte[] iccPowerOnReply = Hex.decode("80150000000000000000" + ATR); + expectReadPreamble(); + expect(iccPowerOnVoltage5VCommand, iccPowerOnReply); + + + CcidDataBlock ccidDataBlock = ccidTransceiver.iccPowerOn(); + + + verifyDialog(); + assertArrayEquals(Hex.decode(ATR), ccidDataBlock.getData()); + } + + @Test + public void testManualVoltageSelection_failFirst() throws Exception { + CcidDescription description = CcidDescription.fromValues((byte) 0, (byte) 3, 2, 132210); + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, description); + + byte[] iccPowerOnVoltage5VCommand = Hex.decode("62000000000000010000"); + byte[] iccPowerOnFailureReply = Hex.decode("80000000000000010700"); + byte[] iccPowerOffCommand = Hex.decode("6300000000000100"); + byte[] iccPowerOnVoltage3VCommand = Hex.decode("62000000000002020000"); + byte[] iccPowerOnReply = Hex.decode("80150000000002000000" + ATR); + expectReadPreamble(); + expect(iccPowerOnVoltage5VCommand, iccPowerOnFailureReply); + expect(iccPowerOffCommand, null); + expect(iccPowerOnVoltage3VCommand, iccPowerOnReply); + + + CcidDataBlock ccidDataBlock = ccidTransceiver.iccPowerOn(); + + + verifyDialog(); + assertArrayEquals(Hex.decode(ATR), ccidDataBlock.getData()); + } + + @Test + public void testXfer() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = "010203"; + byte[] command = Hex.decode("6F030000000000000000" + commandData); + String responseData = "0304"; + byte[] response = Hex.decode("80020000000000000000" + responseData); + expect(command, response); + + CcidDataBlock ccidDataBlock = ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + + verifyDialog(); + assertArrayEquals(Hex.decode(responseData), ccidDataBlock.getData()); + } + + @Test + public void testXfer_IncrementalSeqNums() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = "010203"; + byte[] commandSeq1 = Hex.decode("6F030000000000000000" + commandData); + byte[] commandSeq2 = Hex.decode("6F030000000001000000" + commandData); + String responseData = "0304"; + byte[] responseSeq1 = Hex.decode("80020000000000000000" + responseData); + byte[] responseSeq2 = Hex.decode("80020000000001000000" + responseData); + expect(commandSeq1, responseSeq1); + expect(commandSeq2, responseSeq2); + + ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + + verifyDialog(); + } + + @Test(expected = UsbTransportException.class) + public void testXfer_badSeqNumberReply() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = "010203"; + byte[] command = Hex.decode("6F030000000000000000" + commandData); + String responseData = "0304"; + byte[] response = Hex.decode("800200000000AA000000" + responseData); + expect(command, response); + + + ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + } + + @Test + public void testXfer_errorReply() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = "010203"; + byte[] command = Hex.decode("6F030000000000000000" + commandData); + byte[] response = Hex.decode("80000000000000012A00"); + expect(command, response); + + try { + ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + } catch (UsbCcidErrorException e) { + assertEquals(0x01, e.getErrorResponse().getIccStatus()); + assertEquals(0x2A, e.getErrorResponse().getError()); + return; + } + + fail(); + } + + @Test + public void testXfer_chainedCommand() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = + "0000000000000123456789000000000000000000000000000000000000000000" + + "0000000000000000000000012345678900000000000000000000000000000000" + + "00000000000001234567890000000000"; + byte[] command = Hex.decode("6F500000000000000000" + commandData); + String responseData = "0304"; + byte[] response = Hex.decode("80020000000000000000" + responseData); + expectChained(command, response); + + CcidDataBlock ccidDataBlock = ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + + verifyDialog(); + assertArrayEquals(Hex.decode(responseData), ccidDataBlock.getData()); + } + + @Test + public void testXfer_chainedReply() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = "010203"; + byte[] command = Hex.decode("6F030000000000000000" + commandData); + String responseData = + "0000000000000000000000000000000000012345678900000000000000000000" + + "0000000000000000000000000001234567890000000000000000000000000000" + + "00000012345678900000000000000000"; + byte[] response = Hex.decode("80500000000000000000" + responseData); + expect(command, response); + + CcidDataBlock ccidDataBlock = ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + + verifyDialog(); + assertArrayEquals(Hex.decode(responseData), ccidDataBlock.getData()); + } + + @Test + public void testXfer_timeoutExtensionReply() throws Exception { + CcidTransceiver ccidTransceiver = new CcidTransceiver(usbConnection, usbBulkIn, usbBulkOut, null); + + String commandData = "010203"; + byte[] command = Hex.decode("6F030000000000000000" + commandData); + byte[] timeExtensionResponse = Hex.decode("80000000000000800000"); + String responseData = "0304"; + byte[] response = Hex.decode("80020000000000000000" + responseData); + expect(command, timeExtensionResponse); + expect(null, response); + + CcidDataBlock ccidDataBlock = ccidTransceiver.sendXfrBlock(Hex.decode(commandData)); + + verifyDialog(); + assertArrayEquals(Hex.decode(responseData), ccidDataBlock.getData()); + } + + private void verifyDialog() { + assertTrue(expectReplies.isEmpty()); + assertFalse(expectRepliesVerify.isEmpty()); + + for (byte[] command : expectRepliesVerify) { + if (command == null) { + continue; + } + verify(usbConnection).bulkTransfer(same(usbBulkIn), aryEq(command), any(Integer.class), any(Integer.class)); + } + + expectRepliesVerify.clear(); + } + + private void expectReadPreamble() { + expectReplies.add(null); + expectRepliesVerify.add(null); + } + + private void expectChained(byte[] command, byte[] reply) { + for (int i = 0; i < command.length; i+= MAX_PACKET_LENGTH_OUT) { + int len = Math.min(MAX_PACKET_LENGTH_OUT, command.length - i); + when(usbConnection.bulkTransfer(same(usbBulkOut), aryEq(command), eq(i), eq(len), + any(Integer.class))).thenReturn(len); + } + if (reply != null) { + expectReplies.add(reply); + expectRepliesVerify.add(null); + } + } + + private void expect(byte[] command, byte[] reply) { + if (command != null) { + when(usbConnection.bulkTransfer(same(usbBulkOut), aryEq(command), eq(0), eq(command.length), + any(Integer.class))).thenReturn(command.length); + } + if (reply != null) { + expectReplies.add(reply); + expectRepliesVerify.add(null); + } + } +} From 83b6c0e2f04b82cde6590bc2e52741efc7fa7c59 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sun, 29 Oct 2017 02:40:39 +0200 Subject: [PATCH 05/10] use correct max packet size in CcidReceiver --- .../keychain/securitytoken/usb/CcidTransceiver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java index 72b2a1fb2..99b1f95ac 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/CcidTransceiver.java @@ -168,7 +168,7 @@ public class CcidTransceiver { int sentBytes = 0; while (sentBytes < data.length) { - int bytesToSend = Math.min(usbBulkIn.getMaxPacketSize(), data.length - sentBytes); + int bytesToSend = Math.min(usbBulkOut.getMaxPacketSize(), data.length - sentBytes); sendRaw(data, sentBytes, bytesToSend); sentBytes += bytesToSend; } From 5f622339b1d48a1f0968405dde37481bda794b8e Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Mon, 30 Oct 2017 17:01:49 +0100 Subject: [PATCH 06/10] Output usb data to debug log --- .../keychain/securitytoken/usb/UsbTransport.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java index 7c52994e2..5b066d630 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java @@ -30,6 +30,7 @@ import android.support.annotation.NonNull; import android.support.annotation.Nullable; import android.util.Pair; +import org.bouncycastle.util.encoders.Hex; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.securitytoken.CommandApdu; import org.sufficientlysecure.keychain.securitytoken.ResponseApdu; @@ -152,7 +153,17 @@ public class UsbTransport implements Transport { */ @Override public ResponseApdu transceive(CommandApdu data) throws UsbTransportException { - return ResponseApdu.fromBytes(ccidTransportProtocol.transceive(data.toBytes())); + byte[] rawCommand = data.toBytes(); + if (Constants.DEBUG) { + Log.d(Constants.TAG, "USB >> " + Hex.toHexString(rawCommand)); + } + + byte[] rawResponse = ccidTransportProtocol.transceive(rawCommand); + if (Constants.DEBUG) { + Log.d(Constants.TAG, "USB << " + Hex.toHexString(rawResponse)); + } + + return ResponseApdu.fromBytes(rawResponse); } @Override From 778fb8e94acd1483c163aefc02517f8676301798 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Mon, 30 Oct 2017 21:45:38 +0100 Subject: [PATCH 07/10] Retain RSA key format when setting key attributes in putKey operation For the put secret key operation, openpgp applet implementations differ in their handling of attributes: - there are four formats for sending key data: standard, standard with modulus, with crt, and with crt and modulus. - the key attributes (modulus length, public exponent length, key format) can not be changed on all cards. changing them is only necessary for cards that support different key lengths (that is, RSA 4096) - on the cards where they *can* be changed, not all parameters might be changeable. in particular, modulus length may be changeable but not key format. Because of this constellation, the put key operation now only sets the modulus of the key, while retaining the key format. At the time of writing, the Gnuk and Nitrokey use the standard format, while the Yubikey and other applets use crt+modulus. This fixes loading keys into the Nitrokey Pro, and partially for the Gnuk token. --- .../keychain/securitytoken/KeyType.java | 14 +++-- .../SecurityTokenConnection.java | 52 +++++++------------ .../securitytoken/SecurityTokenUtils.java | 9 ++-- 3 files changed, 34 insertions(+), 41 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyType.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyType.java index 59c0dc89e..da27a3ccb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyType.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyType.java @@ -20,20 +20,22 @@ package org.sufficientlysecure.keychain.securitytoken; import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey; public enum KeyType { - SIGN(0, 0xB6, 0xCE, 0xC7), - ENCRYPT(1, 0xB8, 0xCF, 0xC8), - AUTH(2, 0xA4, 0xD0, 0xC9),; + SIGN(0, 0xB6, 0xCE, 0xC7, 0xC1), + ENCRYPT(1, 0xB8, 0xCF, 0xC8, 0xC2), + AUTH(2, 0xA4, 0xD0, 0xC9, 0xC3); private final int mIdx; private final int mSlot; private final int mTimestampObjectId; private final int mFingerprintObjectId; + private final int mAlgoAttributeSlot; - KeyType(final int idx, final int slot, final int timestampObjectId, final int fingerprintObjectId) { + KeyType(int idx, int slot, int timestampObjectId, int fingerprintObjectId, int algoAttributeSlot) { this.mIdx = idx; this.mSlot = slot; this.mTimestampObjectId = timestampObjectId; this.mFingerprintObjectId = fingerprintObjectId; + this.mAlgoAttributeSlot = algoAttributeSlot; } public static KeyType from(final CanonicalizedSecretKey key) { @@ -62,4 +64,8 @@ public enum KeyType { public int getFingerprintObjectId() { return mFingerprintObjectId; } + + public int getAlgoAttributeSlot() { + return mAlgoAttributeSlot; + } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java index 70fff4866..644c992ec 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java @@ -193,8 +193,7 @@ public class SecurityTokenConnection { throw new CardException("Initialization failed!", response.getSw()); } - OpenPgpCapabilities openPgpCapabilities = new OpenPgpCapabilities(getData(0x00, 0x6E)); - setConnectionCapabilities(openPgpCapabilities); + refreshConnectionCapabilities(); mPw1ValidatedForSignature = false; mPw1ValidatedForDecrypt = false; @@ -235,6 +234,13 @@ public class SecurityTokenConnection { tokenType = TokenType.UNKNOWN; } + private void refreshConnectionCapabilities() throws IOException { + byte[] rawOpenPgpCapabilities = getData(0x00, 0x6E); + + OpenPgpCapabilities openPgpCapabilities = new OpenPgpCapabilities(rawOpenPgpCapabilities); + setConnectionCapabilities(openPgpCapabilities); + } + @VisibleForTesting void setConnectionCapabilities(OpenPgpCapabilities openPgpCapabilities) throws IOException { this.mOpenPgpCapabilities = openPgpCapabilities; @@ -493,33 +499,13 @@ public class SecurityTokenConnection { } } - private void setKeyAttributes(Passphrase adminPin, final KeyType slot, final CanonicalizedSecretKey secretKey) - throws IOException { - - if (mOpenPgpCapabilities.isAttributesChangable()) { - int tag; - - if (slot == KeyType.SIGN) { - tag = 0xC1; - } else if (slot == KeyType.ENCRYPT) { - tag = 0xC2; - } else if (slot == KeyType.AUTH) { - tag = 0xC3; - } else { - throw new IOException("Unknown key for card."); - } - - try { - - putData(adminPin, tag, SecurityTokenUtils.attributesFromSecretKey(slot, secretKey)); - - mOpenPgpCapabilities.updateWithData(getData(0x00, tag)); - - } catch (PgpGeneralException e) { - throw new IOException("Key algorithm not supported by the security token."); - } - + private void setKeyAttributes(Passphrase adminPin, KeyType keyType, byte[] data) throws IOException { + if (!mOpenPgpCapabilities.isAttributesChangable()) { + return; } + + putData(adminPin, keyType.getAlgoAttributeSlot(), data); + refreshConnectionCapabilities(); } /** @@ -546,9 +532,11 @@ public class SecurityTokenConnection { try { secretKey.unlock(passphrase); - setKeyAttributes(adminPin, slot, secretKey); + setKeyAttributes(adminPin, slot, SecurityTokenUtils.attributesFromSecretKey(slot, secretKey, + mOpenPgpCapabilities.getFormatForKeyType(slot))); - switch (mOpenPgpCapabilities.getFormatForKeyType(slot).keyFormatType()) { + KeyFormat formatForKeyType = mOpenPgpCapabilities.getFormatForKeyType(slot); + switch (formatForKeyType.keyFormatType()) { case RSAKeyFormatType: if (!secretKey.isRSA()) { throw new IOException("Security Token not configured for RSA key."); @@ -561,7 +549,7 @@ public class SecurityTokenConnection { } keyBytes = SecurityTokenUtils.createRSAPrivKeyTemplate(crtSecretKey, slot, - (RSAKeyFormat) (mOpenPgpCapabilities.getFormatForKeyType(slot))); + (RSAKeyFormat) formatForKeyType); break; case ECKeyFormatType: @@ -574,7 +562,7 @@ public class SecurityTokenConnection { ecPublicKey = secretKey.getSecurityTokenECPublicKey(); keyBytes = SecurityTokenUtils.createECPrivKeyTemplate(ecSecretKey, ecPublicKey, slot, - (ECKeyFormat) (mOpenPgpCapabilities.getFormatForKeyType(slot))); + (ECKeyFormat) formatForKeyType); break; default: diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java index 31df10576..35f12a317 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java @@ -22,9 +22,6 @@ import org.bouncycastle.util.Arrays; import org.bouncycastle.util.encoders.Hex; import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; -import org.sufficientlysecure.keychain.securitytoken.ECKeyFormat; -import org.sufficientlysecure.keychain.securitytoken.RSAKeyFormat; -import org.sufficientlysecure.keychain.securitytoken.KeyType; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -33,8 +30,10 @@ import java.security.interfaces.ECPrivateKey; import java.security.interfaces.ECPublicKey; import java.security.interfaces.RSAPrivateCrtKey; + class SecurityTokenUtils { - static byte[] attributesFromSecretKey(final KeyType slot, final CanonicalizedSecretKey secretKey) throws IOException, PgpGeneralException { + static byte[] attributesFromSecretKey(KeyType slot, CanonicalizedSecretKey secretKey, KeyFormat formatForKeyType) + throws IOException, PgpGeneralException { if (secretKey.isRSA()) { final int mModulusLength = secretKey.getBitStrength(); final int mExponentLength = secretKey.getSecurityTokenRSASecretKey().getPublicExponent().bitLength(); @@ -46,7 +45,7 @@ class SecurityTokenUtils { attrs[i++] = (byte) (mModulusLength & 0xff); attrs[i++] = (byte) ((mExponentLength >> 8) & 0xff); attrs[i++] = (byte) (mExponentLength & 0xff); - attrs[i] = RSAKeyFormat.RSAAlgorithmFormat.CRT_WITH_MODULUS.getValue(); + attrs[i] = ((RSAKeyFormat) formatForKeyType).getAlgorithmFormat().getValue(); return attrs; } else if (secretKey.isEC()) { From a51d0555e1a2c4dfaa88bc9155f8873bc8913f06 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Mon, 30 Oct 2017 22:57:13 +0100 Subject: [PATCH 08/10] Retain RSA public exponent length when setting key attributes in putKey operation Similar to the previous commit, openpgp applet implementations differ in regards to the public exponent length. As of this writing: - The SmartPGP applet requires an 11 bit public exponent size - The Gnuk token requires a 32 bit public exponent size For this reason, we simply set the public exponent size to the one previously set in the key attribute info. With this commit, the only variable that can actually change for an RSA key is its modulus size. --- .../securitytoken/SecurityTokenUtils.java | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java index 35f12a317..0a87fa01a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java @@ -22,6 +22,7 @@ import org.bouncycastle.util.Arrays; import org.bouncycastle.util.encoders.Hex; import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; +import org.sufficientlysecure.keychain.securitytoken.RSAKeyFormat.RSAAlgorithmFormat; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -33,24 +34,12 @@ import java.security.interfaces.RSAPrivateCrtKey; class SecurityTokenUtils { static byte[] attributesFromSecretKey(KeyType slot, CanonicalizedSecretKey secretKey, KeyFormat formatForKeyType) - throws IOException, PgpGeneralException { + throws IOException { if (secretKey.isRSA()) { - final int mModulusLength = secretKey.getBitStrength(); - final int mExponentLength = secretKey.getSecurityTokenRSASecretKey().getPublicExponent().bitLength(); - final byte[] attrs = new byte[6]; - int i = 0; - - attrs[i++] = (byte) 0x01; - attrs[i++] = (byte) ((mModulusLength >> 8) & 0xff); - attrs[i++] = (byte) (mModulusLength & 0xff); - attrs[i++] = (byte) ((mExponentLength >> 8) & 0xff); - attrs[i++] = (byte) (mExponentLength & 0xff); - attrs[i] = ((RSAKeyFormat) formatForKeyType).getAlgorithmFormat().getValue(); - - return attrs; + return attributesForRsaKey(secretKey.getBitStrength(), (RSAKeyFormat) formatForKeyType); } else if (secretKey.isEC()) { - final byte[] oid = new ASN1ObjectIdentifier(secretKey.getCurveOid()).getEncoded(); - final byte[] attrs = new byte[1 + (oid.length - 2) + 1]; + byte[] oid = new ASN1ObjectIdentifier(secretKey.getCurveOid()).getEncoded(); + byte[] attrs = new byte[1 + (oid.length - 2) + 1]; if (slot.equals(KeyType.SIGN)) attrs[0] = ECKeyFormat.ECAlgorithmFormat.ECDSA_WITH_PUBKEY.getValue(); @@ -68,6 +57,21 @@ class SecurityTokenUtils { } } + private static byte[] attributesForRsaKey(int modulusLength, RSAKeyFormat formatForKeyType) { + RSAAlgorithmFormat algorithmFormat = formatForKeyType.getAlgorithmFormat(); + int exponentLength = formatForKeyType.getExponentLength(); + + int i = 0; + byte[] attrs = new byte[6]; + attrs[i++] = (byte) 0x01; + attrs[i++] = (byte) ((modulusLength >> 8) & 0xff); + attrs[i++] = (byte) (modulusLength & 0xff); + attrs[i++] = (byte) ((exponentLength >> 8) & 0xff); + attrs[i++] = (byte) (exponentLength & 0xff); + attrs[i] = algorithmFormat.getValue(); + + return attrs; + } static byte[] createRSAPrivKeyTemplate(RSAPrivateCrtKey secretKey, KeyType slot, RSAKeyFormat format) throws IOException { From aef66e97ea77d91cee920433cdec4e25e4c90e40 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 31 Oct 2017 15:39:14 +0100 Subject: [PATCH 09/10] Disable reset for Gnuk token version < 1.2.5 --- .../securitytoken/SecurityTokenInfo.java | 34 +++++++++++++++++-- .../securitytoken/usb/UsbTransport.java | 6 +++- .../ui/token/ManageSecurityTokenContract.java | 1 + .../ui/token/ManageSecurityTokenFragment.java | 9 +++++ .../token/ManageSecurityTokenPresenter.java | 9 +++++ OpenKeychain/src/main/res/values/strings.xml | 4 ++- 6 files changed, 59 insertions(+), 4 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 2547830bb..9fa9f58ba 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -5,6 +5,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import android.os.Parcelable; import android.support.annotation.Nullable; @@ -18,6 +20,7 @@ import org.sufficientlysecure.keychain.ui.util.KeyFormattingUtils; @AutoValue public abstract class SecurityTokenInfo implements Parcelable { private static final byte[] EMPTY_ARRAY = new byte[20]; + private static final Pattern GNUK_VERSION_PATTERN = Pattern.compile("FSIJ-(\\d\\.\\d\\.\\d)-.+"); public abstract TransportType getTransportType(); public abstract TokenType getTokenType(); @@ -90,7 +93,8 @@ public abstract class SecurityTokenInfo implements Parcelable { } public enum TokenType { - YUBIKEY_NEO, YUBIKEY_4, FIDESMO, NITROKEY_PRO, NITROKEY_STORAGE, NITROKEY_START, GNUK, LEDGER_NANO_S, UNKNOWN + YUBIKEY_NEO, YUBIKEY_4, FIDESMO, NITROKEY_PRO, NITROKEY_STORAGE, NITROKEY_START, + GNUK_OLD, GNUK_UNKNOWN, GNUK_NEWER_1_25, LEDGER_NANO_S, UNKNOWN } private static final HashSet SUPPORTED_USB_TOKENS = new HashSet<>(Arrays.asList( @@ -98,7 +102,15 @@ public abstract class SecurityTokenInfo implements Parcelable { TokenType.YUBIKEY_4, TokenType.NITROKEY_PRO, TokenType.NITROKEY_STORAGE, - TokenType.GNUK + TokenType.GNUK_OLD, + TokenType.GNUK_UNKNOWN, + TokenType.GNUK_NEWER_1_25 + )); + + private static final HashSet SUPPORTED_USB_RESET = new HashSet<>(Arrays.asList( + TokenType.YUBIKEY_NEO, + TokenType.YUBIKEY_4, + TokenType.GNUK_NEWER_1_25 )); private static final HashSet SUPPORTED_USB_PUT_KEY = new HashSet<>(Arrays.asList( @@ -120,4 +132,22 @@ public abstract class SecurityTokenInfo implements Parcelable { return isKnownSupported || isNfcTransport; } + public boolean isResetSupported() { + boolean isKnownSupported = SUPPORTED_USB_RESET.contains(getTokenType()); + boolean isNfcTransport = getTransportType() == TransportType.NFC; + + return isKnownSupported || isNfcTransport; + } + + public static String parseGnukVersionString(String serialNo) { + if (serialNo == null) { + return null; + } + + Matcher matcher = GNUK_VERSION_PATTERN.matcher(serialNo); + if (!matcher.matches()) { + return null; + } + return matcher.group(1); + } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java index 5b066d630..ae37841ef 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java @@ -34,6 +34,7 @@ import org.bouncycastle.util.encoders.Hex; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.securitytoken.CommandApdu; import org.sufficientlysecure.keychain.securitytoken.ResponseApdu; +import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TokenType; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TransportType; import org.sufficientlysecure.keychain.securitytoken.Transport; @@ -217,7 +218,10 @@ public class UsbTransport implements Transport { break; } case VENDOR_FSIJ: { - return TokenType.GNUK; + String serialNo = usbConnection.getSerial(); + String gnukVersion = SecurityTokenInfo.parseGnukVersionString(serialNo); + boolean versionBigger125 = gnukVersion != null && "1.2.5".compareTo(gnukVersion) < 0; + return versionBigger125 ? TokenType.GNUK_NEWER_1_25 : TokenType.GNUK_OLD; } case VENDOR_LEDGER: { return TokenType.LEDGER_NANO_S; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenContract.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenContract.java index ae110d8bc..a05bdb00f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenContract.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenContract.java @@ -98,6 +98,7 @@ class ManageSecurityTokenContract { void requestStoragePermission(); + void showErrorCannotReset(boolean isGnuk); void showErrorCannotUnlock(); } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenFragment.java index 198b4ba09..faba27b1d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenFragment.java @@ -352,6 +352,15 @@ public class ManageSecurityTokenFragment extends Fragment implements ManageSecur Notify.create(getActivity(), R.string.token_error_locked_indefinitely, Style.ERROR).show(); } + @Override + public void showErrorCannotReset(boolean isGnuk) { + if (isGnuk) { + Notify.create(getActivity(), R.string.token_error_cannot_reset_gnuk_old, Style.ERROR).show(); + } else { + Notify.create(getActivity(), R.string.token_error_cannot_reset, Style.ERROR).show(); + } + } + @Override public void showDisplayLogActivity(OperationResult result) { Intent intent = new Intent(getActivity(), LogDisplayActivity.class); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenPresenter.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenPresenter.java index a52a0ad87..dc731c979 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenPresenter.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/token/ManageSecurityTokenPresenter.java @@ -30,6 +30,7 @@ import org.sufficientlysecure.keychain.operations.results.GenericOperationResult import org.sufficientlysecure.keychain.operations.results.OperationResult; import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo; +import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TokenType; import org.sufficientlysecure.keychain.ui.token.ManageSecurityTokenContract.ManageSecurityTokenMvpPresenter; import org.sufficientlysecure.keychain.ui.token.ManageSecurityTokenContract.ManageSecurityTokenMvpView; import org.sufficientlysecure.keychain.ui.token.ManageSecurityTokenFragment.StatusLine; @@ -362,6 +363,14 @@ class ManageSecurityTokenPresenter implements ManageSecurityTokenMvpPresenter { @Override public void onClickResetToken() { + if (!tokenInfo.isResetSupported()) { + TokenType tokenType = tokenInfo.getTokenType(); + boolean isGnuk = tokenType == TokenType.GNUK_OLD || tokenType == TokenType.GNUK_UNKNOWN; + + view.showErrorCannotReset(isGnuk); + return; + } + view.showConfirmResetDialog(); } diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index ee4a8079b..8c9f2e346 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -1910,7 +1910,9 @@ "1 attempt left" "%d attempts left" - Too many reset attempts. Token is locked irrecoverably! + Too many reset attempts. Token cannot be unlocked! + "The Gnuk Token does not support reset until version 1.2.5" + "This Security Token does not support reset" Change PIN View Log From a42391f7e9df6c9c9fb1befb3f028097a6f88e04 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 31 Oct 2017 16:06:58 +0100 Subject: [PATCH 10/10] add Nitrokey Pro to whitelist for reset and key import after testing --- .../keychain/securitytoken/SecurityTokenInfo.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 9fa9f58ba..4e5137da2 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -110,12 +110,14 @@ public abstract class SecurityTokenInfo implements Parcelable { private static final HashSet SUPPORTED_USB_RESET = new HashSet<>(Arrays.asList( TokenType.YUBIKEY_NEO, TokenType.YUBIKEY_4, + TokenType.NITROKEY_PRO, TokenType.GNUK_NEWER_1_25 )); private static final HashSet SUPPORTED_USB_PUT_KEY = new HashSet<>(Arrays.asList( TokenType.YUBIKEY_NEO, - TokenType.YUBIKEY_4 // Not clear, will be tested: https://github.com/open-keychain/open-keychain/issues/2069 + TokenType.YUBIKEY_4, // Not clear, will be tested: https://github.com/open-keychain/open-keychain/issues/2069 + TokenType.NITROKEY_PRO )); public boolean isSecurityTokenSupported() {