Warn user when non-common derivation path is used

[pythcoiner]

A "blackmail" attack can be attempted by an attacker by making the user sign a transaction to a change with a random looking derivation path like m/12345678'/567891'/2147483647'.

It seems from looking at the code that the user is not warn in such case.

[odudex]

Yes, definitely an important verification and warning to be added.

[that-avg-dev]

hii , would like to work on this issue , new to the project and diving in to understand better.

[odudex]

hii , would like to work on this issue , new to the project and diving in to understand better.

Feel free to contribute with feedback and code.

[pythcoiner]

Looking more at the code, we have a skip in the signing logic that has some hardcoded derivation paths at

Kern/main/core/psbt.c

Lines 310 to 334 in 7fa7577

	// BIP84 single-sig: m/84'/coin'/account'/change/index (24 bytes keypath)
	// BIP48 multisig: m/48'/coin'/account'/script_type'/change/index (28
	// bytes keypath)
	if (purpose_value == 84 && keypath_len >= 24 &&
	account == expected_account) {
	uint32_t change_val, index_val;
	memcpy(&change_val, keypath + 16, sizeof(uint32_t));
	memcpy(&index_val, keypath + 20, sizeof(uint32_t));
	snprintf(path_str, sizeof(path_str), "m/84'/%u'/%u'/%u/%u", coin_value,
	wallet_get_account(), change_val, index_val);
	} else if (purpose_value == 48 && keypath_len >= 28 &&
	account == expected_account) {
	uint32_t script_type, change_val, index_val;
	memcpy(&script_type, keypath + 16, sizeof(uint32_t));
	memcpy(&change_val, keypath + 20, sizeof(uint32_t));
	memcpy(&index_val, keypath + 24, sizeof(uint32_t));
	uint32_t script_type_value = script_type & 0x7FFFFFFF;
	snprintf(path_str, sizeof(path_str), "m/48'/%u'/%u'/%u'/%u/%u",
	coin_value, wallet_get_account(), script_type_value,
	change_val, index_val);
	} else {
	continue;
	}

I think we should:

• extract this check in a is_common_derivation_path() fn
• add and arg in psbt_sign() to skip this tests
• warn the user before signing if is_common_derivation_path() fails on the psbt and ask him if he want sign anyway

[odudex]

I think I got it.
Currently, if an output has an assigned derivation path, but it is not the same as the loaded wallet's, Kern will show it as a spend (not change or self-transfer). What you mean is that this is not enough to catch user's attention, and a user might not notice this when signing, and you are recommending prompting the user with a warning about this output derivation that differs from the derivation of the loaded wallet?

[pythcoiner]

this check should even be at descriptor load time maybe, and this change could have a quite wide scope, I need to take more time to look at the complete flow

[odudex]

Maybe scan.c > classify_output could be more picky and bring to the user's attention "anomalies" like outputs with derivation paths that doesn't belong to the loaded wallet. (Currently it just classifies as spends)