fix(wallets): prevent path traversal in `wallet_file_path` and update file extension by shamardy · Pull Request #2400 · GLEECBTC/komodo-defi-framework

shamardy · 2025-03-20T17:10:26Z

This PR prevents wallet file path traversal attacks by properly validating wallet names before constructing file paths.

Changes

Added validation in wallet_file_path to only allow alphanumeric characters, dash, and underscore in wallet names
Changed wallet file extension from .dat to .json to better reflect content

smk762

Is it intended to disallow wallet names with spaces in them? Current docs example at https://komodoplatform.com/en/docs/komodo-defi-framework/setup/configure-mm2-json/#example-with-wallet-name-and-wallet-password does not conform.

Tested otherwise and encountered expected errors where special chars where used. Wallet file saved to expected location with json extension, launches as expected with correct wallet_password, and fails to launch if wallet_password deviates from its initial setting.

Incidentally, wallet_password accepted weakpass without issue. Shouldn't we enforce the same password requirements as we do for RPC password?

shamardy · 2025-03-21T12:50:42Z

Is it intended to disallow wallet names with spaces in them?

I will allow spaces again.

shamardy · 2025-03-21T18:32:23Z

@smk762 Allowed spaces in wallet name, leading or trailing spaces are trimmed as well.

shamardy · 2025-03-21T18:35:05Z

Incidentally, wallet_password accepted weakpass without issue. Shouldn't we enforce the same password requirements as we do for RPC password?

I meant for this to let GUI handle it, but just found we have allow_weak_password config param so I will enforce same password requirements as RPC password and if allow_weak_password is passed it will be for both.

smk762 · 2025-03-24T07:11:57Z

@smk762 Allowed spaces in wallet name, leading or trailing spaces are trimmed as well.

Confirming this is functioning as expected, and resultant wallet file trims pre/post spaces.

smk762 · 2025-03-24T07:13:13Z

Incidentally, wallet_password accepted weakpass without issue. Shouldn't we enforce the same password requirements as we do for RPC password?

I meant for this to let GUI handle it, but just found we have allow_weak_password config param so I will enforce same password requirements as RPC password and if allow_weak_password is passed it will be for both.

Should I open a separate issue for this? It's currently the only aspect of this PR blocking approval.

shamardy · 2025-04-11T02:09:02Z

Shouldn't we enforce the same password requirements as we do for RPC password?

This should be fixed now @smk762 . Password policy will be enforced if allow_weak_password is not passed or false, this will happen for 3 cases:

Mnemonic generation
Passing a passphrase while creating a wallet
change_mnemonic_password RPC

smk762 · 2025-04-15T06:08:03Z

Thanks!

On launch, creating a wallet with existing seed:

15 05:39:20, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:218] Error initializing wallet: Password does not meet policy requirements: Password length should be at least 8 characters long
15 05:42:13, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:218] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 lowercase character
15 05:38:20, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:218] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 uppercase character
15 05:38:52, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:218] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 special character
15 05:38:06, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:218] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 digit

On launch, creating wallet with new seed:

15 06:00:04, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:187] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 digit
15 06:00:57, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:187] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 lowercase character
15 06:01:15, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:187] Error initializing wallet: Password does not meet policy requirements: Password should contain at least 1 uppercase character
15 06:01:37, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:187] Error initializing wallet: Password does not meet policy requirements: Password can't contain the word password
15 06:02:39, mm2:299] mm2:372] mm2:169] lp_native_dex:509] lp_wallet:187] Error initializing wallet: Password does not meet policy requirements: Password can't contain the same character 3 times in a row

On change_mnemonic_password RPC:

15 05:47:30, mm2_main::rpc::dispatcher:125] ERROR RPC error response: lp_wallet:576] Password does not meet policy requirements: Password length should be at least 8 characters long
15 05:48:22, mm2_main::rpc::dispatcher:125] ERROR RPC error response: lp_wallet:576] Password does not meet policy requirements: Password should contain at least 1 lowercase character
15 05:48:47, mm2_main::rpc::dispatcher:125] ERROR RPC error response: lp_wallet:576] Password does not meet policy requirements: Password should contain at least 1 uppercase character
15 05:49:19, mm2_main::rpc::dispatcher:125] ERROR RPC error response: lp_wallet:576] Password does not meet policy requirements: Password should contain at least 1 special character
15 05:49:50, mm2_main::rpc::dispatcher:125] ERROR RPC error response: lp_wallet:576] Password does not meet policy requirements: Password should contain at least 1 digit
15 05:50:39, mm2_main::rpc::dispatcher:125] ERROR RPC error response: lp_wallet:576] Password does not meet policy requirements: Password can't contain the word password
able to change to weak password when allow_weak_password:false
not able to change to weak password when allow_weak_password:false
able to launch after changing to a weak password and changing config to allow_weak_password:false (good, no lock outs!)

Other

allow_weak_password still defaults to false, and functions as expected with both boolean values.
get_mnemonic returns expected response after changing to a weak password and changing config to allow_weak_password:false

LGTM!

mariocynicys

Thanks!
looks good. only a couple of nits and questions.

mariocynicys · 2025-04-15T10:57:09Z

mm2src/mm2_main/src/lp_wallet.rs

+            let is_weak_password_accepted = ctx.conf["allow_weak_password"].as_bool() == Some(true);
+            if !is_weak_password_accepted {


looks off, what about ctx.conf["allow_weak_password"].as_bool().unwrap_or(false)

I just used what was used elsewhere for RPC password, but your suggestion works. Will fix it.

mariocynicys · 2025-04-15T11:02:08Z

mm2src/mm2_main/src/lp_wallet.rs

+            if wallet_password.is_empty() {
+                return MmError::err(WalletInitError::PasswordPolicyViolation(
+                    "`wallet_password` cannot be empty".to_string(),
+                ));
+            }


nit: we can allow empty pass and have such check in the password_policy checker (i.e. when weak passes are disabled).

again, following same standards as rpc password ref. https://github.com/KomodoPlatform/komodo-defi-framework/blob/60302fc6da783d65da02485a40d7fa50a79bdab5/mm2src/mm2_main/src/mm2.rs#L143-L145
I will leave the empty check as if it were removed it would allow no password not a weak password.

mariocynicys · 2025-04-15T11:09:34Z

mm2src/mm2_core/src/mm_ctx.rs

+
+        if !wallet_name_trimmed
+            .chars()
+            .all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == ' ')


nit: ['-', '_', ' '].contains(c)

My approach is actually more memory efficient :)

mariocynicys · 2025-04-15T11:13:09Z

mm2src/mm2_core/src/mm_ctx.rs

    #[cfg(not(target_arch = "wasm32"))]
-    pub fn wallet_file_path(&self, wallet_name: &str) -> PathBuf {
-        self.db_root().join(wallet_name.to_string() + ".dat")
+    pub fn wallet_file_path(&self, wallet_name: &str) -> Result<PathBuf, String> {
+        let wallet_name_trimmed = wallet_name.trim();


nit: this method feels too specific to be included in mm_ctx.rs. it's used solely in mnemonic_storage.rs and i think should be define there.

mariocynicys · 2025-04-15T11:20:02Z

mm2src/mm2_core/src/mm_ctx.rs

+
+        if !wallet_name_trimmed
+            .chars()
+            .all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == ' ')


is_alphanumeric() allows non-english unicode chars. do we want that?

Why not :)
A wallet can have a non english name I suppose

shamardy · 2025-04-15T17:56:05Z

@mariocynicys all comments were addressed, please give this another look.

mariocynicys

lgtm! thanks!

