Closed Bug 1038215 Opened 10 years ago Closed 10 years ago

Session Restore storing CC info, circumventing autofill restrictions.

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 34

People

(Reporter: mhoye, Assigned: tyler.colgan, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

Despite bug 188285 I can still see credit card numbers getting autofilled in Firefox.

Here's a snippet of a sessionstore-backups/recovery.bak file where it's all coming from, but from a user's perspective it appears as regular autofill pulldown.

formdata":{"xpath":{
"//xhtml:div[@id='main']/xhtml:form[@name='ActionForm']/xhtml:table/xhtml:tbody/xhtml:tr[3]/xhtml:td[2]/xhtml:select[@name='paymentMethodCategory']":{"selectedIndex":1,"value":"CC"}, 
"//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr/xhtml:td[2]/xhtml:select[@name='paymentMethod.type']":{"selectedIndex":0,
"value":""}, 
"//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr[2]/xhtml:td[2]/xhtml:input[@name='paymentMethod.accountNumber']":"################",
"//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr[4]/xhtml:td[2]/xhtml:select[@name='paymentMethod.creditCardExpiryDat
e.month']":{"selectedIndex":0,"value":""},
"//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr[4]/xhtml:td[2]/xhtml:select[@name='paymentMethod.creditCardExpiryDate.year']":{"selectedIndex":0,

[...]
Implementing the Luhn checksum algorithm from bug 188285 and applying that only to <input> fields shouldn't be too hard. FormData.collect() in FormData.jsm would need to do that in order to prevent us from saving CC numbers.
Mentor: ttaubert
Whiteboard: [good-first-bug][lang=js]
What would be the best way to detect <input> fields in FormData.collect() and exclude them from being collected if they contain CC numbers? Something like:

while (node = formNodes.iterateNext()) {
  let hasDefaultValue = true;
  let value;

  ...

  // We do not want to collect credit card numbers. See bug 1038215.
  if (node instanceof Ci.nsIDOMHTMLInputElement &&
      IsValidCCNumber(node.value)) {
    continue;
  }

  ...

}

(This is assuming that a working version of IsValidCCNumber() would be established.)
Yeah, that looks great to me. That's exactly how we should do it.
Here's a patch that adds this check along with an implementation of the Luhn algorithm.

Would it be a good idea to add some automated test cases for this as well? (cf. test cases added in this diff: https://bugzilla.mozilla.org/attachment.cgi?id=433049&action=diff) If so, could you point me to some information about how to run the pre-existing tests for this area and how to add new test cases?
Attachment #8463135 - Flags: feedback?(ttaubert)
Comment on attachment 8463135 [details] [diff] [review]
Change FormData.collect() to avoid storing CC data (version 1)

Review of attachment 8463135 [details] [diff] [review]:
-----------------------------------------------------------------

Tyler, thank you, this looks great!

You're right, this should have tests. You can take a look at browser_formdata.js [1] and add another task/test at the bottom there. I hope this is straight forward looking at the existing code but if there are any questions feel free to ask here or on IRC! Thanks!

[1] https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_formdata.js

::: toolkit/modules/sessionstore/FormData.jsm
@@ +42,5 @@
>  
>  /**
> + * Returns whether the given value is a valid credit card number based on
> + * the Luhn algorithm. See https://en.wikipedia.org/wiki/Luhn_algorithm and
> + * bug 1038215.

Nit: I don't think we need a reference to the bug number here, it's all in Git/Hg for us :)

@@ +68,5 @@
> +    if (i % 2) {
> +      // Double every other value.
> +      total += currentDigit * 2;
> +      // If the doubled value will be >10, add the digits together.
> +      if(currentDigit > 4) {

Tiny nit: if () {

@@ +149,5 @@
>        if (!node.id && generatedCount > MAX_TRAVERSED_XPATHS) {
>          continue;
>        }
>  
> +      // We do not want to collect credit card numbers. See bug 1038215.

Nit: I don't think we need a reference to the bug number here either.
Attachment #8463135 - Flags: feedback?(ttaubert) → feedback+
Thanks for the info, Tim. Here's another iteration of the patch, which includes your recommendations for FormData.collect(). This version also adds a test to check that valid CC numbers are not being collected, and that non-valid CC numbers are still being collected. The example credit card numbers used in the test are taken verbatim from the test added in the fix for bug 188285.
Attachment #8463135 - Attachment is obsolete: true
Attachment #8463513 - Flags: feedback?(ttaubert)
Comment on attachment 8463513 [details] [diff] [review]
Change FormData.collect() to avoid storing CC data (version 2)

Review of attachment 8463513 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Tyler, this looks great. I'd like to fix a few nits but otherwise I will very likely r+ your next patch :) Before we land it we should then push it to try (I can do that for you) to check that it doesn't break any of our tests on any of our platforms.

::: browser/components/sessionstore/test/browser_formdata.js
@@ +268,5 @@
> +    "3350852696538147", "5011802870046957"
> +  ];
> +
> +  const URL = "http://mochi.test:8888/browser/browser/components/" +
> +            "sessionstore/test/browser_formdata_sample.html";

Nit: Please indent by two more spaces.

@@ +272,5 @@
> +            "sessionstore/test/browser_formdata_sample.html";
> +
> +  // Creates a tab, loads a page with a form field, sets the value of the
> +  // field, and then removes the tab to trigger data collection.
> +  function createAndRemoveTab(formValue) {

function* createAndRemoveTab(formValue) {

@@ +286,5 @@
> +    gBrowser.removeTab(tab);
> +  }
> +
> +  // Test that valid CC numbers are not collected.
> +  for (let i = 0; i < validCCNumbers.length; i++) {

for (let number of validCCNumbers) {

@@ +289,5 @@
> +  // Test that valid CC numbers are not collected.
> +  for (let i = 0; i < validCCNumbers.length; i++) {
> +    yield createAndRemoveTab(validCCNumbers[i]);
> +    let [{state: {formdata}}] = JSON.parse(ss.getClosedTabData(window));
> +    ok(typeof formdata === 'undefined', "valid CC numbers are not collected");

Does this yell in strict mode? Should we maybe check |"formdata" in state|?

@@ +293,5 @@
> +    ok(typeof formdata === 'undefined', "valid CC numbers are not collected");
> +  }
> +
> +  // Test that non-CC numbers are still collected.
> +  for (let i = 0; i < invalidCCNumbers.length; i++) {

for (let number of invalidCCNumbers) {

@@ +297,5 @@
> +  for (let i = 0; i < invalidCCNumbers.length; i++) {
> +    yield createAndRemoveTab(invalidCCNumbers[i]);
> +    let [{state: {formdata}}] = JSON.parse(ss.getClosedTabData(window));
> +    is(formdata.id.txt, invalidCCNumbers[i],
> +      "numbers that are not valid CC numbers are still collected");

Tiny nit: please indent by one more space.

::: toolkit/modules/sessionstore/FormData.jsm
@@ +66,5 @@
> +
> +    if (i % 2) {
> +      // Double every other value.
> +      total += currentDigit * 2;
> +      // If the doubled value will be >10, add the digits together.

Nit: "will be >=10" or "will be >9"
Attachment #8463513 - Flags: feedback?(ttaubert) → feedback+
Thanks, here's another iteration.
I actually didn't get any complaints in strict mode about using formdata that way, but I think checking |"formdata" in state| is probably better, so I updated it to use that.
Attachment #8463513 - Attachment is obsolete: true
Attachment #8464087 - Flags: review?(ttaubert)
Comment on attachment 8464087 [details] [diff] [review]
Change FormData.collect() to avoid storing CC data (version 3)

Review of attachment 8464087 [details] [diff] [review]:
-----------------------------------------------------------------

That looks great, thank you! I'll push it to try in a second.
Attachment #8464087 - Flags: review?(ttaubert) → review+
Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=2801f6661759
Assignee: nobody → tyler.colgan
Status: NEW → ASSIGNED
Pushed the patch again including a request for a longer test timeout. It failed (i.e. timed out) on debug builds that run a lot slower than our opt builds.

https://tbpl.mozilla.org/?tree=Try&rev=4ce4e84dfdda
Thank you, Tyler! Tim, do we have something else Tyler could sink his teeth into, now that this is landed?
Yes, I thought about that as well!

Tyler, if you want to help us fix a few more sessionstore bugs these are good candidates: bug 1030765, bug 1031298, bug 1036341, and bug 952224.

If you want to take a look at different parts of the browser then that's fine, we have mentored bugs for almost everything and we can certainly help you find something :)
https://hg.mozilla.org/mozilla-central/rev/d751028876fa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
You need to log in before you can comment on or make changes to this bug.