Closed Bug 474846 Opened 11 years ago Closed 11 years ago

Data loss by migration from wallet to login manager

Categories

(Toolkit :: Password Manager, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: h.figge, Assigned: Dolske)

Details

(Keywords: dataloss, fixed1.9.1)

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090122 Mnenhy/0.7.5.20005.h1 SeaMonkey/2.0a3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090122 Mnenhy/0.7.5.20005.h1 SeaMonkey/2.0a3pre

I have found two reasons why i lost some of my login datas by the migration to signons.sqlite and signons3.txt.

1) In my profile i had a number.s from 20090109 and a signons2.txt from 20070825. The new signons3.txt was created using the elder signons2.txt, thus omitting newer entries in the number.s.

This issue could be fixed by deleting the signons2.txt, but there is more.

2) Parsing the file to be transferred to signons3.txt the parser lost track for sometime resulting in some not usable entries. I have made a testcase from my old numbers.txt named 123.s which i will upload.

Clearly i have anonymized the data so no login is possible. But nevertheless the choices can be seen.

Create a new profile, close SM, copy 123.s in this profile, start SM, set signon.SignonFileName to 123.s, start the Password Manager, close SM.

Now look at signons3.txt and compare it with 123.s. The offending part for the parser is the entry for www.lob.de. The following entries for www.lycos.de and bugs.gentoo.org are not usable. But not all hope is lost because the next entry for app.winehq.org and the following ones are OK. *g*

3) There are 5 entries for bugzilla.mozilla.org. Trying to login offers 5 choices, but two of them are passwords. ;)

The Passord Manager shows this also. But using the same profile with an elder SM nightly, e.g. 20090110, and looking there on the Password Manager the data is correct.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Attached file Testcase
Version: unspecified → Trunk
So, you let an existing SM2 profile automatically convert the old data to something it can use, and you're not using the profile migration wizard?

In that case, the bug is in toolkit code, I just wonder if this is form manager data or password/login manager data.

Mark, any idea?
Hmm...

http://www.lob.de
kdnr
~
email
~bmFtZTE=
*passwort
~cGFzc3cx
.

That entry has 3 fields, but the code only expects 2 (1 username, 1 password, in that order), and so the format is invalid and won't be properly parsed.

I didn't realize Wallet did this, although now that I think about it I vaguely remember some ancient WONTFIX bugs about the original Firefox password manager having dropped support for that.

In what cases does Wallet save > 2 fields, or in a different order?

Note sure what the fix here should be... We could just not import from such Wallet files, but that would obviously suck. Reading this file format would require a modified parser, but then we'd have to figure out which fields to keep when there are more than 2. I suspect it wouldn't be clear, and we'd just have to throw those entries away.

Where did Seamonkey/Wallet normally store data? In 1234.s or signons*.txt?
(In reply to comment #2)

> So, you let an existing SM2 profile automatically convert the old data to
> something it can use, and you're not using the profile migration wizard?

>No wizard involved, AFAIK. I build a new SM nearly every day and let it attack my profile, which is many years old. ;)

Surely i have backups.
(In reply to comment #3)

> I didn't realize Wallet did this, although now that I think about it I vaguely
> remember some ancient WONTFIX bugs about the original Firefox password manager
> having dropped support for that.

Well, my profile is really old and has survived many nighlies and many crashes.

> Where did Seamonkey/Wallet normally store data? In 1234.s or signons*.txt?

My SM used an 22261767.s until before some days. There was also a signon2.txt over one year old and therefore not used since that time.

But the migration took this file instead of the newer 22261767.s, which is not so good. ;)
The import first looks at the signon.SignonFileName3 pref, then signon.SignonFileName2, then signon.SignonFileName. Not file timestamps, because that introduces other problems.
Well, if i am not the only one who has an ancient signons*.txt in the profile whereas in reality a numbers.txt ist used, then that would would mean data loss.

