Closed Bug 368959 Opened 17 years ago Closed 17 years ago

Several Logic Errors in nsPasswordManager::FillDocument

Categories

(Toolkit :: Password Manager, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: shinyairplane, Assigned: dveditz)

References

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

Before committing any patches for Bug #360493, several other bugs should be fixed.

I will attempt to upload a diff file, however it will need to be thoroughly reviewed because I have not used CVS before.

Reproducible: Always
Attached patch First Patch Attempt (obsolete) — Splinter Review
Attached patch First Patch Attempt (obsolete) — Splinter Review
Needed to delete some lines that showed up because of version mismatch.
Attachment #253594 - Attachment is obsolete: true
Attachment #253596 - Flags: review?(mconnor)
Fixing some formatting.
Attachment #253596 - Attachment is obsolete: true
Attachment #253598 - Flags: review?(mconnor)
Attachment #253596 - Flags: review?(mconnor)
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.2?
Attached file Source File
In case the diff file doesn't work, this is the original.  It is based on
http://lxr.mozilla.org/mozilla/source/toolkit/components/passwordmgr/base/nsPasswordManager.cpp?raw=1
retrieved 02/01/2007.  I think it's version 1.90
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Robert, rather just posting a diff, care to explain what changes you want to make?
Okay.

#1  The use of PRBool attachedToInput was confused by its dual purpose as a flag and a counter.  As a result, there was an extra reference to AttachToInput().  The confused flow of control also contributed to several potential vectors of attack.  In keeping with the existing style, I added a second flag called onePasswordFound. This will be used to decide if a user/pass should be auto filled instead of attachedToInput, which is not appropriate.  I eliminated the second reference to AttachToInput() and changed the prefilled username case so that it sets the value of firstMatch the same way as all other cases.

#2  The use of userField->SetValue() and passField->SetValue() to auto fill the user/pass left open the possibility of attack because the value of userField and passField could change any number of times between validating the DataEntry and actually filling the form.  This would actually make it possible for a malicious website to extract older sets of credentials from the PM.  Instead, adding firstUserField and firstPassField, then setting their values exactly one time, gives the desired behavior and should be much clearer.

#3  The DataEntry iterator was designed to spin even if FillDocument had nothing left to do.  This is particularly hazardous when the username has been prefilled.  Therefore, adding the term !prefilledUser stops the iterator when it is no longer needed.

#4  Eliminated all references to nsAutoString oldPassValue, because it didn't do anything.

#5  prefilledUser = PR_TRUE was inappropriately set before validating the userField.  Moved this statement down a few lines.

#6  Changed nsAutoString userValue to nsAutoString buffer, which is consistent with the auto fill block.

#7  if (firstMatch && userField && !attachedToInput) was the biggest problem in the existing function.  There is no logical reason to check firstMatch and attachedToInput in the same phrase because it allows for both of the previously mentioned attack vectors to succeed.  It's also confusing as heck to debug.  So, this block has been appropriately separated into two blocks.  There is no behavioral reason I'm aware of for those blocks to depend on each other, in fact, the PM should also be more reliable now.

That about does it.  All other changes support those 7 bugs.
So, all sane stuff, kinda hard to wrap your head around a patch if you don't know what its doing.

Main feedback, initially, is that we don't comment stuff out in general, if its going, just kill the lines.  Makes the patch and the code harder to follow if its not all the real code.  this also seems like major surgery for the branch, but I guess I don't get the real-world impact of the malicious site stealing an old user-pass for itself.  If the site knows the old field names, can't they just put up that form directly and let it autofill?  Or even have multiple forms with the appropriate field names, and then the likelihood of autofilling is greater?
Yes, there are other vectors and this is not a complete overhaul.  I just want to plug the most obvious holes so that the 360493 patch ends up looking "sane" as you put it :)
Robert, are there testcases associated here?  Its hard to fix theoretical bugs, especially on a stable branch.  Its even harder to avoid possibly regressing things as people change out, without some form of reproducible testcase outlining the potential attack...
Depends on: 362576
Flags: blocking1.8.1.2?
Depends on: 372885
Comment on attachment 253599 [details]
Source File

Daniel wants userField = NULL; at the top of this iterator.  I'll check if that's safe with the other changes in this patch.

>    for (SignonDataEntry* e = hashEnt->head; e && !prefilledUser; e = e->next) {
Comment on attachment 253598 [details] [diff] [review]
First Patch Attempt

Looks good to me.  If firstUserField = userField = NULL then it should still be handled correctly by:

>+      if (firstUserField && !prefilledUser) {
>+        if (NS_FAILED(DecryptData(firstMatch->userValue, buffer)))
>           return NS_OK;
> 
>-        passField->SetValue(buffer);
>+        firstUserField->SetValue(buffer);
>       }

Let me know if you want a revised patch.
By the way, temp also needs to be reset, and so it probably wouldn't hurt to do passField as well.  Would there be any performance hit by moving all three declarations from the first iterator into the second iterator?
Blocks: 373140
Assignee: nobody → dveditz
Flags: blocking1.8.1.4? → blocking1.8.1.4+
Flags: blocking1.8.1.4+ → blocking1.8.1.5?
Since an attempt has been made to close Bug 372885, I want to copy one of the auxiliary concerns into this bug.  Searching through FillDocument there is exactly one instance of e->userField.Assign(name); and there are none similarly affecting passField.  This could mean that any legitimate use of empty values for passField isn't followed-up with a write to disk when the non-empty value is found.
This bug will shortly be moot on trunk, as fillDocument is being redesigned and rewritten by bug 380222 (and the JS port has already mitigated some issues).

I'm closing this as INCOMPLETE, based on mconnor's question in comments 7 and 9. But I don't see any obviously severe problems here -- especially which merit a patch to the 2.0 branch -- so this is probably WONTFIX anyway. 
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → INCOMPLETE
Hrm. I guess it's silly to make this INCOMPLETE if it would just be WONTFIX based on what's currently known anyway.

Sorry for the bugspam.
Severity: critical → normal
Resolution: INCOMPLETE → WONTFIX
Group: security
Flags: blocking1.8.1.5?
Product: Firefox → Toolkit
Attachment #253598 - Flags: review?(mconnor)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: