Skip to content

Commit

Permalink
conway: review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@janmazak
janmazak committed Dec 16, 2023
1 parent 25f9281 commit c6352df
Show file tree
Hide file tree
Showing 17 changed files with 197 additions and 209 deletions.
27 changes: 23 additions & 4 deletions src/bip44.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,20 @@ static bool bip44_hasReasonableAddress(const bip44_path_t* pathSpec)
return (address <= MAX_REASONABLE_ADDRESS);
}

static bool bip44_isConwayPathRecommended(const bip44_path_t* pathSpec)
{
switch (bip44_classifyPath(pathSpec)) {
case PATH_DREP_KEY:
case PATH_COMMITTEE_COLD_KEY:
case PATH_COMMITTEE_HOT_KEY:
// strongly recommended in CIP-0105 to only use 0 as address
return (bip44_getAddressValue(pathSpec) == 0);
default:
ASSERT(false);
return false;
}
}

static bool bip44_containsMoreThanAddress(const bip44_path_t* pathSpec)
{
return (pathSpec->length > BIP44_I_ADDRESS + 1);
Expand Down Expand Up @@ -263,7 +277,8 @@ bool bip44_isDRepKeyPath(const bip44_path_t* pathSpec)
CHECK(bip44_hasShelleyPrefix(pathSpec));
CHECK(isHardened(bip44_getAccount(pathSpec)));
CHECK(bip44_getChainTypeValue(pathSpec) == CARDANO_CHAIN_DREP_KEY);
CHECK(bip44_getAddressValue(pathSpec) == 0); // TODO allow other values and check for hardened only?
// is it strongly recommended (but not forbidden) to only use 0 as address
CHECK(!isHardened(bip44_getAddressValue(pathSpec)));
return true;
#undef CHECK
}
Expand All @@ -276,7 +291,8 @@ bool bip44_isCommitteeColdKeyPath(const bip44_path_t* pathSpec)
CHECK(bip44_hasShelleyPrefix(pathSpec));
CHECK(isHardened(bip44_getAccount(pathSpec)));
CHECK(bip44_getChainTypeValue(pathSpec) == CARDANO_CHAIN_COMMITTEE_COLD_KEY);
CHECK(bip44_getAddressValue(pathSpec) == 0); // TODO allow other values and check for hardened only?
// is it strongly recommended (but not forbidden) to only use 0 as address
CHECK(!isHardened(bip44_getAddressValue(pathSpec)));
return true;
#undef CHECK
}
Expand All @@ -289,7 +305,8 @@ bool bip44_isCommitteeHotKeyPath(const bip44_path_t* pathSpec)
CHECK(bip44_hasShelleyPrefix(pathSpec));
CHECK(isHardened(bip44_getAccount(pathSpec)));
CHECK(bip44_getChainTypeValue(pathSpec) == CARDANO_CHAIN_COMMITTEE_HOT_KEY);
CHECK(bip44_getAddressValue(pathSpec) == 0); // TODO allow other values and check for hardened only?
// is it strongly recommended (but not forbidden) to only use 0 as address
CHECK(!isHardened(bip44_getAddressValue(pathSpec)));
return true;
#undef CHECK
}
Expand Down Expand Up @@ -552,7 +569,9 @@ bool bip44_isPathReasonable(const bip44_path_t* pathSpec)
case PATH_DREP_KEY:
case PATH_COMMITTEE_COLD_KEY:
case PATH_COMMITTEE_HOT_KEY:
return bip44_hasReasonableAccount(pathSpec) && bip44_hasReasonableAddress(pathSpec);
return bip44_hasReasonableAccount(pathSpec)
&& bip44_hasReasonableAddress(pathSpec)
&& bip44_isConwayPathRecommended(pathSpec);

