Skip to content

Commit

Permalink
feat(core): increase maximum passphrase length to 128 bytes
Browse files Browse the repository at this point in the history
Legacy is still limited to 50 bytes.

Also, deduplicate common passphrase test vectors definitions.
  • Loading branch information
romanz committed Jan 20, 2025
1 parent e9aca68 commit dd92be9
Show file tree
Hide file tree
Showing 16 changed files with 106 additions and 132 deletions.
1 change: 1 addition & 0 deletions core/.changelog.d/4084.changed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Increase maximum passphrase length to 128 bytes.
8 changes: 5 additions & 3 deletions core/embed/rust/src/ui/component/base.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use heapless::Vec;
use heapless::{String, Vec};

use crate::{
strutil::{ShortString, TString},
strutil::TString,
time::Duration,
ui::{
button_request::{ButtonRequest, ButtonRequestCode},
Expand Down Expand Up @@ -592,6 +592,8 @@ impl EventCtx {
}
}

pub type FlowMsgText = String<128>;

/// Component::Msg for component parts of a swipe flow. Converting results of
/// different screens to a shared type makes things easier to work with.
///
Expand All @@ -603,7 +605,7 @@ pub enum FlowMsg {
Cancelled,
Info,
Choice(usize),
Text(ShortString),
Text(FlowMsgText),
}

#[cfg(feature = "micropython")]
Expand Down
10 changes: 5 additions & 5 deletions core/embed/rust/src/ui/component/text/common.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
strutil::ShortString,
ui::{component::EventCtx, util::ResultExt},
use crate::ui::{
component::{base::FlowMsgText, EventCtx},
util::ResultExt,
};

/// Reified editing operations of `TextBox`.
Expand All @@ -16,13 +16,13 @@ pub enum TextEdit {
/// operations over it. Text ops usually take a `EventCtx` to request a paint
/// pass in case of any state modification.
pub struct TextBox {
text: ShortString,
text: FlowMsgText,
}

impl TextBox {
/// Create a new `TextBox` with content `text`.
pub fn new(text: &str, max_len: usize) -> Self {
let text = unwrap!(ShortString::try_from(text));
let text = unwrap!(FlowMsgText::try_from(text));
debug_assert!(text.capacity() >= max_len);
Self { text }
}
Expand Down
2 changes: 1 addition & 1 deletion core/embed/rust/src/ui/layout/obj.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ impl LayoutObj {
}

pub fn new_root(root: impl LayoutMaybeTrace + 'static) -> Result<Gc<Self>, Error> {
// SAFETY: This is a Python object and hase a base as first element
// SAFETY: This is a Python object and has a base as first element
unsafe {
Gc::new_with_custom_finaliser(Self {
base: Self::obj_type().as_base(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const KEYBOARD: [[&str; KEY_COUNT]; PAGE_COUNT] = [
["_<>", ".:@", "/|\\", "!()", "+%&", "-[]", "?{}", ",'`", ";\"~", "$^="],
];

const MAX_LENGTH: usize = 50;
const MAX_LENGTH: usize = 128;
const INPUT_AREA_HEIGHT: i16 = ScrollBar::DOT_SIZE + 9;

impl PassphraseKeyboard {
Expand Down
19 changes: 8 additions & 11 deletions core/embed/rust/src/ui/layout_caesar/component/changing_text.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
use crate::{
strutil::ShortString,
ui::{
component::{Component, Event, EventCtx, Never, Pad},
display::Font,
geometry::{Alignment, Point, Rect},
shape::{self, Renderer},
util::long_line_content_with_ellipsis,
},
use crate::ui::{
component::{base::FlowMsgText, Component, Event, EventCtx, Never, Pad},
display::Font,
geometry::{Alignment, Point, Rect},
shape::{self, Renderer},
util::long_line_content_with_ellipsis,
};

use super::theme;
Expand All @@ -16,7 +13,7 @@ use super::theme;
/// and without being affected by other components.
pub struct ChangingTextLine {
pad: Pad,
text: ShortString,
text: FlowMsgText,
font: Font,
/// Whether to show the text. Can be disabled.
show_content: bool,
Expand All @@ -29,7 +26,7 @@ pub struct ChangingTextLine {

impl ChangingTextLine {
pub fn new(text: &str, font: Font, alignment: Alignment, max_len: usize) -> Self {
let text = unwrap!(ShortString::try_from(text));
let text = unwrap!(FlowMsgText::try_from(text));
debug_assert!(text.capacity() >= max_len);
Self {
pad: Pad::with_background(theme::BG),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use crate::{
strutil::{ShortString, TString},
strutil::TString,
translations::TR,
trezorhal::random,
ui::{
component::{text::common::TextBox, Child, Component, ComponentExt, Event, EventCtx},
component::{
base::FlowMsgText, text::common::TextBox, Child, Component, ComponentExt, Event,
EventCtx,
},
display::Icon,
geometry::Rect,
shape::Renderer,
Expand All @@ -26,7 +29,7 @@ enum ChoiceCategory {
SpecialSymbol,
}

const MAX_PASSPHRASE_LENGTH: usize = 50;
const MAX_PASSPHRASE_LENGTH: usize = 128;

const DIGITS: &str = "0123456789";
const LOWERCASE_LETTERS: &str = "abcdefghijklmnopqrstuvwxyz";
Expand Down Expand Up @@ -291,17 +294,17 @@ impl PassphraseEntry {

fn update_passphrase_dots(&mut self, ctx: &mut EventCtx) {
debug_assert!({
let s = ShortString::new();
let s = FlowMsgText::new();
s.capacity() >= MAX_PASSPHRASE_LENGTH
});

let text_to_show = if self.show_plain_passphrase {
unwrap!(ShortString::try_from(self.passphrase()))
unwrap!(FlowMsgText::try_from(self.passphrase()))
} else if self.is_empty() {
unwrap!(ShortString::try_from(""))
unwrap!(FlowMsgText::try_from(""))
} else {
// Showing asterisks and possibly the last digit.
let mut dots = ShortString::new();
let mut dots = FlowMsgText::new();
for _ in 0..self.textbox.len() - 1 {
unwrap!(dots.push('*'));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::{
strutil::{ShortString, TString},
strutil::TString,
translations::TR,
ui::{
component::{
base::ComponentExt, swipe_detect::SwipeConfig, text::common::TextBox, Component, Event,
EventCtx, Label, Maybe, Never, Swipe,
base::{ComponentExt, FlowMsgText},
swipe_detect::SwipeConfig,
text::common::TextBox,
Component, Event, EventCtx, Label, Maybe, Never, Swipe,
},
display,
geometry::{Alignment, Direction, Grid, Insets, Offset, Rect},
Expand All @@ -27,7 +29,7 @@ use core::cell::Cell;
use num_traits::ToPrimitive;

pub enum PassphraseKeyboardMsg {
Confirmed(ShortString),
Confirmed(FlowMsgText),
Cancelled,
}

Expand Down Expand Up @@ -101,7 +103,7 @@ const KEYBOARD: [[&str; KEY_COUNT]; PAGE_COUNT] = [
["_<>", ".:@", "/|\\", "!()", "+%&", "-[]", "?{}", ",'`", ";\"~", "$^="],
];

const MAX_LENGTH: usize = 50;
const MAX_LENGTH: usize = 128;

const CONFIRM_BTN_INSETS: Insets = Insets::new(5, 0, 5, 0);
const CONFIRM_EMPTY_BTN_MARGIN_RIGHT: i16 = 7;
Expand Down Expand Up @@ -338,12 +340,12 @@ impl Component for PassphraseKeyboard {
// Confirm button was clicked, we're done.
if let Some(ButtonMsg::Clicked) = self.confirm_empty_btn.event(ctx, event) {
return Some(PassphraseKeyboardMsg::Confirmed(unwrap!(
ShortString::try_from(self.passphrase())
FlowMsgText::try_from(self.passphrase())
)));
}
if let Some(ButtonMsg::Clicked) = self.confirm_btn.event(ctx, event) {
return Some(PassphraseKeyboardMsg::Confirmed(unwrap!(
ShortString::try_from(self.passphrase())
FlowMsgText::try_from(self.passphrase())
)));
}
if let Some(ButtonMsg::Clicked) = self.cancel_btn.event(ctx, event) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use crate::{
error,
strutil::ShortString,
translations::TR,
ui::{
component::ComponentExt,
component::{base::FlowMsgText, ComponentExt},
flow::{
base::{Decision, DecisionBuilder as _},
FlowController, FlowMsg, SwipeFlow,
Expand Down Expand Up @@ -44,7 +43,7 @@ impl FlowController for RequestPassphrase {
(Self::Keypad, FlowMsg::Cancelled) => self.return_msg(FlowMsg::Cancelled),
(Self::ConfirmEmpty, FlowMsg::Cancelled) => Self::Keypad.goto(),
(Self::ConfirmEmpty, FlowMsg::Confirmed) => {
self.return_msg(FlowMsg::Text(ShortString::new()))
self.return_msg(FlowMsg::Text(FlowMsgText::new()))
}
_ => self.do_nothing(),
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/apps/common/passphrase.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import storage.device as storage_device
from trezor.wire import DataError

_MAX_PASSPHRASE_LEN = const(50)
_MAX_PASSPHRASE_LEN = const(128)


def is_enabled() -> bool:
Expand Down
2 changes: 1 addition & 1 deletion python/src/trezorlib/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

LOG = logging.getLogger(__name__)

MAX_PASSPHRASE_LENGTH = 50
MAX_PASSPHRASE_LENGTH = 128
MAX_PIN_LENGTH = 50

PASSPHRASE_ON_DEVICE = object()
Expand Down
26 changes: 26 additions & 0 deletions tests/click_tests/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,32 @@ class CommonPass:

EMPTY_ADDRESS = "mvbu1Gdy8SUjTenqerxUaZyYjmveZvt33q"

LONGEST = 64 * "da"
LONGEST_ADDRESS = "mn13QW58ziH2obHPZmGnAhBrqkbYf3LRpo"

ALMOST_LONGEST = LONGEST[:-1]
ALMOST_LONGEST_ADDRESS = "n48uTdBpsWzNb26ZNysXgkQiocYpR37D2m"

TOO_LONG = LONGEST + "d"
TOO_LONG_ADDRESS = LONGEST_ADDRESS

VECTORS = ( # passphrase, address
(SHORT, SHORT_ADDRESS),
(WITH_SPACE, WITH_SPACE_ADDRESS),
(RANDOM_25, RANDOM_25_ADDRESS),
(ALMOST_LONGEST, ALMOST_LONGEST_ADDRESS),
(LONGEST, LONGEST_ADDRESS),
)


assert len(CommonPass.ALMOST_LONGEST) == 127
assert len(CommonPass.LONGEST) == 128
assert len(CommonPass.TOO_LONG) == 129
assert CommonPass.LONGEST_ADDRESS != CommonPass.ALMOST_LONGEST_ADDRESS
assert CommonPass.LONGEST_ADDRESS == CommonPass.TOO_LONG_ADDRESS
# TODO: show some UI message when length reaches the limit?
# (it currently disabled typing and greys out the buttons)


class PassphraseCategory(Enum):
MENU = "MENU"
Expand Down
40 changes: 7 additions & 33 deletions tests/click_tests/test_passphrase_bolt.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,24 +45,6 @@
TT_CATEGORY = PassphraseCategory.LOWERCASE
TT_COORDS_PREV: buttons.Coords = (0, 0)

# Testing the maximum length is really 50
# TODO: show some UI message when length reaches 50?
# (it currently disabled typing and greys out the buttons)

DA_50 = 25 * "da"
DA_50_ADDRESS = "mg5L2i8HZKUvceK1sfmGHhE4gichFSsdvm"
assert len(DA_50) == 50

DA_49 = DA_50[:-1]
DA_49_ADDRESS = "mxrB75ydMS3ZzqmYKK28fj4bNMEx7dDw6e"
assert len(DA_49) == 49
assert DA_49_ADDRESS != DA_50_ADDRESS

DA_51 = DA_50 + "d"
DA_51_ADDRESS = DA_50_ADDRESS
assert len(DA_51) == 51
assert DA_51_ADDRESS == DA_50_ADDRESS


@contextmanager
def prepare_passphrase_dialogue(
Expand Down Expand Up @@ -151,16 +133,7 @@ def delete_char(debug: "DebugLink") -> None:
debug.click(coords)


VECTORS = ( # passphrase, address
(CommonPass.SHORT, CommonPass.SHORT_ADDRESS),
(CommonPass.WITH_SPACE, CommonPass.WITH_SPACE_ADDRESS),
(CommonPass.RANDOM_25, CommonPass.RANDOM_25_ADDRESS),
(DA_49, DA_49_ADDRESS),
(DA_50, DA_50_ADDRESS),
)


@pytest.mark.parametrize("passphrase, address", VECTORS)
@pytest.mark.parametrize("passphrase, address", CommonPass.VECTORS)
@pytest.mark.setup_client(passphrase=True)
def test_passphrase_input(
device_handler: "BackgroundDeviceHandler", passphrase: str, address: str
Expand All @@ -171,10 +144,10 @@ def test_passphrase_input(


@pytest.mark.setup_client(passphrase=True)
def test_passphrase_input_over_50_chars(device_handler: "BackgroundDeviceHandler"):
with prepare_passphrase_dialogue(device_handler, DA_51_ADDRESS) as debug: # type: ignore
input_passphrase(debug, DA_51, check=False)
assert debug.read_layout().passphrase() == DA_50
def test_passphrase_input_too_long(device_handler: "BackgroundDeviceHandler"):
with prepare_passphrase_dialogue(device_handler, CommonPass.TOO_LONG_ADDRESS) as debug: # type: ignore
input_passphrase(debug, CommonPass.TOO_LONG, check=False)
assert debug.read_layout().passphrase() == CommonPass.LONGEST
enter_passphrase(debug)


Expand Down Expand Up @@ -293,6 +266,7 @@ def test_cycle_through_last_character(
# Checks that we can cycle through the last (50th) passphrase character
# (was a bug previously)
with prepare_passphrase_dialogue(device_handler) as debug:
passphrase = DA_49 + "i" # for i we need to cycle through "ghi" three times
# for "i" we need to cycle through "ghi" three times
passphrase = CommonPass.ALMOST_LONGEST + "i"
input_passphrase(debug, passphrase)
enter_passphrase(debug)
27 changes: 9 additions & 18 deletions tests/click_tests/test_passphrase_caesar.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,16 +186,7 @@ def cancel(debug: "DebugLink") -> None:
delete_char(debug)


VECTORS = ( # passphrase, address
(CommonPass.SHORT, CommonPass.SHORT_ADDRESS),
(CommonPass.WITH_SPACE, CommonPass.WITH_SPACE_ADDRESS),
(CommonPass.RANDOM_25, CommonPass.RANDOM_25_ADDRESS),
(AAA_49, AAA_49_ADDRESS),
(AAA_50, AAA_50_ADDRESS),
)


@pytest.mark.parametrize("passphrase, address", VECTORS)
@pytest.mark.parametrize("passphrase, address", CommonPass.VECTORS)
@pytest.mark.setup_client(passphrase=True)
def test_passphrase_input(
device_handler: "BackgroundDeviceHandler", passphrase: str, address: str
Expand All @@ -207,22 +198,22 @@ def test_passphrase_input(


@pytest.mark.setup_client(passphrase=True)
def test_passphrase_input_over_50_chars(device_handler: "BackgroundDeviceHandler"):
with prepare_passphrase_dialogue(device_handler, AAA_51_ADDRESS) as debug: # type: ignore
# First 50 chars
input_passphrase(debug, AAA_51[:-1])
def test_passphrase_input_too_long(device_handler: "BackgroundDeviceHandler"):
with prepare_passphrase_dialogue(device_handler, CommonPass.TOO_LONG_ADDRESS) as debug: # type: ignore
# First 128 chars
input_passphrase(debug, CommonPass.TOO_LONG[:-1])
layout = debug.read_layout()
assert AAA_51[:-1] in layout.passphrase()
assert CommonPass.TOO_LONG[:-1] in layout.passphrase()

show_passphrase(debug)

# Over-limit character
press_char(debug, AAA_51[-1])
press_char(debug, CommonPass.TOO_LONG[-1])

# No change
layout = debug.read_layout()
assert AAA_51[:-1] in layout.passphrase()
assert AAA_51 not in layout.passphrase()
assert CommonPass.TOO_LONG[:-1] in layout.passphrase()
assert CommonPass.TOO_LONG not in layout.passphrase()

show_passphrase(debug)
enter_passphrase(debug)
Expand Down
Loading

0 comments on commit dd92be9

Please sign in to comment.