372885 - PM Saves Invalid Form Signatures

Status: RESOLVED WONTFIX

(Toolkit :: Password Manager, defect)

Description

[Robert Chapin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

There is an "if" statement missing in nsPasswordManager::FillDocument() after if (!(e->passField).IsEmpty()) { ... } else if (userField) {

Reproducible: Always

Steps to Reproduce:
A testcase will be attached.
Actual Results:  
Half of the form signature check is bypassed when Firefox can't find the password field name.

Expected Results:  
Any value other than NULL should represent a mismatch and fail the signature check.

Note although severity is being set to Major, this bug has a direct impact on 368959 and 360493 which are both Critical.  If we have overlooked any potential remote exploit resulting from this bug, then we will recommend upgrading this to Critical as well.
_____________
Robert Chapin
Chapin Information Services, Inc.

Comment 1

[Robert Chapin]

Attachment: First Test Case

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Why do you think this is a flaw? Why is filling in a password for a a password field with a different name a problem? The name of the field is stored as a convenience only, to avoid having to search the document each time.

Comment 3

[Robert Chapin]

Gavin, this bug is interacting with other very serious problems in the password manager.  (e.g. Bug 362576)  "This is a flaw" under the circumstances.

Comment 4

[Robert Chapin]

The other reason, of course, is that the block in question is followed by
if (temp) { ... } else { continue; }
... which causes Firefox to discard any password field whose name does not match the saved value.  This bug is an avenue to circumvent that behavior.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> Gavin, this bug is interacting with other very serious problems in the password
> manager.  (e.g. Bug 362576)  "This is a flaw" under the circumstances.

How is it interacting with that other bug? What is the attack vector here? You seem to be filing bugs about "issues" the code has without tying them to actual user-facing problems, so it's difficult determine what you think should be fixed.

(In reply to comment #4)
> The other reason, of course, is that the block in question is followed by
> if (temp) { ... } else { continue; }
> ... which causes Firefox to discard any password field whose name does not
> match the saved value.  This bug is an avenue to circumvent that behavior.

That behavior is a performance optimization, not a security feature. Why is circumventing it a security problem?

Comment 6

[Robert Chapin]

(In reply to comment #5)
> How is it interacting with that other bug? What is the attack vector here?

> That behavior is a performance optimization, not a security feature. Why is
> circumventing it a security problem?
 
The autocomplete attribute is not a valid HTML mechanism.  The correct way for a webmaster to disable auto form filling is to randomize the name attribute.  Now we have a potential situation where neither work at all.  This is the first example that comes to mind.

Comment 7

[Robert Chapin]

(In reply to comment #5)
> That behavior is a performance optimization, not a security feature. Why is
> circumventing it a security problem?

By the way, that doesn't make sense to me.  The DataEntry iterator is designed to spin in that scenario.  Skipping a single instance of passField->SetValue() is a performance optimization?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> The autocomplete attribute is not a valid HTML mechanism.  The correct way for
> a webmaster to disable auto form filling is to randomize the name attribute. 
> Now we have a potential situation where neither work at all.  This is the
> first example that comes to mind.

It may not be valid HTML, but the autocomplete attribute is the well established way to prevent passwords from being remembered automatically by Firefox. Random names on the form elements is not, and the fact that it might not prevent autocomplete in an extremely contrived scenario (the user had previously saved a password on a form with no name for the password field) is not a security issue.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> (In reply to comment #5)
> > That behavior is a performance optimization, not a security feature. Why is
> > circumventing it a security problem?
> 
> By the way, that doesn't make sense to me.  The DataEntry iterator is designed
> to spin in that scenario.  Skipping a single instance of passField->SetValue()
> is a performance optimization?

No, the performance optimization is that we don't need to iterate over the page's DOM manually to find the password element, if we know its name.

Comment 10

[Daniel Veditz [:dveditz]]

This does not appear to be an exploitable security flaw. If the form is maliciously injected then the exploiter could just as easily use the right field names. And if the exploiter didn't change the action URL then they won't get the data back, and if they did change it then the fix from bug 360493 should save us.

Seems like we could safely un-hide this and treat it as a normal bug.

Comment 11

[Robert Chapin]

(In reply to comment #9)
> No, the performance optimization is that we don't need to iterate over the
> page's DOM manually to find the password element, if we know its name.

Pardon me, but the block in question DOES iterate over the page's DOM manually to find the password element.  What I am reporting is that the "if" statement to avoid doing that is missing!

Comment 12

[Robert Chapin]

(In reply to comment #10)
> This does not appear to be an exploitable security flaw.

I can't agree with that, but I will handle this according to your decision if you choose to make it public.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> What I am reporting is that the "if" statement to avoid doing that is missing!

You never mentioned which "if" statement, so perhaps I misunderstood. Could you post a patch that shows the addition of the missing if statement, or point to a specific line number at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/base/nsPasswordManager.cpp&rev=1.92?

Comment 14

[Robert Chapin]

I can do that later tonight.  The patch would need to commit name changes to signons2.txt, which is not within my expertise.  However, I should be able to make a rough draft.

Comment 15

[Robert Chapin]

Actually, let me take that back.  :)  I would be in over my head tinkering with DOM iterators.

In the version you link, this block is at 1962.  Two tests are needed at 1983 and 1995.  Alternatively, one test anywhere between 2001 through 2012.  If I knew how to simplify this block more I would do it.

If you sincerely want performance, then while we're here let's take a look at why those two iterators don't seem to have stopping conditions.  It looks like they spin through all possible form elements.

As for signons2.txt, I would want to know if it contains any imported passwords with blank field names that need to be flagged and/or updated.

Comment 16

[Daniel Veditz [:dveditz]]

At line 1929 do we really only want to check if ((e->passField).IsEmpty())? That means we could end up with an empty userField matching a userField without a name. That's fine, except why only if there isn't also a password field?

Seems like instead of 
  } else if ((e->passField).IsEmpty()) {
it should be one of
  } else if (!e->userField.IsEmpty() && e->passField.IsEmpty()) {
  } else if (e->userField.IsEmpty() || e->passField.IsEmpty()) {

depending on whether we *want* to find a user field with no name or not.

At line 1919 we should set userfield to null otherwise if we've set it on a previous pass through the loop at line 1966 we could be operating on the wrong field.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/base/nsPasswordManager.cpp&rev=1.92&mark=1919,1929,1966#1910

Comment 17

[Robert Chapin]

(In reply to comment #16)
> At line 1919 we should set userfield to null otherwise if we've set it on a
> previous pass through the loop at line 1966 we could be operating on the wrong
> field.

userField is re-used later at 2067.  This problem is tentatively patched in Bug 368959, so let's discuss it in there instead ;)

Comment 18

[Robert Chapin]

(In reply to comment #16)
> depending on whether we *want* to find a user field with no name or not.

This is an excellent point.  I just remembered Firefox doesn't transmit unnamed form fields.  Not only that, nsPasswordManager::FillPassword() doesn't fill them.

Perhaps the correct solution is to disable saving of credentials for unnamed password fields?  This would prevent e->passField.IsEmpty() from being true unless the credentials were imported from another application.  If they are imported, then signons2.txt needs to be updated the first time the credentials are matched to a live form signature, as is already done for username fields on line 1950.

Comment 19

[Robert Chapin]

Correcting bug title.  This bug still leads to circumventing the signature check, we are just changing the cause.

Comment 20

[Robert Chapin]

That puts us up around line 929 to test the field name at save instead of retrieve.  As with Bug 360493, we should consider possible interference with websites that want to change the form attributes using javascript.

Comment 21

[Mike Connor [:mconnor]]

not going to block at this point given dveditz's comment 10

Comment 22

[Robert Chapin]

(In reply to comment #16)
> That means we could end up with an empty userField matching a userField without
> a name.

Here's another example of bug interaction.  Viewing the test case for Bug #373309 caused a password to fill in for no apparent reason.  I traced the cause back to the test case for this bug!  The lack of a userField match causes the PM to search for a blank passField entry, which I just so happen to have in signons2.txt.  No username gets filled in to the form.  I don't think this is the intended behavior :)

Comment 23

[Mike Connor [:mconnor]]

Right, that's a bug.  What's exploitable about it?

As has already been pointed out, if I'm injecting content I'll just copy the form I'm trying to mimic, meaning that I'll have a valid form signature.

I'm just going to unhide this bug, I fail to see how the signature mismatch constitutes anything resembling a security exploit, since its not like the forms are signed or anything like that.

Comment 24

[Justin Dolske [:Dolske]]

Bug 368959 already covers the ugly code in fillDocument().

Comment 25

[Robert Chapin]

Justin, per Comment 20 the bug is in Notify() and not fillDocument() and Bug 368959 makes no mention of this issue.  Specifically, there is a weak test at

 930                     if (control->GetType() == NS_FORM_INPUT_PASSWORD) {

This leads to passwords being saved in an ambiguous state when a password-type element has no name.  The empty name is used in other parts of nsPasswordManager to flag imported data.

Comment 26

[Justin Dolske [:Dolske]]

Hmm, I was focusing on comment #0, which was referencing ::FillDocument().

In any case, this is WONTFIX. There's no sign of anything exploitable here. Form field names are not a security mechanism. At worst, the password manager might not properly save or fill in a form login -- but without a compelling reason to dive into this tangled mess it's not something I think is justified for branch.

Comment 27

[Robert Chapin]

Justin, I will work on an exploit if that's what it takes to get this bug reopened.  Keep in mind the security lock was removed long ago.

Comment 28

[Justin Dolske [:Dolske]]

Robert, you've been repeatedly asked in this bug (and others, fwiw) by multiple people to explain what you think the problem is and how it's "exploitable". (see comments #2, 5, 23) None of your answers describe any legitimate security problems, and it has been explained why (comments #2, 8, 10, 23, 26).

We take security seriously, so if you have some new piece of information or if there's something we've overlooked because it's unclear, I'll be happy to reassess the bug. But if not, revisiting the same non-issues isn't a productive use of time.