fix: critical security issues from code review

- Thread safety: Add @Synchronized methods for ECDH buffer access
- Crypto: Use X509EncodedKeySpec instead of hard-coded EC curve params
- Sensitive data: Make aes_key/shared_secret private in CryptoECDH
- Credential storage: Migrate PSK/UUID to EncryptedSharedPreferences

android/app/src/main/java/nl/eddydevink/sylda/BleRelayManager.kt

@@ -22,8 +22,10 @@ import androidx.core.app.ActivityCompat
 import java.nio.ByteBuffer
 import java.nio.ByteOrder
 import java.security.KeyPairGenerator
+import java.security.KeyFactory
 import java.security.SecureRandom
 import java.security.spec.ECGenParameterSpec
+import java.security.spec.X509EncodedKeySpec
 import java.security.interfaces.ECPublicKey
 import java.util.UUID
 import javax.crypto.Cipher
@@ -61,6 +63,25 @@ class BleRelayManager(
         private const val ECDH_PKT_SIZE = 97
         private const val BLE_MTU = 512
         private const val ECDH_TIMEOUT_MS = 5000L
+
+        private val SECP256R1_SPKI_HEADER = byteArrayOf(
+            0x30.toByte(), 0x59.toByte(), 0x30.toByte(), 0x13.toByte(),
+            0x06.toByte(), 0x07.toByte(), 0x2A.toByte(), 0x86.toByte(),
+            0x48.toByte(), 0xCE.toByte(), 0x3D.toByte(), 0x02.toByte(),
+            0x01.toByte(), 0x06.toByte(), 0x08.toByte(), 0x2A.toByte(),
+            0x86.toByte(), 0x48.toByte(), 0xCE.toByte(), 0x3D.toByte(),
+            0x03.toByte(), 0x01.toByte(), 0x07.toByte(), 0x03.toByte(),
+            0x42.toByte(), 0x00.toByte()
+        )
+
+        private fun decodeEcPublicKey(uncompressedKey: ByteArray): ECPublicKey {
+            require(uncompressedKey.size == ECDH_PUBKEY_LEN && uncompressedKey[0] == 0x04.toByte()) {
+                "Invalid uncompressed EC key format"
+            }
+            val spkiEncoded = SECP256R1_SPKI_HEADER + uncompressedKey
+            return KeyFactory.getInstance("EC")
+                .generatePublic(X509EncodedKeySpec(spkiEncoded)) as ECPublicKey
+        }
     }
 
     private var bluetoothGatt: BluetoothGatt? = null
@@ -84,6 +105,16 @@ class BleRelayManager(
     private var ecdhNotifyBuf: ByteArray = ByteArray(0)
     private var ecdhCccdPending = false
 
+    @Synchronized
+    private fun setEcdhNotifyBuf(value: ByteArray) {
+        ecdhNotifyBuf = value.copyOf()
+    }
+
+    @Synchronized
+    private fun getEcdhNotifyBuf(): ByteArray {
+        return ecdhNotifyBuf.copyOf()
+    }
+
     interface OtaNotifyCallback {
         fun onOtaNotify(status: Int, seq: Int)
     }
@@ -404,7 +435,7 @@ class BleRelayManager(
                 Log.d(TAG, "OTA notify: status=$statusName seq=$seqVal raw=${value.joinToString("") { "%02X".format(it) }}")
                 handler.post { otaNotifyCallback?.onOtaNotify(statusVal, seqVal) }
             } else if (characteristic.uuid == ECDH_UUID) {
-                ecdhNotifyBuf = value.copyOf()
+                setEcdhNotifyBuf(value)
                 ecdhDeferred?.complete(true)
                 Log.d(TAG, "ECDH notify received (${value.size} bytes)")
             }
@@ -506,7 +537,7 @@ class BleRelayManager(
                     return@Thread
                 }
 
-                val resp = ecdhNotifyBuf
+                val resp = getEcdhNotifyBuf()
                 if (resp.size != ECDH_PKT_SIZE) {
                     Log.w(TAG, "ECDH: bad response size ${resp.size}")
                     handler.post {
@@ -530,28 +561,7 @@ class BleRelayManager(
 
                 val ka = KeyAgreement.getInstance("ECDH")
                 ka.init(keyPair.private)
-                val spec = java.security.spec.ECPublicKeySpec(
+                val serverKey = decodeEcPublicKey(serverPub)
-                    java.security.spec.ECPoint(
-                        java.math.BigInteger(1, serverPub.copyOfRange(1, 33)),
-                        java.math.BigInteger(1, serverPub.copyOfRange(33, 65))
-                    ),
-                    java.security.spec.ECParameterSpec(
-                        java.security.spec.EllipticCurve(
-                            java.security.spec.ECFieldFp(
-                                java.math.BigInteger("FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF", 16)
-                            ),
-                            java.math.BigInteger("FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFC", 16),
-                            java.math.BigInteger("5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B", 16)
-                        ),
-                        java.security.spec.ECPoint(
-                            java.math.BigInteger("6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296", 16),
-                            java.math.BigInteger("4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5", 16)
-                        ),
-                        java.math.BigInteger("FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551", 16),
-                        1
-                    )
-                )
-                val serverKey = java.security.KeyFactory.getInstance("EC").generatePublic(spec)
                 ka.doPhase(serverKey, true)
                 val shared = ka.generateSecret()
 
@@ -606,13 +616,13 @@ class BleRelayManager(
     }
 
     private fun loadState() {
-        val prefs = PrefsHelper.getPrefs(context)
+        PrefsHelper.migrateCredentialsIfNeeded(context)
-        val hexKey = prefs.getString(PrefsHelper.PREF_KEY, null)
+        val hexKey = PrefsHelper.getPsk(context)
         psk = hexKey?.let { hexStringToBytes(it) }
         val hexUuid = PrefsHelper.getServerUuid(context)
-            ?: prefs.getString(PrefsHelper.PREF_UUID, null)
+            ?: PrefsHelper.getDeviceCredentialUuid(context)
         deviceUuid = hexUuid?.let { hexStringToBytes(it) }
-        seq = prefs.getInt(PrefsHelper.PREF_SEQ, 1).coerceAtMost(PrefsHelper.SEQ_MAX)
+        seq = PrefsHelper.getPrefs(context).getInt(PrefsHelper.PREF_SEQ, 1).coerceAtMost(PrefsHelper.SEQ_MAX)
         Log.d(TAG, "PSK loaded: ${psk?.size ?: "none"} bytes, UUID: ${deviceUuid?.size ?: "none"} bytes, seq=$seq")
     }
 

android/app/src/main/java/nl/eddydevink/sylda/PrefsHelper.kt

@@ -3,12 +3,16 @@ package nl.eddydevink.sylda
 
 import android.content.Context
 import android.content.SharedPreferences
+import android.util.Log
 import androidx.appcompat.app.AppCompatDelegate
+import androidx.security.crypto.EncryptedSharedPreferences
+import androidx.security.crypto.MasterKey
 
 object PrefsHelper {
 
     private const val TAG = "PrefsHelper"
     const val PREFS_NAME = "relay_prefs"
+    private const val ENCRYPTED_PREFS_NAME = "encrypted_prefs"
     const val PREF_KEY = "psk"
     const val PREF_UUID = "device_uuid"
     const val PREF_SEQ = "seq_counter"
@@ -20,6 +24,49 @@ object PrefsHelper {
     const val KEY_CLIENT_UUID = "clientUuid"
     const val SEQ_MAX = 1_000_000
 
+    private var masterKey: MasterKey? = null
+
+    private fun getMasterKey(context: Context): MasterKey {
+        return masterKey ?: run {
+            MasterKey.Builder(context)
+                .setKeyScheme(MasterKey.KeyScheme.AES256_GCM)
+                .build().also { masterKey = it }
+        }
+    }
+
+    fun getEncryptedPrefs(context: Context): SharedPreferences {
+        return EncryptedSharedPreferences.create(
+            context,
+            ENCRYPTED_PREFS_NAME,
+            getMasterKey(context),
+            EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
+            EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
+        )
+    }
+
+    fun migrateCredentialsIfNeeded(context: Context) {
+        val plainPrefs = getPrefs(context)
+        val encPrefs = getEncryptedPrefs(context)
+
+        val legacyKey = plainPrefs.getString(PREF_KEY, null)
+        val legacyUuid = plainPrefs.getString(PREF_UUID, null)
+
+        if (legacyKey != null && !encPrefs.contains(PREF_KEY)) {
+            Log.i(TAG, "Migrating credentials to encrypted storage")
+            encPrefs.edit().apply {
+                putString(PREF_KEY, legacyKey)
+                if (legacyUuid != null) putString(PREF_UUID, legacyUuid)
+                apply()
+            }
+            plainPrefs.edit().apply {
+                remove(PREF_KEY)
+                remove(PREF_UUID)
+                apply()
+            }
+            Log.i(TAG, "Credentials migration complete")
+        }
+    }
+
     fun getDeviceMode(context: Context): String = getPrefs(context).getString(KEY_DEVICE_MODE, "server") ?: "server"
     fun setDeviceMode(context: Context, mode: String) = getPrefs(context).edit().putString(KEY_DEVICE_MODE, mode).apply()
     fun getTransportMode(context: Context): String = getPrefs(context).getString("transportMode", "ble") ?: "ble"
@@ -29,6 +76,11 @@ object PrefsHelper {
     fun getClientUuid(context: Context): String? = getPrefs(context).getString(KEY_CLIENT_UUID, null)
     fun setClientUuid(context: Context, uuid: String) = getPrefs(context).edit().putString(KEY_CLIENT_UUID, uuid).apply()
 
+    fun getPsk(context: Context): String? = getEncryptedPrefs(context).getString(PREF_KEY, null)
+    fun setPsk(context: Context, psk: String) = getEncryptedPrefs(context).edit().putString(PREF_KEY, psk).apply()
+    fun getDeviceCredentialUuid(context: Context): String? = getEncryptedPrefs(context).getString(PREF_UUID, null)
+    fun setDeviceCredentialUuid(context: Context, uuid: String) = getEncryptedPrefs(context).edit().putString(PREF_UUID, uuid).apply()
+
     fun applyTheme(context: Context) {
         val prefs = getPrefs(context)
         val mode = when (prefs.getString(PREF_THEME, "system")) {

android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt

@@ -372,9 +372,8 @@ class SettingsActivity : AppCompatActivity() {
     private fun doKeyExchange(usbManager: UsbManager, device: UsbDevice) {
         showProgressDialog(getString(R.string.title_key_exchange), getString(R.string.msg_reading_key), STYLE_SPINNER)
 
-        val prefs = PrefsHelper.getPrefs(this)
+        val appUuid = PrefsHelper.getDeviceCredentialUuid(this)
-        val appUuid = prefs.getString(PrefsHelper.PREF_UUID, null)
+        val appKey = PrefsHelper.getPsk(this)
-        val appKey = prefs.getString(PrefsHelper.PREF_KEY, null)
 
         lifecycleScope.launch(Dispatchers.IO) {
             val result = readKeyFromEsp(usbManager, device, appUuid, appKey)
@@ -384,10 +383,8 @@ class SettingsActivity : AppCompatActivity() {
                 progressDialog = null
                 when (result) {
                     is KeyResult.Success -> {
-                        prefs.edit()
+                        PrefsHelper.setDeviceCredentialUuid(this@SettingsActivity, result.deviceUuid)
-                            .putString(PrefsHelper.PREF_UUID, result.deviceUuid)
+                        PrefsHelper.setPsk(this@SettingsActivity, result.hexKey)
-                            .putString(PrefsHelper.PREF_KEY, result.hexKey)
-                            .apply()
                         if (PrefsHelper.getDeviceMode(this@SettingsActivity) == "server") {
                             PrefsHelper.setServerUuid(this@SettingsActivity, result.deviceUuid)
                         } else {
@@ -838,8 +835,7 @@ class SettingsActivity : AppCompatActivity() {
 
     private fun showConflictDialog(usbManager: UsbManager, device: UsbDevice,
                                    espUuid: String, espKey: String, appKey: String) {
-        val prefs = PrefsHelper.getPrefs(this)
+        val appUuid = PrefsHelper.getDeviceCredentialUuid(this)
-        val appUuid = prefs.getString(PrefsHelper.PREF_UUID, null)
         val newAppKey = appKey.ifEmpty { generateRandomHexKey() }
         val newAppUuid = appUuid ?: generateRandomHex(UUID_BYTE_COUNT)
 
@@ -946,11 +942,8 @@ class SettingsActivity : AppCompatActivity() {
                 }
             }
 
-            val prefs = PrefsHelper.getPrefs(this)
+            PrefsHelper.setDeviceCredentialUuid(this, newUuid)
-            prefs.edit()
+            PrefsHelper.setPsk(this, appKey)
-                .putString(PrefsHelper.PREF_UUID, newUuid)
-                .putString(PrefsHelper.PREF_KEY, appKey)
-                .apply()
             if (PrefsHelper.getDeviceMode(this) == "server") {
                 PrefsHelper.setServerUuid(this, newUuid)
             } else {

src/client.cpp

@@ -260,7 +260,7 @@ static bool performEcdhHandshake(NimBLERemoteService *pSvc) {
         return false;
     }
 
-    memcpy(aesKey, ecdh.aes_key, AES_KEY_LEN);
+    memcpy(aesKey, ecdh.getAesKey(), AES_KEY_LEN);
     ecdh.free();
     ecdhReady = true;
     ESP_LOGI(TAG, "ECDH: handshake complete");

src/ecdh_helper.h

@@ -21,6 +21,9 @@ private:
     mbedtls_ctr_drbg_context ctr_drbg;
     bool initialized = false;
 
+    uint8_t shared_secret[32];
+    uint8_t aes_key[AES_KEY_LEN];
+
     CryptoECDH(const CryptoECDH&) = delete;
     CryptoECDH& operator=(const CryptoECDH&) = delete;
 
@@ -29,8 +32,9 @@ public:
 
     uint8_t public_key[ECDH_PUBKEY_LEN];
     size_t pub_len = 0;
-    uint8_t shared_secret[32];
+
-    uint8_t aes_key[AES_KEY_LEN];
+    const uint8_t* getAesKey() const { return aes_key; }
+    size_t getAesKeyLen() const { return AES_KEY_LEN; }
 
     bool init() {
         mbedtls_ecdh_init(&ecdh);

src/main.cpp

@@ -406,7 +406,7 @@ class EcdhCharacteristicCallbacks : public NimBLECharacteristicCallbacks {
             return;
         }
 
-        memcpy(clients[idx].aesKey, clients[idx].ecdh.aes_key, AES_KEY_LEN);
+        memcpy(clients[idx].aesKey, clients[idx].ecdh.getAesKey(), AES_KEY_LEN);
 
         uint8_t resp[ECDH_PKT_SIZE];
         memcpy(resp, clients[idx].ecdh.public_key, ECDH_PUBKEY_LEN);