Closed Bug 216474 Opened 21 years ago Closed 21 years ago

Browser Crashes in Password Manager

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firebird0.7

People

(Reporter: bugs, Assigned: bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch to fix bug (obsolete) — Splinter Review
This patch avoids the crash and makes sure login information from bad
SignonDataEntries is ignored.
Need this for .7 ;-) 
Priority: -- → P1
Target Milestone: --- → Firebird0.7
Attached patch Better patch. Splinter Review
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)
Attachment #129947 - Attachment is obsolete: true
Attachment #129949 - Flags: review+
-> me
Assignee: bryner → bugs
fixed. 
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: