1185000 - Password manager should never offer to save credit card numbers

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[François Marier [:francois]]

After typing my credit card number and CCV on a payment gateway, the Firefox password manager asked me whether I wanted to save username 45..... and password ***.

I believe we have the ability to detect credit card numbers in the autocomplete code. We should use it to ensure we never prompt to save passwords in the password manager.

Nigthtly 42.0a1 (2015-07-16)

Comment 2

[Matthew N. [:MattN]]

There are banks / credit card companies that use the card number (PAN) as the username for login e.g. Bank of Montreal (BMO) so this would break those legitimate saves…[1]

I agree we should never try to save a CC-number as a password though… that has affected United.com before IIRC.

[1] https://www1.bmo.com/onlinebanking/cgi-bin/netbnx/NBmain?product=5

Comment 4

[Matthew N. [:MattN]]

Another heuristic to use is if the password is 3-4 digits which is the length of a CVV/CSC value.

So I think there are two cases where we shouldn't save:

1. If the Luhn checksum matches on the username field (see CreditCard.jsm) AND the password is 3-4 numerical digits (N.B. lookup actual CVV/CSC formats)
2. If the Luhn checksum matches on the password value AND we detect that the type=password field is a credit card field via autocomplete=cc-number or looking at the field label.
   ** We must include the @autocomplete/label check otherwise sites will abuse this loophole on legit login forms and set autocomplete=cc-number on their password fields to avoid saving.

For both of these cases we could also consider a dismissed:true doorhanger, rather than not showing one at all, in case there are false-negatives.

Comment 5

[:prathiksha]

Attachment: Bug 1185000 - Password manager should not offer to save credit card numbers. r?jaws

Password manager should not offer to save credit card numbers in certain straight-forward cases.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b36ee12b7088
Password manager should not offer to save credit card numbers. r=jaws

Comment 7

[Cristina Coroiu [:ccoroiu]]

Backed out changeset b36ee12b7088 (bug 1185000) for browser-chrome failures at toolkit/components/passwordmgr/test/browser/browser_notifications_2.js

Backout: https://hg.mozilla.org/integration/autoland/rev/2c4c3f7a6e7b1a4c1465295a501f1d86168a5f12

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=240116590&revision=b36ee12b7088d76fbb7596026b20f1d4c80a66c1

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240071039&repo=autoland&lineNumber=7640

00:08:04 INFO - TEST-START | toolkit/components/passwordmgr/test/browser/browser_notifications_2.js
00:08:05 INFO - GECKO(1252) | console.warn: nsLoginManager: "searchLogins: formSubmitURL or httpRealm is recommended"
00:08:05 INFO - GECKO(1252) | console.warn: nsLoginManager: "searchLogins: formSubmitURL or httpRealm is recommended"
00:08:05 INFO - TEST-INFO | started process screencapture
00:08:05 INFO - TEST-INFO | screencapture: exit 0
00:08:05 INFO - Buffered messages logged at 00:08:04
00:08:05 INFO - Entering test bound common_initialize
00:08:05 INFO - Leaving test bound common_initialize
00:08:05 INFO - Entering test bound setup
00:08:05 INFO - Leaving test bound setup
00:08:05 INFO - Entering test bound test_empty_password
00:08:05 INFO - Buffered messages finished
00:08:05 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications_2.js | Main action button is disabled - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js :: test_empty_password/< :: line 41
00:08:05 INFO - Stack trace:
00:08:05 INFO - chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js:test_empty_password/<:41
00:08:05 INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:111
00:08:05 INFO - chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js:test_empty_password:14
00:08:05 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
00:08:05 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
00:08:05 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005
00:08:05 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
00:08:05 INFO - Leaving test bound test_empty_password
00:08:05 INFO - Entering test bound test_toggle_password
00:08:05 INFO - GECKO(1252) | console.warn: nsLoginManager: "searchLogins: formSubmitURL or httpRealm is recommended"
00:08:05 INFO - GECKO(1252) | console.warn: nsLoginManager: "searchLogins: formSubmitURL or httpRealm is recommended"

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9be442c871e
Show a dismissed password manager doorhanger when credit card numbers are detected. r=jaws

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e9be442c871e

Comment 10

[Andrei Ciure[:aciure]]

Backed out changeset 33f6d42d7fa9 (bug 1543449) for turning 1272849 into perma

push that casued the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&searchStr=browser-chrome&revision=e9be442c871e173a409f3b969f5bcea0e1ae4d71

failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&searchStr=os%2Cx%2C10.10%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx64-shippable%2Fopt-mochitest-browser-chrome-e10s-3%2Cm%28bc3%29&revision=e9be442c871e173a409f3b969f5bcea0e1ae4d71&selectedJob=241611983

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7d2b9798c226df217f96a5d9a9862798647792

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42af01d53e4e
Backed out changeset e9be442c871e for turning 1272849 into perma CLOSED TREE

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/42af01d53e4e

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8541439e101
Show a dismissed password manager doorhanger when credit card numbers are detected. r=jaws

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f8541439e101

Comment 15

[Daniel Bodea [:danibodea]]

I have tested 2 sites:

1. united.com: I have attempted to perform a fake payment on the affected version and the 2 digit security code (CVV) would be saved by the password manager as a password, while this did not happen when using the latest Nightly build.

2. https://www1.bmo.com/onlinebanking/cgi-bin/netbnx/NBmain?product=5: in which case, the card number is not saved on the affected version, nor on the fixed version. Here, where the card number seems to be treated as a username, it should probably be saved.

Can you help me with a test case or several test cases that could help me verify it correctly? Or ideas on how to do it? Thanks!

Comment 16

[Matthew N. [:MattN]]

(In reply to Bodea Daniel [:danibodea] from comment #15)

I have tested 2 sites:

1. united.com: I have attempted to perform a fake payment on the affected version and the 2 digit security code (CVV) would be saved by the password manager as a password, while this did not happen when using the latest Nightly build.

That's good news.

2. https://www1.bmo.com/onlinebanking/cgi-bin/netbnx/NBmain?product=5: in which case, the card number is not saved on the affected version, nor on the fixed version. Here, where the card number seems to be treated as a username, it should probably be saved.

Thanks I filed bug 1546749 for this.

Can you help me with a test case or several test cases that could help me verify it correctly? Or ideas on how to do it?

Did you try the example from bug 1187695? You should be able to find other sites which have the patterns mentioned in the User Story… you can also find other sites that have credit card fields and edit the <input> in the inspector to convert the type attribute to type=password to make them fit the conditions of this bug. Note that for case 1 in the User Story, the card number field must be the first eligible field that we consider a username before the type=password CVV one.

Let me know if you need more assistance finding testcases.

Comment 17

[Daniel Bodea [:danibodea]]

I have also tested the scenario from this bug's duplicate, bug 1187695, but the scenario could not be reproduced so I asked the reporter about its reproduction.
Furthermore, I have also tested the scenario from its duplicate's duplicate, bug 1444235 and the scenario was reproduced in Beta and fixed in the latest Nightly.

I have also tested 2 random scenarios with fake payments and the instructions in the previous comment and the issue seems to be fixed. Thank you.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

It's late in the 67 cycle for uplift so I'm wontfixing. If you disagree please let Pascal or me know.