Skip to content

Commit e354f38

Browse files
Merge pull request #1026 from LedgerHQ/cev/fix_cerberus_high
Fix cerberus high
2 parents fd46d04 + b8f41b5 commit e354f38

141 files changed

Lines changed: 349 additions & 46 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

src/features/set_external_plugin/cmd_set_external_plugin.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,20 @@ uint16_t handle_set_external_plugin(const uint8_t *workBuffer, uint8_t dataLengt
7373
workBuffer += ADDRESS_LENGTH;
7474
memmove(dataContext.tokenContext.methodSelector, workBuffer, SELECTOR_SIZE);
7575
pluginType = PLUGIN_TYPE_EXTERNAL;
76+
// External-plugin registration is intentionally chain-unbound: the signed
77+
// payload here is [name_len | name | address | selector] and contains no
78+
// chain_id field, unlike handle_set_plugin which does carry one. Marking
79+
// the binding as PLUGIN_CHAIN_ID_ANY is required so the deferred chain
80+
// check in finalize_parsing_helper() lets the registration through.
81+
//
82+
// Closing the cross-chain replay surface for external plugins (Cerberus
83+
// CWE-345 follow-up) requires extending the signed payload format on
84+
// both the device and the metadata-signing side, and is intentionally
85+
// out of scope of this fix. Tracked separately, do not patch in place:
86+
// naively widening payload_size here would either reject every existing
87+
// signature (the extra bytes are not yet sent) or accept attacker-
88+
// controlled bytes outside the signed range.
89+
dataContext.tokenContext.pluginChainId = PLUGIN_CHAIN_ID_ANY;
7690

7791
return SWO_SUCCESS;
7892
}

src/features/set_plugin/cmd_set_plugin.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,11 @@ uint16_t handle_set_plugin(const uint8_t *workBuffer, uint8_t dataLength) {
145145
UNSUPPORTED_CHAIN_ID_MSG(chain_id);
146146
return SWO_INCORRECT_DATA;
147147
}
148+
// Bind the registration to the signed chain. Without this, a host could
149+
// load a valid signed plugin registration for one EVM chain and then have
150+
// the device activate the plugin UI for a transaction on a different
151+
// chain where the same address/selector means something else.
152+
tokenContext->pluginChainId = chain_id;
148153
offset += CHAIN_ID_SIZE;
149154

150155
keyId = workBuffer[offset];

src/features/sign_authorization_eip7702/commands_7702.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,13 +189,28 @@ uint16_t handle_sign_eip7702_authorization(uint8_t p1,
189189
uint8_t dataLength) {
190190
g_7702_sw = SWO_PARAMETER_ERROR_NO_INFO;
191191
if (p1 == P1_FIRST_CHUNK) {
192+
// Lock the EIP-7702 authorization flow against being restarted while
193+
// another signing/review is in progress. Without this guard, a hostile
194+
// host could overwrite tmpCtx.authSigningContext7702.bip32 during a
195+
// pending review and trick the user into signing with a path other
196+
// than the one displayed on screen.
197+
if (appState != APP_STATE_IDLE) {
198+
PRINTF("Cannot start an EIP-7702 authorization while another flow is active\n");
199+
return SWO_COMMAND_NOT_ALLOWED;
200+
}
201+
appState = APP_STATE_SIGNING_EIP7702;
192202
if ((dataBuffer =
193203
parseBip32(dataBuffer, &dataLength, &tmpCtx.authSigningContext7702.bip32)) ==
194204
NULL) {
205+
reset_app_context();
195206
return SWO_INCORRECT_DATA;
196207
}
208+
} else if (appState != APP_STATE_SIGNING_EIP7702) {
209+
PRINTF("EIP-7702 continuation chunk without an active authorization session\n");
210+
return SWO_COMMAND_NOT_ALLOWED;
197211
}
198212
if (!tlv_from_apdu(p1 == P1_FIRST_CHUNK, dataLength, dataBuffer, &handle_auth7702_tlv)) {
213+
reset_app_context();
199214
return g_7702_sw;
200215
}
201216
return SWO_NO_RESPONSE;

src/features/sign_authorization_eip7702/ui_common_7702.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@ unsigned int auth_7702_ok_cb(void) {
2121
} else {
2222
G_io_tx_buffer[0] = 0;
2323
}
24-
return io_seproxyhal_send_status(SWO_SUCCESS, ECDSA_SIGNATURE_LENGTH, false, true);
24+
// Reset to release the APP_STATE_SIGNING_EIP7702 lock and clear the
25+
// signing context so a new authorization can start from a clean slate.
26+
return io_seproxyhal_send_status(SWO_SUCCESS, ECDSA_SIGNATURE_LENGTH, true, true);
2527
}
2628

2729
unsigned int auth_7702_cancel_cb(void) {

src/features/sign_tx/logic_sign_tx.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,20 @@ __attribute__((noinline)) static uint16_t finalize_parsing_helper(const txContex
318318
goto end;
319319
}
320320
PRINTF("FROM address displayed: %s\n", strings.common.fromAddress);
321+
// Enforce chain binding for plugin registrations now that the full tx has
322+
// been parsed and get_tx_chain_id() is reliable (LEGACY transactions only
323+
// expose chain_id through the V field, which is parsed after the plugin
324+
// init triggered on the data field).
325+
if ((dataContext.tokenContext.pluginStatus >= ETH_PLUGIN_RESULT_SUCCESSFUL) &&
326+
(dataContext.tokenContext.pluginChainId != PLUGIN_CHAIN_ID_ANY) &&
327+
(dataContext.tokenContext.pluginChainId != chain_id)) {
328+
PRINTF("Plugin registered for chain %llu but tx is on chain %llu\n",
329+
dataContext.tokenContext.pluginChainId,
330+
chain_id);
331+
report_finalize_error();
332+
error = SWO_NO_RESPONSE;
333+
goto end;
334+
}
321335
// Finalize the plugin handling
322336
if (dataContext.tokenContext.pluginStatus >= ETH_PLUGIN_RESULT_SUCCESSFUL) {
323337
eth_plugin_prepare_finalize(&pluginFinalize);

src/nbgl/ui_home.c

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,20 @@ nbgl_warning_t warning;
1818
// Tagline format for plugins
1919
#define FORMAT_PLUGIN "This app enables clear\nsigning transactions for\nthe %s dApp."
2020

21+
// Maximum caller-provided plugin name length we accept for the tagline. 64 is
22+
// generous compared to real Ledger plugin app names (typically < 20 chars) and
23+
// the fuzz harnesses' NAME_LENGTH=32, while staying well below the byte that
24+
// would let the total line length wrap. Any value below ~196 closes the
25+
// CWE-120 overflow.
26+
#define MAX_PLUGIN_NAME_LEN 64
27+
28+
// Catch any future change to FORMAT_PLUGIN or MAX_PLUGIN_NAME_LEN that would
29+
// re-introduce a path where the tagline allocation length wraps. sizeof
30+
// includes the NUL terminator; 256 keeps a comfortable margin below the 8-bit
31+
// boundary that used to wrap the original uint8_t accumulator.
32+
_Static_assert(sizeof(FORMAT_PLUGIN) + MAX_PLUGIN_NAME_LEN < 256,
33+
"Plugin tagline buffer math exceeds the historical 8-bit bound ");
34+
2135
enum {
2236
TRANSACTION_CHECKS_TOKEN = FIRST_USER_TOKEN,
2337
BLIND_SIGNING_TOKEN,
@@ -211,14 +225,18 @@ static void prepare_and_display_home(const char *appname, const char *tagline, u
211225
*/
212226
static void get_appname_and_tagline(const char **appname, const char **tagline) {
213227
uint64_t mainnet_chain_id;
214-
uint8_t line_len = 1; // Initialize lengths to 1 for '\0' character
215228

216229
if (caller_app) {
217230
*appname = caller_app->name;
218231

219232
if (caller_app->type == CALLER_TYPE_PLUGIN) {
220-
line_len += strlen(FORMAT_PLUGIN);
221-
line_len += strlen(caller_app->name);
233+
size_t name_len = strnlen(caller_app->name, MAX_PLUGIN_NAME_LEN + 1);
234+
if (name_len > MAX_PLUGIN_NAME_LEN) {
235+
PRINTF("Plugin caller_app->name exceeds %u bytes; tagline omitted\n",
236+
(unsigned) MAX_PLUGIN_NAME_LEN);
237+
return;
238+
}
239+
size_t line_len = 1 + strlen(FORMAT_PLUGIN) + name_len;
222240
// Allocate the buffer - will never be deallocated...
223241
if (APP_MEM_CALLOC((void **) &g_tag_line, line_len) == true) {
224242
snprintf(g_tag_line, line_len, FORMAT_PLUGIN, *appname);

src/plugins/eip7002/eip7002_plugin.c

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,48 @@ static void eip7002_plugin_provider_parameter(ethPluginProvideParameter_t *param
6262
}
6363
}
6464

65+
// EIP-7002 charges a dynamic per-request fee paid in native value. In normal
66+
// operation this fee is in the wei-to-gwei range — showing a "Tx value: 1 wei"
67+
// screen on every legitimate request is noisy without informing the user.
68+
// Hide the screen as long as the value stays below this threshold; anything
69+
// above is worth surfacing because it dwarfs the protocol fee. The attacker
70+
// budget for a hidden value is therefore capped at TX_VALUE_MIN_DISPLAY_WEI,
71+
// i.e. dust ($0.000004 at $4000/ETH), instead of the unbounded original CVE.
72+
#define TX_VALUE_MIN_DISPLAY_WEI 1000000000ULL // 1 gwei
73+
74+
// Whether the transaction carries a native value worth surfacing to the user.
75+
// NULL-safe so callers can pass param->txContent directly from either the
76+
// ETH_PLUGIN_FINALIZE or ETH_PLUGIN_QUERY_CONTRACT_UI message structs.
77+
static bool has_tx_value(const txContent_t *txContent) {
78+
if ((txContent == NULL) || (txContent->value.length == 0)) {
79+
return false;
80+
}
81+
if (txContent->value.length > sizeof(uint64_t)) {
82+
// > 2^64 wei is unambiguously larger than the threshold.
83+
return true;
84+
}
85+
uint64_t val = 0;
86+
for (uint8_t i = 0; i < txContent->value.length; ++i) {
87+
val = (val << 8) | txContent->value.value[i];
88+
}
89+
return val > TX_VALUE_MIN_DISPLAY_WEI;
90+
}
91+
6592
static void eip7002_plugin_finalize(ethPluginFinalize_t *param) {
6693
eip7002_context_t *context = (eip7002_context_t *) param->pluginContext;
6794

6895
param->uiType = ETH_UI_TYPE_GENERIC;
69-
param->numScreens = is_zeroes_buffer(context->raw_amount, sizeof(context->raw_amount)) ? 1 : 2;
96+
// Validator screen is always shown. The native tx.value is shown only
97+
// when it exceeds the dust threshold defined by has_tx_value, so a
98+
// hostile dApp cannot smuggle a meaningful ETH/native value into a
99+
// staking-style request that otherwise only renders calldata.
100+
param->numScreens = 1;
101+
if (has_tx_value(param->txContent)) {
102+
param->numScreens++;
103+
}
104+
if (!is_zeroes_buffer(context->raw_amount, sizeof(context->raw_amount))) {
105+
param->numScreens++;
106+
}
70107
param->result = (context->received == sizeof(context->withdrawal_request))
71108
? ETH_PLUGIN_RESULT_OK
72109
: ETH_PLUGIN_RESULT_ERROR;
@@ -88,9 +125,23 @@ static void eip7002_plugin_query_contract_ui(ethQueryContractUI_t *param) {
88125
eip7002_context_t *context = (eip7002_context_t *) param->pluginContext;
89126
uint64_t chain_id = get_tx_chain_id();
90127
const char *ticker = get_displayable_ticker(&chain_id, g_chain_config, true);
128+
// Map a screen index to a logical screen kind based on which optional
129+
// screens are present for this transaction.
130+
bool show_tx_value = has_tx_value(param->txContent);
131+
bool show_request_amount = !is_zeroes_buffer(context->raw_amount, sizeof(context->raw_amount));
132+
uint8_t idx = param->screenIndex;
133+
enum { S_VALIDATOR, S_TX_VALUE, S_REQUEST_AMOUNT, S_UNKNOWN } screen = S_UNKNOWN;
134+
135+
if (idx == 0) {
136+
screen = S_VALIDATOR;
137+
} else if (show_tx_value && idx == 1) {
138+
screen = S_TX_VALUE;
139+
} else if (show_request_amount && idx == (show_tx_value ? 2 : 1)) {
140+
screen = S_REQUEST_AMOUNT;
141+
}
91142

92-
switch (param->screenIndex) {
93-
case 0:
143+
switch (screen) {
144+
case S_VALIDATOR:
94145
if (param->msgLength < 2) {
95146
return;
96147
}
@@ -101,7 +152,19 @@ static void eip7002_plugin_query_contract_ui(ethQueryContractUI_t *param) {
101152
&param->msg[2],
102153
param->msgLength - 2);
103154
break;
104-
case 1:
155+
case S_TX_VALUE:
156+
strlcpy(param->title, "Tx value", param->titleLength);
157+
if (!amountToString(param->txContent->value.value,
158+
param->txContent->value.length,
159+
WEI_TO_ETHER,
160+
ticker,
161+
param->msg,
162+
param->msgLength)) {
163+
param->result = ETH_PLUGIN_RESULT_ERROR;
164+
return;
165+
}
166+
break;
167+
case S_REQUEST_AMOUNT:
105168
strlcpy(param->title, "Amount", param->titleLength);
106169
if (!amountToString(context->raw_amount,
107170
sizeof(context->raw_amount),
@@ -110,10 +173,10 @@ static void eip7002_plugin_query_contract_ui(ethQueryContractUI_t *param) {
110173
param->msg,
111174
param->msgLength)) {
112175
param->result = ETH_PLUGIN_RESULT_ERROR;
113-
break;
176+
return;
114177
}
115178
break;
116-
default:
179+
case S_UNKNOWN:
117180
break;
118181
}
119182
param->result = ETH_PLUGIN_RESULT_OK;

src/plugins/eip7251/eip7251_plugin.c

Lines changed: 90 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,49 @@ static void eip7251_plugin_provider_parameter(ethPluginProvideParameter_t *param
6464
}
6565
}
6666

67+
// EIP-7251 charges a dynamic per-request fee paid in native value. In normal
68+
// operation this fee is in the wei-to-gwei range — showing a "Tx value: 1 wei"
69+
// screen on every legitimate request is noisy without informing the user.
70+
// Hide the screen as long as the value stays below this threshold; anything
71+
// above is worth surfacing because it dwarfs the protocol fee. The attacker
72+
// budget for a hidden value is therefore capped at TX_VALUE_MIN_DISPLAY_WEI,
73+
// i.e. dust ($0.000004 at $4000/ETH), instead of the unbounded original CVE.
74+
#define TX_VALUE_MIN_DISPLAY_WEI 1000000000ULL // 1 gwei
75+
76+
// Whether the transaction carries a native value worth surfacing to the user.
77+
// NULL-safe so callers can pass param->txContent directly from either the
78+
// ETH_PLUGIN_FINALIZE or ETH_PLUGIN_QUERY_CONTRACT_UI message structs.
79+
static bool has_tx_value(const txContent_t *txContent) {
80+
if ((txContent == NULL) || (txContent->value.length == 0)) {
81+
return false;
82+
}
83+
if (txContent->value.length > sizeof(uint64_t)) {
84+
// > 2^64 wei is unambiguously larger than the threshold.
85+
return true;
86+
}
87+
uint64_t val = 0;
88+
for (uint8_t i = 0; i < txContent->value.length; ++i) {
89+
val = (val << 8) | txContent->value.value[i];
90+
}
91+
return val > TX_VALUE_MIN_DISPLAY_WEI;
92+
}
93+
6794
static void eip7251_plugin_finalize(ethPluginFinalize_t *param) {
6895
eip7251_context_t *context = (eip7251_context_t *) param->pluginContext;
6996

7097
param->uiType = ETH_UI_TYPE_GENERIC;
71-
param->numScreens = target_equals_source(context) ? 1 : 2;
98+
// Source validator is always shown. Target is shown when distinct. The
99+
// native tx.value is shown only when it exceeds the dust threshold
100+
// defined by has_tx_value, so a hostile dApp cannot smuggle a meaningful
101+
// ETH/native value into a consolidation request that otherwise only
102+
// renders validator pubkeys.
103+
param->numScreens = 1;
104+
if (!target_equals_source(context)) {
105+
param->numScreens++;
106+
}
107+
if (has_tx_value(param->txContent)) {
108+
param->numScreens++;
109+
}
72110
param->result = (context->received == sizeof(context->consolidation_request))
73111
? ETH_PLUGIN_RESULT_OK
74112
: ETH_PLUGIN_RESULT_ERROR;
@@ -86,33 +124,60 @@ static void eip7251_plugin_query_contract_id(ethQueryContractID_t *param) {
86124

87125
static void eip7251_plugin_query_contract_ui(ethQueryContractUI_t *param) {
88126
eip7251_context_t *context = (eip7251_context_t *) param->pluginContext;
127+
// Map a screen index to a logical screen kind based on which optional
128+
// screens are present for this transaction.
129+
bool show_target = !target_equals_source(context);
130+
bool show_tx_value = has_tx_value(param->txContent);
131+
uint8_t idx = param->screenIndex;
132+
enum { S_SOURCE, S_TARGET, S_TX_VALUE, S_UNKNOWN } screen = S_UNKNOWN;
133+
134+
if (idx == 0) {
135+
screen = S_SOURCE;
136+
} else if (show_target && idx == 1) {
137+
screen = S_TARGET;
138+
} else if (show_tx_value && idx == (show_target ? 2 : 1)) {
139+
screen = S_TX_VALUE;
140+
}
89141

90-
if (param->msgLength >= 2) {
91-
memcpy(param->msg, "0x", 2);
92-
switch (param->screenIndex) {
93-
case 0:
94-
if (target_equals_source(context)) {
95-
strlcpy(param->title, "Validator", param->titleLength);
96-
} else {
97-
strlcpy(param->title, "From validator", param->titleLength);
98-
}
99-
format_hex(context->source_pubkey,
100-
sizeof(context->source_pubkey),
101-
&param->msg[2],
102-
param->msgLength - 2);
103-
break;
104-
case 1:
105-
strlcpy(param->title, "To validator", param->titleLength);
106-
format_hex(context->target_pubkey,
107-
sizeof(context->target_pubkey),
108-
&param->msg[2],
109-
param->msgLength - 2);
110-
break;
111-
default:
112-
break;
142+
if (param->msgLength < 2) {
143+
return;
144+
}
145+
switch (screen) {
146+
case S_SOURCE:
147+
memcpy(param->msg, "0x", 2);
148+
strlcpy(param->title, show_target ? "From validator" : "Validator", param->titleLength);
149+
format_hex(context->source_pubkey,
150+
sizeof(context->source_pubkey),
151+
&param->msg[2],
152+
param->msgLength - 2);
153+
break;
154+
case S_TARGET:
155+
memcpy(param->msg, "0x", 2);
156+
strlcpy(param->title, "To validator", param->titleLength);
157+
format_hex(context->target_pubkey,
158+
sizeof(context->target_pubkey),
159+
&param->msg[2],
160+
param->msgLength - 2);
161+
break;
162+
case S_TX_VALUE: {
163+
uint64_t chain_id = get_tx_chain_id();
164+
const char *ticker = get_displayable_ticker(&chain_id, g_chain_config, true);
165+
strlcpy(param->title, "Tx value", param->titleLength);
166+
if (!amountToString(param->txContent->value.value,
167+
param->txContent->value.length,
168+
WEI_TO_ETHER,
169+
ticker,
170+
param->msg,
171+
param->msgLength)) {
172+
param->result = ETH_PLUGIN_RESULT_ERROR;
173+
return;
174+
}
175+
break;
113176
}
114-
param->result = ETH_PLUGIN_RESULT_OK;
177+
case S_UNKNOWN:
178+
return;
115179
}
180+
param->result = ETH_PLUGIN_RESULT_OK;
116181
}
117182

118183
void eip7251_plugin_call(eth_plugin_msg_t msg, void *param) {

src/plugins/erc1155/erc1155_plugin.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,13 @@ void handle_finalize_1155(ethPluginFinalize_t *msg) {
5959
msg->numScreens = 5;
6060
break;
6161
case SAFE_BATCH_TRANSFER:
62-
msg->numScreens = 4;
62+
// To, Collection Name, NFT Address, Total Quantity
63+
// + 2 screens per displayed pair (ID + Quantity)
64+
// + 1 warning screen if truncated.
65+
msg->numScreens = 4 + 2 * context->batch_displayed;
66+
if (context->batch_truncated) {
67+
msg->numScreens += 1;
68+
}
6369
break;
6470
case SET_APPROVAL_FOR_ALL:
6571
msg->numScreens = 3;

0 commit comments

Comments
 (0)