* dev: (26 commits) chore(deps): remove base58 and replace it completely with bs58 (GLEECBTC#2427) feat(tron): initial groundwork for full TRON integration (GLEECBTC#2425) fix(UTXO): improve tx fee calculation and min relay fee handling (GLEECBTC#2316) deps(timed-map): bump to 1.3.1 (GLEECBTC#2413) improvement(tendermint): safer IBC channel handler (GLEECBTC#2298) chore(release): complete v2.4.0-beta changelogs (GLEECBTC#2436) fix(event-streaming): initial addresses registration in utxo balance streaming (GLEECBTC#2431) improvement(watchers): re-write use-watchers handling (GLEECBTC#2430) fix(evm): make withdraw_nft work in HD mode (GLEECBTC#2424) feat(taproot): support parsing taproot output address types chore(RPC): use consistent param name for QTUM delegation (GLEECBTC#2419) fix(makerbot): add LiveCoinWatch price provider (GLEECBTC#2416) chore(release): add changelog entries for v2.4.0-beta (GLEECBTC#2415) fix(wallets): prevent path traversal in `wallet_file_path` and update file extension (GLEECBTC#2400) fix(nft): make `update_nft` work with hd wallets using the enabled address (GLEECBTC#2386) fix(wasm): unify error handling for mm2_main (GLEECBTC#2389) fix(tx-history): token information and query (GLEECBTC#2404) test(electrums): fix failing test_one_unavailable_electrum_proto_version (GLEECBTC#2412) improvement(network): remove static IPs from seed lists (GLEECBTC#2407) improvement(best-orders): return an rpc error when we can't find best orders (GLEECBTC#2318) ...

fix: add wallet name validation and change extension to .json

739f8b8

shamardy added the status: in progress label Mar 20, 2025

shamardy added 2 commits March 20, 2025 19:24

fix: read .json files instead of .dat in read_all_wallet_names

79da793

chore: make the file extension str a const

8f6ce46

shamardy added status: pending review and removed status: in progress labels Mar 20, 2025

smk762 reviewed Mar 21, 2025

View reviewed changes

smk762 mentioned this pull request Mar 21, 2025

[FR] update_wallet_password method #2402

Closed

fix: allow spaces in wallet name

88475e5

shamardy added status: in progress and removed status: pending review labels Mar 31, 2025

shamardy added the 2.4.0-beta label Apr 10, 2025

shamardy added 2 commits April 11, 2025 03:58

review fix: standardize password requirements across wallet and RPC

250658a

Merge remote-tracking branch 'origin/dev' into fix-wallet-file-path

0a37b1e

shamardy added status: pending review and removed status: in progress labels Apr 11, 2025

shamardy requested a review from mariocynicys April 14, 2025 23:04

shamardy added the priority: urgent Critical tasks requiring immediate action. label Apr 14, 2025

smk762 self-requested a review April 15, 2025 06:07

smk762 previously approved these changes Apr 15, 2025

View reviewed changes

mariocynicys reviewed Apr 15, 2025

View reviewed changes

review fixes: move wallet_file_path outside ctx impl

47707d5

shamardy dismissed smk762’s stale review via 47707d5 April 15, 2025 17:55

Merge remote-tracking branch 'origin/dev' into fix-wallet-file-path

f320462

mariocynicys previously approved these changes Apr 15, 2025

View reviewed changes

fix test_get_wallet_names in wasm

fffb649

shamardy dismissed mariocynicys’s stale review via fffb649 April 15, 2025 21:53

mariocynicys approved these changes Apr 15, 2025

View reviewed changes

shamardy merged commit 236cdb1 into dev Apr 16, 2025
24 checks passed

shamardy deleted the fix-wallet-file-path branch April 16, 2025 00:09

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

mariocynicys mentioned this pull request May 6, 2025

security: potential path traversal in wallet_file_path #2355

Closed

		let is_weak_password_accepted = ctx.conf["allow_weak_password"].as_bool() == Some(true);
		if !is_weak_password_accepted {

Conversation

shamardy commented Mar 20, 2025

Changes

Uh oh!

smk762 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented Mar 21, 2025

Uh oh!

shamardy commented Mar 21, 2025

Uh oh!

shamardy commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smk762 commented Mar 24, 2025

Uh oh!

smk762 commented Mar 24, 2025

Uh oh!

shamardy commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smk762 commented Apr 15, 2025

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented Apr 15, 2025

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smk762 left a comment •

edited

Loading

shamardy commented Mar 21, 2025 •

edited

Loading

shamardy commented Apr 11, 2025 •

edited

Loading