Several Logic Errors in nsPasswordManager::FillDocument

RESOLVED WONTFIX

Status

()

Toolkit
Password Manager
RESOLVED WONTFIX
11 years ago
9 years ago

People

(Reporter: Robert Chapin, Assigned: dveditz)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 253594 [details] [diff] [review]
First Patch Attempt
(Reporter)

Comment 2

11 years ago
Created attachment 253596 [details] [diff] [review]
First Patch Attempt

Needed to delete some lines that showed up because of version mismatch.
Attachment #253594 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #253596 - Flags: review?(mconnor)
(Reporter)

Updated

11 years ago
Blocks: 360493
(Reporter)

Comment 3

11 years ago
Created attachment 253598 [details] [diff] [review]
First Patch Attempt

Fixing some formatting.
Attachment #253596 - Attachment is obsolete: true
Attachment #253598 - Flags: review?(mconnor)
Attachment #253596 - Flags: review?(mconnor)
(Reporter)

Updated

11 years ago
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.2?
(Reporter)

Comment 4

11 years ago
Created attachment 253599 [details]
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
(Reporter)

Updated

11 years ago
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Robert, rather just posting a diff, care to explain what changes you want to make?
(Reporter)

Comment 6

11 years ago
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?
(Reporter)

Comment 8

11 years ago
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...
(Reporter)

Updated

11 years ago
Depends on: 362576

Updated

11 years ago
Flags: blocking1.8.1.2?
(Reporter)

Updated

11 years ago
Depends on: 372885
(Reporter)

Comment 10

11 years ago
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) {
(Reporter)

Comment 11

11 years ago
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.
(Reporter)

Comment 12

11 years ago
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?
(Reporter)

Updated

11 years ago
Blocks: 373140
(Assignee)

Updated

11 years ago
Assignee: nobody → dveditz
Flags: blocking1.8.1.4? → blocking1.8.1.4+
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1.4+ → blocking1.8.1.5?

Updated

11 years ago
Duplicate of this bug: 372885
(Reporter)

Comment 14

11 years ago
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
Last Resolved: 11 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
(Assignee)

Updated

11 years ago
Group: security
Flags: blocking1.8.1.5?
Product: Firefox → Toolkit

Updated

9 years ago
Attachment #253598 - Flags: review?(mconnor)
You need to log in before you can comment on or make changes to this bug.