Comment 6 for bug 77859

I managed to find the source to firefox-1.5.dfsg+1.5.0.8 on a mirror (http://mirror.xmu.edu.cn/archive.ubuntu.com/ubuntu/pool/main/f/firefox/; it's already expired out of security.ubuntu.com and archive.ubuntu.com).

Diff between firefox-1.5.dfsg+1.5.0.8 and firefox-1.5.dfsg+1.5.0.9 reveals that a guard on userField being non-null was removed between those two versions, viz:

-=- cut here -=-
@@ -941,20 +945,20 @@
     }

     if (firstMatch && !attachedToInput) {
- nsAutoString buffer;
-
- if (userField) {
+ AttachToInput(userField);
+
+ if (prefillForm) {
+ nsAutoString buffer;
         if (NS_FAILED(DecryptData(firstMatch->userValue, buffer)))
           goto done;
-=- cut here -=-

(Full diff of that file below.)

So since it appears to be fatal to call AttachToInput(NULL), it appears that the function has been "deliberately" changed to cause Firefox to crash when faced with a presaved form which has no username field. This seems to be undesirable. At very worst it should refuse to fill in the form and continue running; ideally it would continue the previous Firefox behaviour and fill in the form without fuss.

There's nothing in the comments or changelog to explain why the guard on userField being non-null was removed. The entire changelog for .8 to .9 is:

-=- cut here -=-
firefox (1.5.dfsg+1.5.0.9-0ubuntu0.6.06) dapper-security; urgency=low

  * New upstream security update:
    - CVE-2006-6504, MFSA 2006-73: SVG Processing Remote Code Execution.
    - CVE-2006-6503, MFSA 2006-72: XSS by setting img.src to javascript: URI.
    - CVE-2006-6502, MFSA 2006-71: LiveConnect crash finalizing JS objects.
    - CVE-2006-6501, MFSA 2006-70: Privilege escallation using watch point.
    - CVE-2006-6497, CVE-2006-6498, CVE-2006-6499, MFSA 2006-68: Crashes
      with evidence of memory corruption.

 -- Kees Cook <email address hidden> Tue, 2 Jan 2007 11:23:28 -0800
-=- cut here -=-

None of the CVE or MFSA documents referenced talk about the password manager or saved password exploits or appear to explain why this code was changed. Given other security discussion I'm aware of I assume it's part of an attempt to avoid the recently publicised password stealing attack through user-created forms being pre-filled.

The list appears to come from here:

http://www.mozilla.org/projects/security/known-vulnerabilities.html#firefox1.5.0.9

which is referenced from here:

http://www.mozilla.com/en-US/firefox/releases/1.5.0.9.html

which is the upstream announcement of 1.5.0.9.

Most of the change in nsPasswordManager.cpp appears to have come from upstream (comparing diffs with and without the ubuntu dapper patches). However upstream 1.5.0.8 upstream also didn't have the guard on userField being non-NULL. That guard was being added by the ubuntu dapper patch in 1.5.0.8, but is not being added in 1.5.0.9.

In particular we seem to have lost this patch:

-=- cut here -=-
* toolkit/components/passwordmgr/base/nsPasswordManager.cpp: Take patch
   from bz#235336 as suggested by Ian Jackson to allow password manager
   to work with sites that only have a password field, no username.
 -- Eric Dorland <email address hidden> Mon, 6 Feb 2006 23:10:29 -0500
-=- cut here -=-

I'm not sure what "bz#235336" is or where it can be found, but I strongly suspect that it includes the guard on userField being non-null.

So it appears that a combination of an upstream change and dropping a patch that allowed password manager to work with password-only forms has caused
this crash.

It also appears to me that upstream will have this bug (crash with saved passwords on forms with only a password field) if I'm reading their code correctly.

Any chance we can have this "bz#235336" patch reapplied and/or the guard
on userField back again, so password manager works with password only forms and firefox doesn't crash?

Ewen

-=- full 1.5.0.8 to 1.5.0.9 diff for nsPasswordManager.cpp -=-
ewen@wat:/var/tmp/ubuntu-dapper-firefox$ diff -u firefox-1.5.dfsg+1.5.0.{8,9}/toolkit/components/passwordmgr/base/nsPasswordManager.cpp
--- firefox-1.5.dfsg+1.5.0.8/toolkit/components/passwordmgr/base/nsPasswordManager.cpp 2007-01-06 13:45:37.000000000 +1300
+++ firefox-1.5.dfsg+1.5.0.9/toolkit/components/passwordmgr/base/nsPasswordManager.cpp 2007-01-06 12:53:22.000000000 +1300
@@ -815,6 +815,10 @@

   PRUint32 formCount;
   forms->GetLength(&formCount);
+
+ // check to see if we should formfill. failure is non-fatal
+ PRBool prefillForm = PR_TRUE;
+ mPrefBranch->GetBoolPref("prefillForms", &prefillForm);

   // We can auto-prefill the username and password if there is only
   // one stored login that matches the username and password field names
@@ -907,7 +911,7 @@
         continue;
       }

- if (!oldUserValue.IsEmpty()) {
+ if (!oldUserValue.IsEmpty() && prefillForm) {
         // The page has prefilled a username.
         // If it matches any of our saved usernames, prefill the password
         // for that username. If there are multiple saved usernames,
@@ -941,20 +945,20 @@
     }

     if (firstMatch && !attachedToInput) {
- nsAutoString buffer;
-
- if (userField) {
+ AttachToInput(userField);
+
+ if (prefillForm) {
+ nsAutoString buffer;
         if (NS_FAILED(DecryptData(firstMatch->userValue, buffer)))
           goto done;

         userField->SetValue(buffer);
- AttachToInput(userField);
- }

- if (NS_FAILED(DecryptData(firstMatch->passValue, buffer)))
- goto done;
+ if (NS_FAILED(DecryptData(firstMatch->passValue, buffer)))
+ goto done;

- passField->SetValue(buffer);
+ passField->SetValue(buffer);
+ }
     }
   }

-=- full 1.5.0.8 to 1.5.0.9 diff for nsPasswordManager.cpp -=-