Blindsign Improvement #155
Conversation
include/zxversion.h
Outdated
| @@ -17,4 +17,4 @@ | |||
|
|
|||
| #define ZXLIB_MAJOR 29 | |||
| #define ZXLIB_MINOR 5 | |||
| #define ZXLIB_PATCH 1 | |||
| #define ZXLIB_PATCH 2 | |||
There was a problem hiding this comment.
New feature. We should bump minor.
src/app_mode.c
Outdated
| @@ -20,6 +20,7 @@ typedef struct { | |||
| uint8_t expert; | |||
| uint8_t account; | |||
| uint8_t blindsign; | |||
| uint8_t blindsign_required; | |||
There was a problem hiding this comment.
Do we really need this to be on flash? This is something that is set on each operation, so I guess it is not necessary.
There was a problem hiding this comment.
Indeed, we don't need this on flash. Let me rethink the approach.
app/ui/view_nano.c
Outdated
| @@ -183,7 +183,7 @@ zxerr_t h_review_update_data() { | |||
| snprintf(viewdata.value, MAX_CHARS_PER_VALUE1_LINE, "Ok"); | |||
| } else { | |||
| #if defined(APP_BLINDSIGN_MODE_ENABLED) | |||
| if (app_mode_blindsign()) { | |||
| if (app_mode_blindsign() && app_mode_blindsign_required()) { | |||
There was a problem hiding this comment.
If we do so, this is not backward compatible to what we currently have. We will end up with blind signing not being displayed, unless the flag is set, which is not the current scenario we have. If someone updates zxlib without changing this in the app, no method will display the blind signing screens
There was a problem hiding this comment.
Sure I will zxlib major since this does not have backward compatible.
There was a problem hiding this comment.
We could make it if we set blindsign_required = 1 whenever blind signing mode is enabled. Then, you need to indicate which methods will not require the blind signing screens while this mode is enabled. I think this way is even more secure, as worst case we show screen when no needed. Instead of not showing them when it is indeed needed
include/app_mode.h
Outdated
| * limitations under the License. | ||
| ********************************************************************************/ | ||
| * (c) 2016 Ledger | ||
| * (c) 2018 Zondax GmbH |
There was a problem hiding this comment.
We could take the opportunity to update this "Zondax AG"
chcmedeiros
force-pushed
the
fix
branch
2 times, most recently
from
45e813f
to
cbd3e6b
Compare
| @@ -531,7 +531,7 @@ void run_ux_review_flow(review_type_e reviewType, const ux_flow_step_t *const st | |||
| case REVIEW_TXN: | |||
| default: | |||
| #ifdef APP_BLINDSIGN_MODE_ENABLED | |||
| if (app_mode_blindsign()) { | |||
| if (app_mode_blindsign_required()) { | |||
There was a problem hiding this comment.
Should we check if app blind signing mode is enabled, and if it is required? Or are we just covering both at the same time?
There was a problem hiding this comment.
If you check both, we run into the following: BS is enabled but not required, if we check if app_mode_blindsign is enabled we will force the required to true but indeed we shouldn't
There was a problem hiding this comment.
mmmm what a problem... because if we set required to true, even if the blind signing mode is not enabled, we will display the screens... right?
There was a problem hiding this comment.
There is no way to set the required to true on the code.
We can only set it to zero with app_mode_blindising_skip_ui(), otherwise it will follow app_mode_blindsign enabled or not value
There was a problem hiding this comment.
Initial possabilities:
BS is enabled by user required will be set to true;
BS is disabled by user required will be set to false;
From that point:
Code checks if BS is true -> Bs is true -> required set to true
Code checks if BS is false -> Bs is false -> required maintains its value. But if BS is false it was set by the user and at that point required will also be set to zero
🔗 zboto Link