Closed Bug 1424812 Opened 7 years ago Closed 7 years ago

Origin confusion when creating a password using the "share" action on Firefox IOS

Categories

(Firefox for iOS :: General, defect, P2)

Product:
Firefox for iOS
Component:
General
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
fxios + ---

People

(Reporter: pauljt, Unassigned)

References

(
URL
)

Details

(Keywords: sec-high)

As expected this is a bit dicey, most probably due to the horrible things that apps have to do to the "share" action on IOS. I'm still looking into this, but I've found that its possible to mess with the script that OnePasswordExtension.m injects into the page. So far I can NOT retrieve a password for another domain, but I can prefill the information for another domain. One password extension injects a script into the page to gather information. I wanted to mess with this, and I noticed that the script ends with: return JSON.stringify(FieldCollector.a(document, 'oneshotUUID')); So I overwrote JSON.stringify with my own function which modifies the object prior to stringifying. The attack scenario here is pretty unlikely due to the amount of user interaction, but Im logging this as it might be chain-able with other issues. It would have to be something like: 1. Visit https://misuse.co/a/pass.html 2. Choose "Share Page with..." from the ... menu 3. Choose OnePassword as the target application 4. Choose "create new login for misuse.co" (The correct URL is shown at this point) 5. The new Login view is shown, but its prefilled with the MODIFIED domain (mozilla.org) So not really sure if/how an attacker could use abuse this. You might be able to convince the user to save a new item, but Im not sure what if anything that gives an attacker. Other than confusing the user perhaps. However, this is pretty weak - it would seem fairly trivial to at least validate the URLs to ensure that at least the form URL was same-origin with the actual URL of the page shown in the webview and remove the risk here.
Component: Security: Review Requests → General
Product: Firefox → Firefox for iOS
Target Milestone: --- → 11.0
Interestingly chrome does NOT have this issue - I wonder if it's something that was fixed in a later version. Or maybe my PoC is broken on Chrome (its hard to test without being able to debug the web view).
And now it's sec-high. Turns out Lastpass IS confused by this webpage. Following the STR above, last pass suggests the wrong password, allowing a page to steal logins for other domains. Again chrome on IOS is NOT affected by this, but it dont know if thats my script erroring on chrome, or if they did something to fix it. (maybe upgrade the version)
Keywords: sec-high
Stefan, can you help us move this bug along by finding an assignee? Stealing passwords from an evil page sound pretty bad :)
Flags: needinfo?(sarentz)
My guess is that this is actually a Lastpass issue. Although in my opinion triggered by a confusing 1Password API. When I look at the native code for this integration, i see the following being passed to the password manager application: https://github.com/agilebits/onepassword-app-extension/blob/1.8.5/OnePasswordExtension.m#L340 NSDictionary *item = @{ AppExtensionVersionNumberKey : VERSION_NUMBER, AppExtensionURLStringKey : URLString, AppExtensionWebViewPageDetails : collectedPageDetailsDictionary }; The URLString is the actual origin of the web page. It comes straight from the WebView: https://github.com/agilebits/onepassword-app-extension/blob/1.8.5/OnePasswordExtension.m#L395 [self findLoginIn1PasswordWithURLString:webView.URL.absoluteString collectedPageDetails:result... So in case of the example that would be https://misuse.co - but the collectedPageDetailsDictionary also contains a page url, which Paul's hack has modified. The URL from the top level frame of the WebView should probably be leading. It is unclear why they send the URL twice, but a proper functioning password manager should check if URLString and collectedPageDetailsDictionary.url match. It seems that is what 1Password does and LastPass doesn't. It would be trivial for us to make this check more strict and to make sure that URLString and collectedPageDetailsDictionary.url have similar domains. I'll try to get AgileBits on this bug for their opinion.
Flags: needinfo?(sarentz)
Does this happen in Safari too? I found this bit of code that Chrome for iOS injects via a user script that indicates that Chrome may be doing something to safeguard against this. It looks like they retain a "pure" reference to the native `JSON.stringify` for their own private usage so that their user scripts don't call a tampered version. That being said, `JSON.stringify` is still able to be overwritten and I don't see them doing anything to prevent that from happening. I wonder if the password managers are calling `__gCrWeb.common.JSONStringify` instead of `JSON.stringify` in Chrome for iOS? https://github.com/chromium/chromium/blob/master/ios/web/web_state/js/resources/common.js#L30-L72
Flags: needinfo?(ptheriault)
I've also noticed that Chrome's own autofill code uses their `JSONSafeObject` to prevent scripts from tampering with the data by overwriting the native `toJSON` implementation: https://github.com/chromium/chromium/blob/master/components/autofill/ios/browser/resources/autofill_controller.js#L323-L325 I don't think this affects 3rd-party password managers though, so I'm still unsure exactly as to why this doesn't reproduce there. That being said, I have a few ideas for how we can possibly fix this: 1. Can we just `Object.freeze(JSON)`? Probably not the ideal solution because it possibly may affect webcompat, but if we can avoid pages re-writing `JSON.stringify` altogether, that would certainly fix this (I think... maybe?). 2. Stefan's suggestion in comment 4 could also work, but would require LastPass to provide the fix. 3. Another possible fix that password managers in general could do is to have a simple check that `JSON.stringify` is indeed the native `JSON.stringify` before calling it. This would also require password managers to provide the fixes, so its also less-than-ideal. If you have any other hints as to how Chrome may be avoiding this exploit, I can continue spelunking through their source to look for specifics, but since I'm not seeing anything else related to hardening against this in their user scripts, I'm sorta out of ideas as to where to look.
Sorry for the late response here. A colleague and I are working on a potential fix for this, but it's requiring a fair bit of reworking to figure out if it'll even work. However, I can at least let you know why Chrome is not impacted. They use our native app filling feature, which doesn't do any of the filling. So they use a different API, then they implement their own filling features. Firefox uses our WKWebView filling feature, which means a bit more of the process is in the 1Password extension. We generally don't recommend people follow the Chrome way of doing this because it takes a lot of the ability to fix out of our hands, and adds more to the host application's plate. We quite dislike that we have to tell users "sorry, we can't fix that, it's in the hands of Chrome, so you'll have to reach out to them to get this fixed." It also means there's a massive disconnect in what we can fill and what Chrome doesn't fill. Inconsistency between apps is kind of a frustrating situation for users and as a result, for us as well. If our fix for this works, it'll just require that we update the App Extension API again, and you all update the cocoa pod or whatever it is you're using. I can do that as well, as I updated it last time and submitted a pull request. I was hoping to be able to tell you all more today but I didn't want you to wait to get the details above.
Thanks Kyle! We really appreciate you addressing this issue. We'd be glad to help you out if you need. Please don't hesitate to let me know. If not, just keep us updated here so we know when to update on our end.
Hi all, I don't want to add someone on the cc list here without you all being aware or doing it yourself. Is there any way I can get a colleague of mine added? jamie@agilebits.com The two of us have been working on the solution to this and he may be better equipped to answer your questions than I am. Thanks!
No problem. Added.
(In reply to Justin D'Arcangelo [:justindarc] from comment #5) > Does this happen in Safari too? > > I found this bit of code that Chrome for iOS injects via a user script that > indicates that Chrome may be doing something to safeguard against this. It > looks like they retain a "pure" reference to the native `JSON.stringify` for > their own private usage so that their user scripts don't call a tampered > version. That being said, `JSON.stringify` is still able to be overwritten > and I don't see them doing anything to prevent that from happening. I wonder > if the password managers are calling `__gCrWeb.common.JSONStringify` instead > of `JSON.stringify` in Chrome for iOS? > > https://github.com/chromium/chromium/blob/master/ios/web/web_state/js/ > resources/common.js#L30-L72 They stash the real JSON.stringify, and restore it just prior to use I think. And for anything internal, they use __gCrWeb.common.JSONStringify. See line 45 of the code you linked, and its usage[1] , i think. They say "less likely to be overwritten" - from testing it looks possible mess with chrome's share action through Object.defineProperty() - I can break share entirely but I haven't been able to mess with the data passed yet. Note that the password managers aren't the ones executing script here I think since the issue is that document is spoofed before it is sent to password manager app. At least I think thats whats happening. [1] https://github.com/chromium/chromium/blob/master/components/autofill/ios/browser/resources/autofill_controller.js#L325
Flags: needinfo?(ptheriault)
And no this doesn't happen in safari - presumably since safari has real APIs. Its only the second-class citizen browsers that have to do this tomfoolery to inject APIs directly into the JavaScript of the pages.
Kyle and I have been working on a fix for this issue and we have arrived at a solution. At a high level, this moves the 1Password app extension in a direction more similar to what Firefox is already doing with using WKUserScript. My initial thinking was that WKUserScript might be sandboxed[0], but it turned out that by using WKUserScript, we didn't have to use JSON serialization at all and could simply pass an object to the handler we defined and get an NSDictionary back on the native side. We have a couple of branches we'd love for you to take a look at.[1][2] These are in public, but the changes only indicate that we're removing support for iOS 7 and migrating to WKWebView+WKUserScript. We still have a small change[3] and some cleanup and testing to do with these changes, but we wanted to get your thoughts on the approach and let us know if you have any concerns. [0] Turns out, it doesn't seem to be. I have been able to trigger our scripts' event listeners from the web inspector console. [1] https://github.com/agilebits/onepassword-app-extension/compare/add-framework-support...jxpx777-gks/wkuserscript [2] https://github.com/mozilla-mobile/firefox-ios/compare/master...agilebits:agilebits/kyle/improved/integrate-wkuserscript-update [3] I really liked the Firefox approach to defining a closure with a security token and using that for mutually assured validation! We'll be modifying this slightly given our CustomEvent approach to avoid the page discovering the token.
(In reply to Jamie Phelps from comment #13) > [0] Turns out, it doesn't seem to be. I have been able to trigger our > scripts' event listeners from the web inspector console. Yeah, I don't believe WKUserScript gets any special treatment aside from control over where and when it executes :-/ > [1] > https://github.com/agilebits/onepassword-app-extension/compare/add-framework- > support...jxpx777-gks/wkuserscript +1 -- One minor nit: since both your collect/fill user scripts get injected `atDocumentEnd` for the main frame only, it might be slightly less overhead to just combine them (or concatenate the two sources) into one WKUserScript. We recently added a JS pre-processor that concatenates and minifies all our user scripts at build time to produce a total of 4 user scripts (atDocumentStart, atDocumentEnd x mainFrame, allFrames). As long as both scripts have the same requirements for injection time and main frame only, I don't see why you couldn't combine them. > [2] > https://github.com/mozilla-mobile/firefox-ios/compare/master...agilebits: > agilebits/kyle/improved/integrate-wkuserscript-update Cool! Thanks for the patch! :-) > [3] I really liked the Firefox approach to defining a closure with a > security token and using that for mutually assured validation! We'll be > modifying this slightly given our CustomEvent approach to avoid the page > discovering the token. We just recently landed this a couple weeks ago. It seems like an effective way to ensure messages passed between JS<->Native are authorized. AFAIK, there's no way for web content to peek inside that closure. But, yes, I think this could easily be adapted to your CustomEvent approach. That would address your first point and prevent web content from triggering your event listeners.
Thanks, Justin! Glad to get such positive feedback on this. You're right that we can now combine these two scripts into one. These are actually created in the build process for the 1Password extension project, which powers the app extension in addition to mobile Safari and our desktop browser extensions for Firefox, Chrome, Safari, and Edge. I'm going to be working on those updates soon.
The bug has not been updated, but it looks like you have been making progress. Mind sharing what the status is here?
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(jamie)
We are in the final steps. We need to do some more in-depth testing and then code and documentation cleanup and then get some key reviews from our side before the app-extension portion can be considered complete. Kyle is leading this effort. The Firefox portion was partly to show how this would work for an app that uses the lower level extension item api (since we don't have an example app that does this yet) and partly to get the ball rolling on implementing the fix for Firefox, so if there's a better approach or some feedback about that aspect, please do let us know. I think any code feedback for either the app-extension or Firefox changes can be left in comments on the Github diffs unless they pertain particularly to the security content of this bug.
Flags: needinfo?(jamie)
(In reply to Jamie Phelps from comment #17) > We are in the final steps. We need to do some more in-depth testing and then > code and documentation cleanup and then get some key reviews from our side > before the app-extension portion can be considered complete. Kyle is leading > this effort. > Hi, just checking in with you to see if you've made any progress on this.
Flags: needinfo?(jdarcangelo) → needinfo?(jamie)
tracking-fxios: --- → 12.0
Priority: -- → P2
Sorry for the delay, Justin. We have some things moving about right now and it'll be useful for support for these changes on the 1Password side to be included with the release that will fix this. I'm hopeful we'll get things pushed along further in the next week or so.
Flags: needinfo?(jamie)
Thanks for the update, Jamie!
(In reply to Jamie Phelps from comment #19) > Sorry for the delay, Justin. We have some things moving about right now and > it'll be useful for support for these changes on the 1Password side to be > included with the release that will fix this. I'm hopeful we'll get things > pushed along further in the next week or so. Sorry to keep nagging, but any update here?
Flags: needinfo?(jamie)
Hi Justin, We've had a few discussions around this issue this week and we'd like to try to get this resolved as quickly as possible at this point. I'm really sorry this has taken as long as it has. In order to accomplish this there is one very clear way that this can be resolved for the most number of people the quickest. The proposed solution is going to be to inform other password managers that they need to be using the correct URL key as this will very likely be the quickest and importantly impact the most users. We can update the API, but there are substantial logistical problems with getting information about the API changes to all users. We may, indeed, do so at some point in the future, but it would necessarily be a slow process of deprecations and warnings. We expect the app-extension to be obsoleted before that process would be complete. The question I have for you is if you'd rather reach out to LastPass or if you'd rather we do it. I can certainly handle this if you'd like, and would be happy to make sure credit is given to you all for finding this issue. Thanks, Kyle
> We expect the app-extension to be obsoleted before that process would be complete. I assume this is because of the iOS 12 Autofill extension? Just want to let you know that for Firefox we will support 11 + 12 - we always support latest + previous.
Short answer, yes. With iOS 12 we’ll be going iOS 12 only I believe. Still subject to change. We won’t be removing extension support in iOS 12 but we do anticipate that it’s going to be replaced by password auto fill at some point. So you’ll be fine to support iOS 11 and 12. Extension support will still be present and iOS 12 users will have both options.
Hi all, I had a long weekend and hope you all enjoyed yours. I wanted to followup and keep progress moving on this. Did you want us to notify LastPass or do you want to? Again we're happy to provide credit where credit is due and let them know you found this. I've already drafted up a reply to them and will modify as necessary to contact a few others we know are compatible with the 1Password App Extension API.
Kyle, thanks for the reply. Let me ping Stefan and see how he wants to proceed here.
Flags: needinfo?(sarentz)
Flags: needinfo?(jamie)
Hi kyle, If your team would coordinate this that would be awesome. Please let us know if you need anything from our end Thanks
Flags: needinfo?(sarentz)
Hi Farhan! Thanks. I'll do my best to get things sent out today but it's possible it might be a Monday thing so that I know I'm around when they're likely to reply with any potential questions. I'll let you all know when things are taken care of as well. Thanks! Kyle
As a small form of update, I reported this to security@lastpass.com, about a day later they've told me I need to file it in their bug bounty program, so I've done that. I informed them I don't want a reward for this and instead suggested that if someone need be paid they pick a nonprofit and donate to them. Mozilla has been given full credit for the find as well. I'll update again once I get an idea of when they may have things fixed. If you'd like to be informed of when they've acknowledged the issue I can do that as well.
Thanks for the update, Kyle.
Hey all! LastPass has notified me that their latest iOS release (4.3.4) fixes this issue on their end. Let me know if you need anything else from me but I think you can call this one closed. Kyle
Thanks Kyle! Catalin, can you verify this?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(catalin.suciu)
Resolution: --- → FIXED
Group: firefox-core-security → core-security-release
I can't reproduce this issue using the latest master and Last Pass. Verifying as fixed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(catalin.suciu)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.