Login manager using incorrect origin for determining generated passwords when submitting form
Categories
(Toolkit :: Password Manager, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | unaffected |
firefox69 | --- | unaffected |
firefox70 | blocking | fixed |
firefox71 | --- | fixed |
People
(Reporter: enndeakin, Assigned: MattN)
Details
(Keywords: sec-low, Whiteboard: [passwords:generation] [skyline])
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
|
Details | Review |
In LoginManagerParent.jsm there are a four different places where the documentPrincipal's origin is used. Three of them use the principal of the document that the form is in. But one of them, at
uses the top-level browsing context to determine the origin. This should be using the form's document's origin.
I'm not sure of the security implications of this, perhaps MattN or Sam can comment, but I'll mark it as such for now.
Assignee | ||
Comment 1•5 years ago
|
||
[Tracking Requested - why for this release]: Incorrect pwgen behaviour with subframes. Not sure of the security impact yet.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Matt, are you intending to work on this or can you help find someone to take it on?
Assignee | ||
Comment 3•5 years ago
|
||
Writing a test would be tedious as it involves subframes, password generation and the doorhanger and this change is trivial so I didn't make one. This code will also get rewritten to use BrowsingContexts properly to improve sandboxing shortly.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Heh, I was already working on this when you needinfo'd me.
Rating as sec-low for "Lack of proper input validation". Note that this code never affected release users so probably doesn't need an advisory. The worst case is that a login for the top-level origin would be deleted but only if the user generated a password for the top-level origin in that same Firefox process lifetime.
Assignee | ||
Comment 5•5 years ago
|
||
Comment on attachment 9094745 [details]
Bug 1581903 - Use the frame origin in LMP.onFormSubmit. r=sfoster
Beta/Release Uplift Approval Request
- User impact if declined: Login for the top-level origin could get deleted if the user submits a login form in a cross-domain subframe after having generated a password in the top-level frame's origin.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Super-straightforward change to do origin checks like we do elsewhere in this file.
- String changes made/needed: None
Updated•5 years ago
|
Comment 6•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/ec2ef662cf5aec227dca6cc2e36647cc99e13c9c
https://hg.mozilla.org/mozilla-central/rev/ec2ef662cf5a
Comment 7•5 years ago
|
||
Comment on attachment 9094745 [details]
Bug 1581903 - Use the frame origin in LMP.onFormSubmit. r=sfoster
Fix for pwgen behavior, let's take it for beta 10.
Comment 8•5 years ago
|
||
uplift |
Comment 11•5 years ago
|
||
Based on previous comment, mark it with qe-verify-
Updated•4 years ago
|
Description
•