-
Notifications
You must be signed in to change notification settings - Fork 177
feat: upgrade security system #1450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
0624288
feat: new security system (phase 1)
HashEngineering 5ab1962
feat: new security system (phase 2)
HashEngineering 4cf7737
feat: add fallback testing
HashEngineering f1f0591
fix: add propagate context
HashEngineering a967b50
feat: add add security status and track
HashEngineering 7f30bdb
feat: add add security status and track
HashEngineering 1b20d3d
feat: add add security status and track
HashEngineering 57963fd
feat: add another test failure mode
HashEngineering c9a3d5f
feat: new security system phase 3, recovery
HashEngineering a794696
feat: remove obsolete files
HashEngineering dd2f403
feat: notify user of lost password/PIN
HashEngineering 110be64
feat: other updates
HashEngineering fd21786
feat: update dialogs with new components
HashEngineering 47403ba
feat: restore migration step
HashEngineering 86efb03
feat: prevent crashes
HashEngineering 9c73707
fixes: minor fixes
HashEngineering e38b795
fix: minor fixes
HashEngineering d3b0dd6
docs: add QA test protocol
HashEngineering 8e61189
Merge branch 'master' of https://github.com/dashevo/dash-wallet into …
HashEngineering 61e1f54
fix: remove debug UI elements
HashEngineering a8168a6
fix: remove password dumps in debug mode
HashEngineering 06c8fe5
fix: only set healthy status if encryption is true
HashEngineering File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25 changes: 25 additions & 0 deletions
25
common/src/main/java/org/dash/wallet/common/data/SecuritySystemStatus.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /* | ||
| * Copyright 2025 Dash Core Group. | ||
| * | ||
| * 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 <http://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
||
| package org.dash.wallet.common.data | ||
|
|
||
| enum class SecuritySystemStatus(val value: Int, val isHealthy: Boolean) { | ||
| DEAD(0, false), | ||
| FALLBACKS(2, false), | ||
| HEALTHY(1, true), | ||
| HEALTHY_WITH_FALLBACKS(3, true); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 changes: 42 additions & 0 deletions
42
common/src/main/java/org/dash/wallet/common/util/security/MasterKeyProvider.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * Copyright 2025 Dash Core Group. | ||
| * | ||
| * 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 <http://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
||
| package org.dash.wallet.common.util.security | ||
|
|
||
| import javax.crypto.SecretKey | ||
|
|
||
| /** | ||
| * Interface for providing master encryption keys | ||
| * Implementations can derive keys from wallet seed, recovery codes, or other sources | ||
| */ | ||
| interface MasterKeyProvider { | ||
| /** | ||
| * Get the master encryption key for a specific key alias | ||
| * This key is used for encrypting/decrypting sensitive data | ||
| * | ||
| * @param keyAlias The alias identifying which key to retrieve | ||
| * @return SecretKey for encryption/decryption | ||
| * @throws GeneralSecurityException if key cannot be derived | ||
| */ | ||
| fun getMasterKey(keyAlias: String): SecretKey | ||
|
|
||
| /** | ||
| * Check if the master key provider is available and ready to use | ||
| * For seed-based providers, this checks if wallet is initialized | ||
| */ | ||
| fun isAvailable(): Boolean | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,226 @@ | ||
| # All PIN Check Locations Updated with Fallback Recovery ✅ | ||
|
|
||
| ## Summary | ||
|
|
||
| All locations in the codebase that check user PINs have been updated to include automatic PIN-based fallback recovery when Android KeyStore fails. | ||
|
|
||
| ## Updated Files | ||
|
|
||
| ### 1. ✅ CheckPinLiveData.kt | ||
| **Location**: `wallet/src/de/schildbach/wallet/livedata/CheckPinLiveData.kt` | ||
| **Method**: `checkPin(pin: String)` (lines 45-89) | ||
| **What it does**: Primary PIN checking logic used by most UI components | ||
| **Used by**: | ||
| - CheckPinDialog | ||
| - LockScreenActivity | ||
| - SetPinActivity | ||
| - SetupPinDuringUpgradeDialog | ||
| - Any screen using CheckPinViewModel | ||
|
|
||
| **Changes**: | ||
| - Try primary KeyStore PIN check | ||
| - On failure → Automatic PIN-based fallback recovery | ||
| - On success → Self-healing restores KeyStore | ||
| - Also calls `ensurePinFallback()` when setting up new PIN | ||
|
|
||
| ### 2. ✅ DecryptSeedViewModel.kt | ||
| **Location**: `wallet/src/de/schildbach/wallet/ui/DecryptSeedViewModel.kt` | ||
| **Method**: `decryptSeed(pin: String)` (lines 58-95) | ||
| **What it does**: Verifies PIN before decrypting/displaying recovery phrase | ||
| **Used by**: Screens that show/verify recovery phrase | ||
|
|
||
| **Changes**: | ||
| - Try primary KeyStore PIN check | ||
| - On failure → Automatic PIN-based fallback recovery | ||
| - On success → Self-healing restores KeyStore | ||
| - Added analytics events for tracking fallback usage | ||
|
|
||
| ### 3. ✅ EncryptWalletLiveData.kt | ||
| **Location**: `wallet/src/de/schildbach/wallet/livedata/EncryptWalletLiveData.kt` | ||
| **Method**: `changePassword(oldPin: String, newPin: String)` (lines 65-101) | ||
| **What it does**: Verifies old PIN before allowing PIN change | ||
| **Used by**: PIN change/update screens | ||
|
|
||
| **Changes**: | ||
| - Try primary KeyStore PIN check for old PIN | ||
| - On failure → Automatic PIN-based fallback recovery | ||
| - On success → Self-healing restores KeyStore | ||
| - Calls `ensurePinFallback()` for both old and new PIN | ||
|
|
||
| ## Flow Diagram | ||
|
|
||
| ### Direct PIN Check Locations | ||
| ``` | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ User needs to verify their PIN │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ↓ | ||
| ┌───────────────────┴───────────────────────────────┐ | ||
| │ │ │ | ||
| ▼ ▼ ▼ | ||
| ┌──────────────┐ ┌──────────────────┐ ┌──────────────────┐ | ||
| │CheckPinLive │ │DecryptSeedView │ │EncryptWalletLive │ | ||
| │Data.checkPin │ │Model.decryptSeed │ │Data.changePassword│ | ||
| └──────────────┘ └──────────────────┘ └──────────────────┘ | ||
| │ │ │ | ||
| └───────────────────┴───────────────────────────────┘ | ||
| ↓ | ||
| ┌───────────────────────┐ | ||
| │ Try KeyStore (Primary)│ | ||
| └───────────────────────┘ | ||
| ↓ | ||
| ┌───────────┴────────────┐ | ||
| ▼ ▼ | ||
| Success Failure | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌──────────┐ ┌─────────────────┐ | ||
| │PIN OK ✓ │ │Try PIN Fallback │ | ||
| └──────────┘ └─────────────────┘ | ||
| │ | ||
| ┌───────────┴────────────┐ | ||
| ▼ ▼ | ||
| Success Failure | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌───────────────┐ ┌──────────┐ | ||
| │Self-heal │ │PIN Wrong │ | ||
| │KeyStore │ └──────────┘ | ||
| └───────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌───────────────┐ | ||
| │PIN OK ✓ │ | ||
| └───────────────┘ | ||
| ``` | ||
|
|
||
| ## UI Components (Indirect Usage) | ||
|
|
||
| These UI components call through ViewModels and eventually use the updated methods above: | ||
|
|
||
| ### Uses CheckPinLiveData (Already Protected ✅) | ||
| 1. **CheckPinDialog** → CheckPinViewModel → CheckPinLiveData.checkPin() | ||
| 2. **LockScreenActivity** → CheckPinViewModel → CheckPinLiveData.checkPin() | ||
| 3. **SetPinActivity** → SetPinViewModel → CheckPinLiveData.checkPin() | ||
| 4. **SetupPinDuringUpgradeDialog** → CheckPinViewModel → CheckPinLiveData.checkPin() | ||
|
|
||
| ### Uses DecryptSeedViewModel (Already Protected ✅) | ||
| 5. **Screens showing recovery phrase** → DecryptSeedViewModel.decryptSeed() | ||
|
|
||
| ### Uses EncryptWalletLiveData (Already Protected ✅) | ||
| 6. **PIN change screens** → EncryptWalletLiveData.changePassword() | ||
|
|
||
| ## Verification | ||
|
|
||
| ### ✅ All PIN Check Entry Points Covered | ||
|
|
||
| ```bash | ||
| # Search for all .checkPin( calls | ||
| grep -r "\.checkPin\(" wallet/src --include="*.kt" --include="*.java" | ||
| ``` | ||
|
|
||
| **Results**: | ||
| - CheckPinLiveData.kt - ✅ Updated with fallback | ||
| - DecryptSeedViewModel.kt - ✅ Updated with fallback | ||
| - EncryptWalletLiveData.kt - ✅ Updated with fallback | ||
| - All other calls go through ViewModels → Use the above ✅ | ||
|
|
||
| ## Testing Scenarios | ||
|
|
||
| ### 1. Normal Login (CheckPinDialog/LockScreenActivity) | ||
| - User enters PIN | ||
| - KeyStore works → PIN accepted | ||
| - No fallback needed ✅ | ||
|
|
||
| ### 2. Login with KeyStore Corruption | ||
| - User enters PIN | ||
| - KeyStore fails → Automatic PIN fallback | ||
| - Recovery succeeds → Self-healing restores KeyStore | ||
| - PIN accepted ✅ | ||
|
|
||
| ### 3. View Recovery Phrase | ||
| - User wants to view recovery phrase | ||
| - Enters PIN in DecryptSeedViewModel | ||
| - KeyStore fails → Automatic PIN fallback | ||
| - Recovery succeeds → Phrase displayed ✅ | ||
|
|
||
| ### 4. Change PIN | ||
| - User wants to change PIN | ||
| - Enters old PIN in EncryptWalletLiveData | ||
| - KeyStore fails → Automatic PIN fallback | ||
| - Recovery succeeds → PIN change allowed ✅ | ||
|
|
||
| ## Log Messages | ||
|
|
||
| Watch for these log messages to verify fallback is working: | ||
|
|
||
| ### Success Path | ||
| ``` | ||
| Primary PIN check failed: <exception> | ||
| Attempting PIN-based fallback recovery | ||
| PIN-based fallback recovery succeeded | ||
| Primary encryption healed for wallet_password_key | ||
| ``` | ||
|
|
||
| ### Failure Path | ||
| ``` | ||
| Primary PIN check failed: <exception> | ||
| Attempting PIN-based fallback recovery | ||
| PIN-based fallback recovery also failed | ||
| ``` | ||
|
|
||
| ## Code Pattern Used | ||
|
|
||
| All three locations use the same pattern: | ||
|
|
||
| ```kotlin | ||
| // Try primary PIN check (KeyStore-based) with automatic fallback | ||
| val isPinCorrect = try { | ||
| securityGuard.checkPin(pin) | ||
| } catch (primaryException: Exception) { | ||
| log.warn("Primary PIN check failed: ${primaryException.message}") | ||
|
|
||
| // Primary failed - try PIN-based fallback recovery | ||
| try { | ||
| log.info("Attempting PIN-based fallback recovery") | ||
| val recoveredPassword = securityGuard.recoverPasswordWithPin(pin) | ||
|
|
||
| // PIN-based recovery succeeded! | ||
| log.info("PIN-based fallback recovery succeeded") | ||
|
|
||
| // Ensure PIN fallback is added if it wasn't already | ||
| securityGuard.ensurePinFallback(pin) | ||
|
|
||
| true // PIN is correct | ||
| } catch (fallbackException: Exception) { | ||
| log.error("PIN-based fallback recovery also failed: ${fallbackException.message}") | ||
| false // PIN is incorrect | ||
| } | ||
| } | ||
|
|
||
| if (!isPinCorrect) { | ||
| // Handle incorrect PIN | ||
| throw IllegalArgumentException("wrong pin") | ||
| } | ||
|
|
||
| // Continue with authenticated operation | ||
| ``` | ||
|
|
||
| ## Benefits | ||
|
|
||
| 1. **Transparent Recovery**: User doesn't know KeyStore failed | ||
| 2. **Self-Healing**: Automatically restores KeyStore after recovery | ||
| 3. **No UI Changes**: Works with existing dialogs and screens | ||
| 4. **Analytics Ready**: Can track fallback usage for monitoring | ||
| 5. **Complete Coverage**: All PIN entry points protected | ||
|
|
||
| ## Production Ready ✅ | ||
|
|
||
| All PIN checking code paths now have: | ||
| - ✅ Automatic fallback recovery | ||
| - ✅ Self-healing capability | ||
| - ✅ Error logging | ||
| - ✅ Analytics events (where applicable) | ||
| - ✅ Backward compatibility | ||
|
|
||
| No additional code changes required! |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.