Crash storing password on paypal.com
Epiphany crashed when clicking on the info bar button to save my password on paypal.com. Relevant fields from the backtrace:
#3 0x00007f3d6acfc00a in g_assertion_message_expr (domain=domain@entry=0x0,
file=file@entry=0x7f3d6a941878 "../lib/sync/ephy-password-manager.c",
line=line@entry=476,
func=func@entry=0x7f3d6a941a20 <__func__.32285> "ephy_password_manager_save", expr=expr@entry=0x7f3d6a941805 "!username_field || username")
at ../glib/gtestutils.c:2644
s = 0x55e47871cfc0 "assertion failed: (!username_field || username)"
#4 0x00007f3d6a93495a in ephy_password_manager_save (self=0x55e4777c7ea0,
origin=0x55e4799c4850 "https://www.paypal.com",
target_origin=0x55e47784e980 "https://www.paypal.com", username=0x0,
password=0x55e477845370 "<censored for obvious reasons>",
username_field=0x55e47867cbd0 "captchaCode",
password_field=0x55e4799e0bc0 "login_password", is_new=1)
at ../lib/sync/ephy-password-manager.c:476
record = <optimized out>
uuid = <optimized out>
id = <optimized out>
timestamp = <optimized out>
__func__ = "ephy_password_manager_save"
_g_boolean_var_ = <optimized out>
So Ephy.FormManager._findFormAuthElements
guessed the username_field
wrong and came up with a NULL username
. That's not great, but OK. But it's illegal use of ephy_password_manager_save()
, which requires username
if username_field
is provided.
I see several problems:
- The assertions don't make sense. Either both
username_field
andusername
should be provided, or neither should be. And bothpassword_field
andpassword
should always be required. - Probably
Ephy.FormManager.handleFormSubmission
should abort saving the data if the above is not satisfied. - The UI process, in
web_extension_password_manager_save_real()
, blindly trust input from the web process and passes it intoephy_password_manager_save()
, which has the asserts. But the web process is untrusted and must not be able to crash the UI process. So we need sanitization at the point the data is received from the web process,web_extension_password_manager_save_real()
. - There is some extra code asserting that nothing unexpected is stored in the keyring. I don't think it's OK to assert on this either. Let's reserve assertions for Epiphany bugs only, not input.
CC @pgriffis @carlosgc since you both rewrote this code recently.