Merge pull request #1338 from josecastillo/development

Improved smart card error handling

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/NfcOperationActivity.java

@@ -10,7 +10,6 @@ import android.content.Intent;
 import android.os.Bundle;
 import android.view.WindowManager;
 
-import org.spongycastle.util.Arrays;
 import org.sufficientlysecure.keychain.Constants;
 import org.sufficientlysecure.keychain.R;
 import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey;
@@ -28,6 +27,7 @@ import org.sufficientlysecure.keychain.util.Preferences;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.util.Arrays;
 
 /**
  * This class provides a communication interface to OpenPGP applications on ISO SmartCard compliant
@@ -47,6 +47,8 @@ public class NfcOperationActivity extends BaseNfcActivity {
     private RequiredInputParcel mRequiredInput;
     private Intent mServiceIntent;
 
+    private static final byte[] BLANK_FINGERPRINT = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
+
     @Override
     protected void onCreate(Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
@@ -126,17 +128,29 @@ public class NfcOperationActivity extends BaseNfcActivity {
                     }
 
                     if (key.canSign() || key.canCertify()) {
-                        nfcPutKey(0xB6, key, passphrase);
+                        if (shouldPutKey(key.getFingerprint(), 0)) {
-                        nfcPutData(0xCE, timestampBytes);
+                            nfcPutKey(0xB6, key, passphrase);
-                        nfcPutData(0xC7, key.getFingerprint());
+                            nfcPutData(0xCE, timestampBytes);
+                            nfcPutData(0xC7, key.getFingerprint());
+                        } else {
+                            throw new IOException("Key slot occupied; card must be reset to put new signature key.");
+                        }
                     } else if (key.canEncrypt()) {
-                        nfcPutKey(0xB8, key, passphrase);
+                        if (shouldPutKey(key.getFingerprint(), 1)) {
-                        nfcPutData(0xCF, timestampBytes);
+                            nfcPutKey(0xB8, key, passphrase);
-                        nfcPutData(0xC8, key.getFingerprint());
+                            nfcPutData(0xCF, timestampBytes);
+                            nfcPutData(0xC8, key.getFingerprint());
+                        } else {
+                            throw new IOException("Key slot occupied; card must be reset to put new decryption key.");
+                        }
                     } else if (key.canAuthenticate()) {
-                        nfcPutKey(0xA4, key, passphrase);
+                        if (shouldPutKey(key.getFingerprint(), 2)) {
-                        nfcPutData(0xD0, timestampBytes);
+                            nfcPutKey(0xA4, key, passphrase);
-                        nfcPutData(0xC9, key.getFingerprint());
+                            nfcPutData(0xD0, timestampBytes);
+                            nfcPutData(0xC9, key.getFingerprint());
+                        } else {
+                            throw new IOException("Key slot occupied; card must be reset to put new authentication key.");
+                        }
                     } else {
                         throw new IOException("Inappropriate key flags for smart card key.");
                     }
@@ -158,6 +172,18 @@ public class NfcOperationActivity extends BaseNfcActivity {
         finish();
     }
 
+    private boolean shouldPutKey(byte[] fingerprint, int idx) throws IOException {
+        byte[] cardFingerprint = nfcGetFingerprint(idx);
+        // Slot is empty, or contains this key already. PUT KEY operation is safe
+        if (Arrays.equals(cardFingerprint, BLANK_FINGERPRINT) ||
+            Arrays.equals(cardFingerprint, fingerprint)) {
+            return true;
+        }
+
+        // Slot already contains a different key; don't overwrite it.
+        return false;
+    }
+
     @Override
     public void handlePinError() {
 

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/base/BaseNfcActivity.java

@@ -95,6 +95,8 @@ public abstract class BaseNfcActivity extends BaseActivity {
         if (NfcAdapter.ACTION_TAG_DISCOVERED.equals(intent.getAction())) {
             try {
                 handleNdefDiscoveredIntent(intent);
+            } catch (CardException e) {
+                handleNfcError(e);
             } catch (IOException e) {
                 handleNfcError(e);
             }
@@ -105,6 +107,81 @@ public abstract class BaseNfcActivity extends BaseActivity {
 
         Log.e(Constants.TAG, "nfc error", e);
         Notify.create(this, getString(R.string.error_nfc, e.getMessage()), Style.WARN).show();
+    }
+
+    public void handleNfcError(CardException e) {
+        Log.e(Constants.TAG, "card error", e);
+
+        short status = e.getResponseCode();
+        // When entering a PIN, a status of 63CX indicates X attempts remaining.
+        if ((status & (short)0xFFF0) == 0x63C0) {
+            Notify.create(this, getString(R.string.error_pin, status & 0x000F), Style.WARN).show();
+            return;
+        }
+
+        // Otherwise, all status codes are fixed values.
+        switch (status) {
+            // These errors should not occur in everyday use; if they are returned, it means we
+            // made a mistake sending data to the card, or the card is misbehaving.
+            case 0x6A80: {
+                Notify.create(this, getString(R.string.error_nfc_bad_data), Style.WARN).show();
+                break;
+            }
+            case 0x6883: {
+                Notify.create(this, getString(R.string.error_nfc_chaining_error), Style.WARN).show();
+                break;
+            }
+            case 0x6B00: {
+                Notify.create(this, getString(R.string.error_nfc_header, "P1/P2"), Style.WARN).show();
+                break;
+            }
+            case 0x6D00: {
+                Notify.create(this, getString(R.string.error_nfc_header, "INS"), Style.WARN).show();
+                break;
+            }
+            case 0x6E00: {
+                Notify.create(this, getString(R.string.error_nfc_header, "CLA"), Style.WARN).show();
+                break;
+            }
+            // These error conditions are more likely to be experienced by an end user.
+            case 0x6285: {
+                Notify.create(this, getString(R.string.error_nfc_terminated), Style.WARN).show();
+                break;
+            }
+            case 0x6700: {
+                Notify.create(this, getString(R.string.error_nfc_wrong_length), Style.WARN).show();
+                break;
+            }
+            case 0x6982: {
+                Notify.create(this, getString(R.string.error_nfc_security_not_satisfied),
+                        Style.WARN).show();
+                break;
+            }
+            case 0x6983: {
+                Notify.create(this, getString(R.string.error_nfc_authentication_blocked),
+                        Style.WARN).show();
+                break;
+            }
+            case 0x6985: {
+                Notify.create(this, getString(R.string.error_nfc_conditions_not_satisfied),
+                        Style.WARN).show();
+                break;
+            }
+            // 6A88 is "Not Found" in the spec, but Yubikey also returns 6A83 for this in some cases.
+            case 0x6A88:
+            case 0x6A83: {
+                Notify.create(this, getString(R.string.error_nfc_data_not_found), Style.WARN).show();
+                break;
+            }
+            // 6F00 is a JavaCard proprietary status code, SW_UNKNOWN, and usually represents an
+            // unhandled exception on the smart card.
+            case 0x6F00: {
+                Notify.create(this, getString(R.string.error_nfc_unknown), Style.WARN).show();
+                break;
+            }
+            default:
+                Notify.create(this, getString(R.string.error_nfc, e.getMessage()), Style.WARN).show();
+        }
 
     }
 
@@ -223,8 +300,9 @@ public abstract class BaseNfcActivity extends BaseActivity {
                         + "06" // Lc (number of bytes)
                         + "D27600012401" // Data (6 bytes)
                         + "00"; // Le
-        if ( ! nfcCommunicate(opening).endsWith(accepted)) { // activate connection
+        String response = nfcCommunicate(opening);  // activate connection
-            throw new IOException("Initialization failed!");
+        if ( ! response.endsWith(accepted) ) {
+            throw new CardException("Initialization failed!", parseCardStatus(response));
         }
 
         byte[] pwStatusBytes = nfcGetPwStatusBytes();
@@ -439,7 +517,7 @@ public abstract class BaseNfcActivity extends BaseActivity {
         }
 
         if ( ! "9000".equals(status)) {
-            throw new IOException("Bad NFC response code: " + status);
+            throw new CardException("Bad NFC response code: " + status, parseCardStatus(response));
         }
 
         // Make sure the signature we received is actually the expected number of bytes long!
@@ -511,9 +589,10 @@ public abstract class BaseNfcActivity extends BaseActivity {
                         + String.format("%02x", mode) // P2
                         + String.format("%02x", pin.length) // Lc
                         + Hex.toHexString(pin);
-            if (!nfcCommunicate(login).equals(accepted)) { // login
+            String response = nfcCommunicate(login); // login
+            if (!response.equals(accepted)) {
                 handlePinError();
-                throw new IOException("Bad PIN!");
+                throw new CardException("Bad PIN!", parseCardStatus(response));
             }
 
             if (mode == 0x81) {
@@ -567,9 +646,10 @@ public abstract class BaseNfcActivity extends BaseActivity {
                 + String.format("%02x", pin.length + newPin.length) // Lc
                 + getHex(pin)
                 + getHex(newPin);
-        if (!nfcCommunicate(changeReferenceDataApdu).equals("9000")) { // Change reference data
+        String response = nfcCommunicate(changeReferenceDataApdu); // change PIN
+        if (!response.equals("9000")) {
             handlePinError();
-            throw new IOException("Failed to change PIN");
+            throw new CardException("Failed to change PIN", parseCardStatus(response));
         }
     }
 
@@ -600,12 +680,9 @@ public abstract class BaseNfcActivity extends BaseActivity {
                 + String.format("%02x", data.length) // Lc
                 + getHex(data);
 
-        String response = nfcCommunicate(putDataApdu);
+        String response = nfcCommunicate(putDataApdu); // put data
         if (!response.equals("9000")) {
-            throw new IOException("Failed to put data for tag "
+            throw new CardException("Failed to put data.", parseCardStatus(response));
-                    + String.format("%02x", (dataObject & 0xFF00) >> 8)
-                    + String.format("%02x", dataObject & 0xFF)
-                    + ": " + response);
         }
     }
 
@@ -713,7 +790,7 @@ public abstract class BaseNfcActivity extends BaseActivity {
             }
 
             if (!response.endsWith("9000")) {
-                throw new IOException("Key export to card failed");
+                throw new CardException("Key export to card failed", parseCardStatus(response));
             }
         }
 
@@ -721,6 +798,24 @@ public abstract class BaseNfcActivity extends BaseActivity {
         Arrays.fill(dataToSend, (byte) 0);
     }
 
+    /**
+     * Parses out the status word from a JavaCard response string.
+     *
+     * @param response A hex string with the response from the card
+     * @return A short indicating the SW1/SW2, or 0 if a status could not be determined.
+     */
+    short parseCardStatus(String response) {
+        if (response.length() < 4) {
+            return 0; // invalid input
+        }
+
+        try {
+            return Short.parseShort(response.substring(response.length() - 4), 16);
+        } catch (NumberFormatException e) {
+            return 0;
+        }
+    }
+
     /**
      * Prints a message to the screen
      *
@@ -790,4 +885,18 @@ public abstract class BaseNfcActivity extends BaseActivity {
         return new String(Hex.encode(raw));
     }
 
+    public class CardException extends IOException {
+        private short mResponseCode;
+
+        public CardException(String detailMessage, short responseCode) {
+            super(detailMessage);
+            mResponseCode = responseCode;
+        }
+
+        public short getResponseCode() {
+            return mResponseCode;
+        }
+
+    }
+
 }

OpenKeychain/src/main/res/values/strings.xml

@@ -1310,6 +1310,17 @@
     <string name="btn_import">"Import"</string>
     <string name="snack_yubi_other">Different key stored on YubiKey!</string>
     <string name="error_nfc">"NFC Error: %s"</string>
+    <string name="error_pin">"NFC: Incorrect PIN; %d tries remaining."</string>
+    <string name="error_nfc_terminated">"NFC: Smart card in termination state"</string>
+    <string name="error_nfc_wrong_length">"NFC: Wrong length for sent / received data"</string>
+    <string name="error_nfc_conditions_not_satisfied">"NFC: Conditions of use not satisfied"</string>
+    <string name="error_nfc_security_not_satisfied">"NFC: Security status not satisfied"</string>
+    <string name="error_nfc_authentication_blocked">"NFC: PIN blocked after too many attempts"</string>
+    <string name="error_nfc_data_not_found">"NFC: Key or object not found"</string>
+    <string name="error_nfc_unknown">"NFC: Unknown Error"</string>
+    <string name="error_nfc_bad_data">"NFC: Card reported invalid data"</string>
+    <string name="error_nfc_chaining_error">"NFC: Card expected last command in a chain"</string>
+    <string name="error_nfc_header">"NFC: Card reported invalid %s byte"</string>
     <string name="error_pin_nodefault">Default PIN was rejected!</string>
     <string name="error_bluetooth_file">Error creating temporary file. Bluetooth sharing will fail.</string>
     <string name="snack_encrypt_filenames_on">"Filenames <b>are</b> encrypted."</string>