216474 - Browser Crashes in Password Manager

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

I visit a particular site, and as the password manager attempts to complete my
login information the browser crashes in:

nsPasswordManager::OnStateChange

at:       passField->SetValue(buffer);

('passField' is null...) Adding a null check for passField caused it to crash a
couple of lines further up ('nameField' is null...)

It seems that it is possible to get into a state where you exit the loop over
all the SignonDataEntries and 'firstMatch' is NON-NULL and yet 'nameField' and
'passField' are null. 

Here's why I think this is happening - 

For this site my password file has THREE entries ... not exactly sure why,
possibly related to the old password manager. The loop in OnStateChange iterates
through each entry... the first entry is invalid - the username field is
identified by a 'name' attribute that does not exist in the document, so
userField is null the loop continues to the next iteration. The next entry has
valid username and password form names and so firstMatch is set to the entry,
and the loop continues. 

The third time through, the final SignonDataEntry contains bad data, i.e.
strings that don't match the name attribute of any form element in the current
form. As such, 'userField' is null and the loop continues, but since I only have
three entries for this site, it stops iterating at this point and continues past
the loop. 

Note now that 'firstMatch' was set when we found the correct password entry
during the SECOND iteration, but that 'nameField' was set to null during the
THIRD iteration when we failed to find a form element for the name field
specified in that entry. Since 'firstMatch' is non-null, we enter the if { }
block after the loop and subsequently crash when we try to dereference the null
pointer that is nameField. 

My first reaction was to put a null check for nameField and passField in the if
block condition, but I realized this would only fix the crash, and would result
in the fields not being autocompleted because of the third, bad data entry
interfering with the second, good one. 

So I'm proposing this fix...

Change this pattern:

form->ResolveName(e->userField, getter_AddRefs(foundNode));
userField = do_QueryInterface(foundNode);
if (!userField)
  continue;

to this one:

form->ResolveName(e->userField, getter_AddRefs(foundNode));
if (foundNode)
  userField = do_QueryInterface(foundNode);
else
  continue;

where the userField variable is only updated if this data entry is legitimate. 

A patch is attached.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Patch to fix bug

This patch avoids the crash and makes sure login information from bad
SignonDataEntries is ignored.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Need this for .7 ;-) 

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Better patch.

bryner points out that the previous patch will fail if foundNode is non-null
but won't QI to an input element. This patch does the QI and returns to a temp,
then only assigns the temp to the final variable if the QI succeeded (temp
non-null)

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

-> me

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

fixed.