case PATH_MINT_KEY:
return bip44_hasReasonableMintPolicy(pathSpec);
Expand Down
4 changes: 2 additions & 2 deletions src/bip44.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,10 @@ typedef enum {
// m / 1852' / 1815' / account' / 3 / address_index
PATH_DREP_KEY,

// constitutional committee hot key TODO not approved yet https://github.com/Ryun1/CIPs/blob/conway-keys/CIP-conway-keys/README.md
// constitutional committee hot key
// m / 1852' / 1815' / account' / 4 / address_index
PATH_COMMITTEE_COLD_KEY,
// constitutional committee cold key TODO not approved yet
// constitutional committee cold key
// m / 1852' / 1815' / account' / 5 / address_index
PATH_COMMITTEE_HOT_KEY,

Expand Down
4 changes: 0 additions & 4 deletions src/getPublicKeys.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ static void getPublicKeys_handleInitAPDU(const uint8_t* wireDataBuffer, size_t w
{
{
CHECK_STAGE(GET_KEYS_STAGE_INIT);

ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA);
}
{
// parse data
Expand Down Expand Up @@ -186,8 +184,6 @@ void getPublicKeys_handleGetNextKeyAPDU(
{
CHECK_STAGE(GET_KEYS_STAGE_GET_KEYS);

ASSERT(wireDataSize < BUFFER_SIZE_PARANOIA);

VALIDATE(ctx->currentPath < ctx->numPaths, ERR_INVALID_STATE);

read_view_t view = make_read_view(wireDataBuffer, wireDataBuffer + wireDataSize);
Expand Down
92 changes: 62 additions & 30 deletions src/securityPolicy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,8 @@ security_policy_t policyForSignTxCollateralOutputAdaAmount(
bool isTotalCollateralPresent
)
{
// WARNING: policies for collateral inputs, collateral return output and total collateral are interdependent

if (outputPolicy == POLICY_ALLOW_WITHOUT_PROMPT) {
// output not shown, so none of its elements should be shown
ALLOW();
Expand All @@ -908,6 +910,8 @@ security_policy_t policyForSignTxCollateralOutputTokens(
const tx_output_description_t* output
)
{
// WARNING: policies for collateral inputs, collateral return output and total collateral are interdependent

if (outputPolicy == POLICY_ALLOW_WITHOUT_PROMPT) {
// output not shown, so none of its elements should be shown
ALLOW();
Expand Down Expand Up @@ -1028,7 +1032,7 @@ security_policy_t policyForSignTxCertificate(
}

// applicable to credentials that are witnessed in this tx
static bool forbiddenCredential(
static bool _forbiddenCredential(
sign_tx_signingmode_t txSigningMode,
const ext_credential_t* credential
)
Expand All @@ -1042,8 +1046,10 @@ static bool forbiddenCredential(
case EXT_CREDENTIAL_KEY_HASH:
// everything is expected to be governed by native scripts
return true;
default:
case EXT_CREDENTIAL_SCRIPT_HASH:
break;
default:
ASSERT(false);
}
break;

Expand All @@ -1060,15 +1066,16 @@ static bool forbiddenCredential(
// if the hash corresponds to some of his keys,
// and might inadvertently sign several certificates with a single witness
return true;
default:
case EXT_CREDENTIAL_KEY_PATH:
break;
default:
ASSERT(false);
}
break;

default:
// this should not be called in POOL_REGISTRATION signing modes
ASSERT(false);
break;
}

return false;
Expand All @@ -1079,17 +1086,20 @@ security_policy_t _policyForSignTxCertificateStakeCredential(
const ext_credential_t* stakeCredential
)
{
DENY_IF(forbiddenCredential(txSigningMode, stakeCredential));
DENY_IF(_forbiddenCredential(txSigningMode, stakeCredential));

switch (stakeCredential->type) {
case EXT_CREDENTIAL_KEY_PATH:
DENY_UNLESS(bip44_isOrdinaryStakingKeyPath(&stakeCredential->keyPath));
DENY_IF(violatesSingleAccountOrStoreIt(&stakeCredential->keyPath));
break;

default:
case EXT_CREDENTIAL_KEY_HASH:
case EXT_CREDENTIAL_SCRIPT_HASH:
// the rest is OK, forbidden credentials have been dealt with above
break;

default:
ASSERT(false);
}

PROMPT();
Expand Down Expand Up @@ -1123,9 +1133,21 @@ security_policy_t policyForSignTxCertificateVoteDelegation(
const ext_drep_t* drep
)
{
// DRep can be anything, but if given by key path, it should be a valid path
if (drep->type == EXT_DREP_KEY_PATH) {
switch (drep->type) {
case EXT_DREP_KEY_PATH:
// DRep can be anything, but if given by key path, it should be a valid path
DENY_UNLESS(bip44_isDRepKeyPath(&drep->keyPath));
break;

case EXT_DREP_KEY_HASH:
case EXT_DREP_SCRIPT_HASH:
case EXT_DREP_ABSTAIN:
case EXT_DREP_NO_CONFIDENCE:
// nothing to deny
break;

default:
ASSERT(false);
}

return _policyForSignTxCertificateStakeCredential(txSigningMode, stakeCredential);
Expand All @@ -1137,17 +1159,21 @@ security_policy_t policyForSignTxCertificateCommitteeAuth(
const ext_credential_t* hotCredential
)
{
DENY_IF(forbiddenCredential(txSigningMode, coldCredential));
DENY_IF(_forbiddenCredential(txSigningMode, coldCredential));

switch (coldCredential->type) {
case EXT_CREDENTIAL_KEY_PATH:
DENY_UNLESS(bip44_isCommitteeColdKeyPath(&coldCredential->keyPath));
DENY_IF(violatesSingleAccountOrStoreIt(&coldCredential->keyPath));
break;

default:
case EXT_CREDENTIAL_KEY_HASH:
case EXT_CREDENTIAL_SCRIPT_HASH:
// the rest is OK, forbidden credentials have been dealt with above
break;

default:
ASSERT(false);
}

switch (hotCredential->type) {
Expand All @@ -1173,17 +1199,21 @@ security_policy_t policyForSignTxCertificateCommitteeResign(
const ext_credential_t* coldCredential
)
{
DENY_IF(forbiddenCredential(txSigningMode, coldCredential));
DENY_IF(_forbiddenCredential(txSigningMode, coldCredential));

switch (coldCredential->type) {
case EXT_CREDENTIAL_KEY_PATH:
DENY_UNLESS(bip44_isCommitteeColdKeyPath(&coldCredential->keyPath));
DENY_IF(violatesSingleAccountOrStoreIt(&coldCredential->keyPath));
break;

default:
case EXT_CREDENTIAL_KEY_HASH:
case EXT_CREDENTIAL_SCRIPT_HASH:
// the rest is OK, forbidden credentials have been dealt with above
break;

default:
ASSERT(false);
}

PROMPT();
Expand All @@ -1194,17 +1224,21 @@ security_policy_t policyForSignTxCertificateDRep(
const ext_credential_t* dRepCredential
)
{
DENY_IF(forbiddenCredential(txSigningMode, dRepCredential));
DENY_IF(_forbiddenCredential(txSigningMode, dRepCredential));

switch (dRepCredential->type) {
case EXT_CREDENTIAL_KEY_PATH:
DENY_UNLESS(bip44_isDRepKeyPath(&dRepCredential->keyPath));
DENY_IF(violatesSingleAccountOrStoreIt(&dRepCredential->keyPath));
break;

default:
case EXT_CREDENTIAL_KEY_HASH:
case EXT_CREDENTIAL_SCRIPT_HASH:
// the rest is OK, forbidden credentials have been dealt with above
break;

default:
ASSERT(false);
}

PROMPT();
Expand Down Expand Up @@ -1265,7 +1299,6 @@ security_policy_t policyForSignTxStakePoolRegistrationInit(

default:
ASSERT(false);
break;
}

DENY(); // should not be reached
Expand Down Expand Up @@ -1439,7 +1472,6 @@ security_policy_t policyForSignTxWithdrawal(
// in POOL_REGISTRATION signing modes, this certificate should have already been
// reported as invalid (only pool registration certificate is allowed)
ASSERT(false);
break;
}
break;

Expand Down Expand Up @@ -1467,7 +1499,6 @@ security_policy_t policyForSignTxWithdrawal(
// in POOL_REGISTRATION signing modes, this certificate should have already been
// reported as invalid (only pool registration certificate is allowed)
ASSERT(false);
break;
}
break;

Expand All @@ -1488,15 +1519,13 @@ security_policy_t policyForSignTxWithdrawal(
// in POOL_REGISTRATION signing modes, this certificate should have already been
// reported as invalid (only pool registration certificate is allowed)
ASSERT(false);
break;
}
break;

default:
// in POOL_REGISTRATION signing modes, non-zero number of withdrawals
// should have already been reported as invalid
ASSERT(false);
break;
}

DENY(); // should not be reached
Expand All @@ -1521,8 +1550,10 @@ static inline security_policy_t _ordinaryWitnessPolicy(const bip44_path_t* path,
case PATH_DREP_KEY:
case PATH_COMMITTEE_COLD_KEY:
case PATH_COMMITTEE_HOT_KEY:
// these have to be shown because the tx might contain
// an action proposal that cannot be fully shown on the device
// used to sign certificates and voting procedures
// these won't occur often, so little benefit from hiding them
// better to show them at least while they are new
// in the future, we might want to hide some of them in non-expert mode
DENY_IF(violatesSingleAccountOrStoreIt(path));
WARN_UNLESS(bip44_isPathReasonable(path));
SHOW();
Expand Down Expand Up @@ -1664,8 +1695,6 @@ security_policy_t policyForSignTxWitness(
const bip44_path_t* poolOwnerPath __attribute__((unused))
)
{
// TODO what about witnesses for voting procedures?

switch (txSigningMode) {
case SIGN_TX_SIGNINGMODE_ORDINARY_TX:
return _ordinaryWitnessPolicy(witnessPath, mintPresent);
Expand Down Expand Up @@ -1955,7 +1984,7 @@ security_policy_t policyForSignTxVotingProcedure(
break;

default:
break;
ASSERT(false);
}
break;

Expand All @@ -1971,8 +2000,13 @@ security_policy_t policyForSignTxVotingProcedure(
DENY();
break;

default:
case EXT_VOTER_COMMITTEE_HOT_SCRIPT_HASH:
case EXT_VOTER_DREP_SCRIPT_HASH:
// scripts are OK
break;

default:
ASSERT(false);
}
break;

Expand All @@ -1984,7 +2018,6 @@ security_policy_t policyForSignTxVotingProcedure(
default:
// this should not be called in POOL_REGISTRATION signing modes
ASSERT(false);
break;
}

SHOW();
Expand All @@ -1993,7 +2026,7 @@ security_policy_t policyForSignTxVotingProcedure(
// For treasury
security_policy_t policyForSignTxTreasury(
sign_tx_signingmode_t txSigningMode MARK_UNUSED,
uint64_t coin MARK_UNUSED
uint64_t treasury MARK_UNUSED
)
{
SHOW();
Expand All @@ -2002,7 +2035,7 @@ security_policy_t policyForSignTxTreasury(
// For donation
security_policy_t policyForSignTxDonation(
sign_tx_signingmode_t txSigningMode MARK_UNUSED,
uint64_t coin MARK_UNUSED
uint64_t donation MARK_UNUSED
)
{
SHOW();
Expand Down Expand Up @@ -2081,7 +2114,6 @@ security_policy_t policyForCVoteRegistrationPaymentDestination(

default:
ASSERT(false);
break;
}

DENY(); // should not be reached
Expand Down
Loading

0 comments on commit c6352df

Please sign in to comment.