I don't recall that i had ever manipulated the filenames, but on the other hand i have had a lot of buggy nightlies. *g*
(In reply to comment #7)
> Well, if i am not the only one who has an ancient signons*.txt in the profile
> whereas in reality a numbers.txt ist used, then that would would mean data
> loss.

You are probably one of the very very few, as you must have it from the few days we once already had loginmanager built and then excluded it again, IIRC. Normal SeaMonkey users only should have a *.s file.
(In reply to comment #8)

> You are probably one of the very very few, as you must have it from the few
> days we once already had loginmanager built and then excluded it again, IIRC.

That may be.

> Normal SeaMonkey users only should have a *.s file.

Mhm. There exist 3 signon.SignonFileName*. One points to numbers.s, the others to signons2.txt and signons3.txt. Looks quite normal to me.

If a normal user only has a numbers.s, why is this file not taken for the migration instead checking for signons*.txt first? At least signons2.txt has not been used for a long time, about signons3.txt i don't know.

If this is true then we can assume that the recent data is contained in the numbers.s, should use this file and ignore the signons*.txt.
SeaMonkey (even SM2) would use a numbers.s file for wallet if it is a profile that has come from Netscape. I think fresh profiles for SM 1.1 at least would use signons.txt.

Regarding sigons2.txt - this was because at one stage about a year and a half ago the toolkit profile manager was enabled for a period of about a week on nightly builds. This caused our nightly users to gain a signons2.txt file. We've already decided not to fix that because it affects only a small amount of users and a fix would be complex.

So the issue we need to deal with here is the 2/3 fields issue.
(In reply to comment #10)

> Regarding sigons2.txt - this was because at one stage about a year and a half
> ago the toolkit profile manager was enabled for a period of about a week on
> nightly builds.

Fine. Then a possible signons2.txt should and must be ignored by the migration.
(In reply to comment #11)
> (In reply to comment #10)
> 
> > Regarding sigons2.txt - this was because at one stage about a year and a half
> > ago the toolkit profile manager was enabled for a period of about a week on
> > nightly builds.
> 
> Fine. Then a possible signons2.txt should and must be ignored by the migration.

No. We will not do that because that we share that code with Firefox and others and removing that upgrade route will potentially break many more Firefox users. Special casing the migration is not an easy option.

The toolkit profile manager was only enabled on nightly builds and so this won't affect end users, and I expect only affects a small number of users.
(In reply to comment #12)
> (In reply to comment #11)

> > Fine. Then a possible signons2.txt should and must be ignored by the migration.
> 
> No. We will not do that because that we share that code with Firefox and others
> and removing that upgrade route will potentially break many more Firefox users.

I am writing about SM and don't know anything about FF. ;)

> Special casing the migration is not an easy option.

OK.

> The toolkit profile manager was only enabled on nightly builds and so this
> won't affect end users, and I expect only affects a small number of users.

And this small number should be able to deal with the issue like myself.
(In reply to comment #0)
> 1) In my profile i had a number.s from 20090109 and a signons2.txt from
> 20070825. The new signons3.txt was created using the elder signons2.txt, thus
> omitting newer entries in the number.s.
> 
> This issue could be fixed by deleting the signons2.txt, but there is more.

Ftr, this part is bug 474161.
Attached patch Patch v.1 (WIP)Splinter Review
So, I took a look at this. I think this is a low-impact fix that should at least help with importing some logins successfully from wallet.

The existing login manager code expects entries in a format like:

-----
url
fieldname (for user)
encoded/encrypted username
*fieldname (for password, 1st char is "*")
encoded/encrypted password
.
-----

If you've saved multiple logins for a site, they're wedged into the same entry (where "entry" means a block starting with a URL and ending with a "."), like:

-----
url
fieldname1
encoded/encrypted username1
*fieldname1
encoded/encrypted password1
fieldname2
encoded/encrypted username2
*fieldname2
encoded/encrypted password3
.
-----

Wallet is similar, except that it can emit >2 field/value pairs for an entry, it may place the password's field/value at any position, and it doesn't seem to place multiple logins in the same entry. [This needs some testing and verification.]

The basic idea of this fix is that when processing an entry, it expects the the 2nd fieldname to begin with "*". If it doesn't, it's fallen out of sync with the expected input (eg, corrupted file or multiple-field wallet entry), and ignores everything up until the next ".". At that point it knows it's reached the end of some entry, and can begin parsing again normally.

This should be safe to do, but there's some risk that it could result in discarding an 2nd entry, which is otherwise valid. Consider:

-----
urlA
fieldA-1
value
*fieldA-2
value
fieldA-3
.
urlB
fieldB-1
value
*fieldB-2
value
.
-----

Basically, the parser isn't expecting an odd number of field/value pairs... It reads fieldA-1 and fieldA-2, and adds them as the user/pass for a login. Then it reads fieldA-3 and its value as a 2nd username, hits the next line and finds a "." instead of a line beginning with a "*" (indicating the expected password field name). So we flip into the discard state, and ignore the following lines until the ".". Which means the login in the entry for urlB is ignored, even though we theoretically could have extracted it.

I'm inclined to stay with this safe-and-simple approach, even though it's not perfect... It could be improved at the cost of more complexity and risk, but some dataloss is going to be unavoidable no matter what -- there's no way to map entries with more than one username or password into what login manager supports.

At the very least, we might want to take this fix for now, and consider a further improvement for later...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Data loss by migration to signons.sqlite → Data loss by migration from wallet to login manager
Flags: blocking-seamonkey2?
Keywords: dataloss
Not sure if this is the same bug. Please let me know, otherwise I will file a new bug.

I just migrated my SM1 profile. I had passwords saved for homebanking at https://www.cortalconsors.de/euroWebDe/- and https://ebanking.easybank.at/InternetBanking/EASYBANK_webbank_de.html. I only had the login saved and password deleted. Now both are gone! The former is lost altogether and the second replace by a wrong entry. I consider this serious dataloss.

pi
Comment on attachment 362347 [details] [diff] [review]
Patch v.1 (WIP)

So... I think we should go ahead and take this patch. It shouldn't regress Firefox (which has no wallet import, and has tests for reading signons.txt). I think we should mitigate any potential risk by getting this in for FF3.5 Beta 4 to ensure real-world testing, just to be sure.

I'd still very appreciate if someone could write some testcases for importing from wallet files, too ensure we don't regress reading those formats in the future. But I think this patch can advance regardless (since we're no worse off than we are today).
Attachment #362347 - Flags: review?(gavin.sharp)
Attachment #362347 - Flags: review?(gavin.sharp) → review+
Attachment #362347 - Flags: approval1.9.1?
Pushed http://hg.mozilla.org/mozilla-central/rev/68fa45d1a039

There are still cases where we can't import data from wallet, but I think those are basically unavoidable due to the difference in how login manager and wallet work. So, calling this fixed.
Assignee: nobody → dolske
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Attachment #362347 - Flags: approval1.9.1? → approval1.9.1+
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/346818ecae9a
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Component: Passwords & Permissions → Password Manager
Flags: blocking-seamonkey2?
Product: SeaMonkey → Toolkit
QA Contact: privacy → password.manager
You need to log in before you can comment on or make changes to this bug.