Closed Bug 1581903 Opened 5 years ago Closed 5 years ago

Login manager using incorrect origin for determining generated passwords when submitting form

Categories

(Toolkit :: Password Manager, defect, P1)

Desktop
All
defect

Tracking

()

RESOLVED FIXED
mozilla71
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)

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

https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/toolkit/components/passwordmgr/LoginManagerParent.jsm#600

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.

[Tracking Requested - why for this release]: Incorrect pwgen behaviour with subframes. Not sure of the security impact yet.

OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → Desktop
Whiteboard: [passwords:generation] [skyline]

Matt, are you intending to work on this or can you help find someone to take it on?

Flags: needinfo?(MattN+bmo)

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.

Severity: normal → major

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: nobody → MattN+bmo
Severity: major → normal
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Keywords: sec-low

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
Attachment #9094745 - Flags: approval-mozilla-beta?
Severity: normal → major
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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.

Attachment #9094745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Matt, anything to check here?

Flags: qe-verify?
Flags: needinfo?(MattN+bmo)

No, the STR are too complicated

Flags: needinfo?(MattN+bmo)

Based on previous comment, mark it with qe-verify-

Flags: qe-verify? → qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: