449701 - legacy storage module shouldn't share login objects across XPCOM

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This came up briefly during the original review for login manager, but I think it should be revisited. The original concern was around if the overhead of creating clone objects was worth the risk of keeping decrypted strings around. Since GC make it difficult to prevent data from floating around in memory anyway, we decided to punt. Still, I think it would be good to at least make a fair effort at not persisting this data for too long.

Additionally, this would have prevented misleading test bug, which would have caught bug 449698.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Added explicit tests for this condition. Build on top of the changes for bug 449698, so there's a bit of extra diffs here. :(

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Cleaned up patch, now that dependent bug landed.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 333305 [details] [diff] [review]
Patch v.3

>diff --git a/toolkit/components/passwordmgr/src/storage-Legacy.js b/toolkit/components/passwordmgr/src/storage-Legacy.js

>             // If decryption failed (corrupt entry?) skip it.
>             // Note that we allow password-only logins, so username con be "".

s/con/can/
s/username/decryptedUsername/

I wonder whether it would be worth adding a clone() on nsILoginInfo.

Might be nice to have this code use the memoizing getter format that we're using elsewhere at some point (e.g. http://hg.mozilla.org/mozilla-central/index.cgi/annotate/a2837ff832c9/browser/base/content/browser.js#l126 ).

Comment 4

[Justin Dolske [:Dolske]]

Fixed nits, pushed changeset bfed06cd8ad7.