1646610 - Consider matching base domains (eTLD+1) instead of origin for formActionOrigin

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Bianca Danforth [:bdanforth]]

formActionOrigin (formerly formSubmitURL) was added in Bug 360493, and in Bug 1121119, we considered ignoring it, but ultimately decided to keep it as a precaution against a "very narrow attack scenario" given that we autofill forms without user interaction.

The telemetry that was added to support ignoring formActionOrigin in Bug 1124888 indicated that 17% of the time, we do not autofill solely due to a difference in formActionOrigin value, with plausible reasons for why this might occur.

While we may not want to ignore formActionOrigin, we may help to lower this 17% by considering the base domain (eTLD+1) instead of the origin. This would allow us to autofill on subdomains for a site, which we hypothesize is a common situation. Are there any reasons why this is not a good idea?

Comment 1

[Daniel Veditz [:dveditz]]

given that we autofill forms without user interaction.

which is, of course, password manager's original sin. I do not understand out complete rejection of addressing that point.

The biggest risk for autofill w/out interaction is when an attacker can inject script, and XSS is still one of the most common website vulnerabilities even at companies with the skills and resources of Google (we've had presentations at W3C conferences).

Using the (eTLD+1) instead of full domain in a secure context (e.g. https: page) won't make us much less secure. Will it solve your problem, though? How common are the cases ckarlof mentions in bug 1121119 comment 12? If most of that 17% turns out to be javascript: urls or no form action at all then this change won't help much at all.

Comment 2

[Matthew N. [:MattN]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

given that we autofill forms without user interaction.

which is, of course, password manager's original sin. I do not understand out complete rejection of addressing that point.

There isn't a complete rejection of this… that's why we added the user-visible pref in about:preferences#privacy-logins last year. I've commented somewhere else before that we want to get the password manager working well before hindering the UX by not autofilling. In order to have a decent UX comparable with other browsers it's not as simple of changing the default value of this pref… Chrome still autofills AFAICT and Safari only stopped autofilling when they added auto-submission of login forms. Implementing the latter is quite difficult (adding even more heuristics to password manager which is complicated enough) and error-prone making it not a priority at the moment.

Using the (eTLD+1) instead of full domain in a secure context (e.g. https: page) won't make us much less secure. Will it solve your problem, though?

Anecdotally I think it will help a large proportion of the 17% as I've seen subdomains changing between the following common subdomains: 'www', 'secure', 'login', & 'm'.

How common are the cases ckarlof mentions in bug 1121119 comment 12? If most of that 17% turns out to be javascript: urls or no form action at all then this change won't help much at all.

Only 0.24% of my 800+ saved logins have a formActionOrigin of javascript: and anecdotally it doesn't seem to be a common pattern nowadays. "No form action" means we use the document origin and therefore could still be helped by this change.

After landing this change we should be able to approximate the effect by looking for a decrease in AUTOFILL_RESULT.NO_SAVED_LOGINS telemetry as it's fairly stable. If you aren't objecting to this change then I'd like to try it, with a pref to revert if necessary.

Comment 3

[Matthew N. [:MattN]]

To implement this improvement:

• change acceptDifferentSubdomains to true at https://searchfox.org/mozilla-central/rev/cfaa250d14e344834932de4c2eed0061701654da/toolkit/components/passwordmgr/LoginManagerChild.jsm#1985,1998
• updating tests that fail:

./mach test toolkit/components/passwordmgr/

• Duplicate the task at https://searchfox.org/mozilla-central/rev/cfaa250d14e344834932de4c2eed0061701654da/toolkit/components/passwordmgr/test/mochitest/test_autofill_different_formActionOrigin.html#59-69 to cover this case e.g. change "https://example.org" to "https://sub.example.com" and maybe duplicate it again reversing the first two arguments to nsLoginInfo

Comment 4

[Tim Giles [:tgiles]]

Attachment: Bug 1646610 - Match base domains (eTLD+1) instead of origin for formActionOrigin. r=MattN

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cb8f7c2eee3
Match base domains (eTLD+1) instead of origin for formActionOrigin. r=MattN

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8cb8f7c2eee3

Comment 7

[Tim Giles [:tgiles]]

Attachment: bug-1646610-script.js

Steps to reproduce:

1. Enable the devtools.chrome.enabled pref in about:config
2. Open the "Browser Console"

• Ctrl+Shift+J on Windows
• Cmd+Shift+J on Mac

1. Run attached bug-1646610-script.js in the Browser Toolbox
2. Navigate to https://twitter.com
3. Verify test credentials have been auto filled

• TestUser for username
• TestPassword for password

Comment 8

[Timea Cernea [:tbabos]]

Verified-fixed on latest Nightly 81.0a1 (2020-08-20) (64-bit) on Windows 10 x64 and MacOS 10.13 based on the steps provided in Comment 7.
Waiting for potential Uplift to Fx80.

Comment 9

[Matthew N. [:MattN]]

(In reply to Timea Cernea [:tbabos] from comment #8)

Waiting for potential Uplift to Fx80.

This is new behaviour that I don't think we should uplift.

Comment 10

[Timea Cernea [:tbabos]]

Thanks Matt, marking this as verified-fixed and removing qe+ flag.