document.com_agilebits_onepassword_fill( raw_data ) is danger in some case

[mala]

How to reproduce

1. save password or create new login entry on 1password app
2. change url to attacker’s domain.
3. fillin password on malicious page by 1password app or extension.
4. nakedDomains contains original domain.

example:

Object.defineProperty(document, "com_agilebits_onepassword_fill", {
    enumerable: false,            
    configurable: false,        
    writable: false,          
    value: function(a){       
        alert(JSON.stringify(a));
    }                         
});                     

• attacker can find another domain that is a target user using same password.
• sub.example.com can get password-saved-domain.example.com (heroku, appspot, etc)

it is problem of js context.
js context of “stringByEvaluatingJavaScriptFromString” is like a bookmarklet.

I also tested on browser extension(chrome,firefox), content script of browser extension run at “isolated world” so browser extensions are maybe not affected.(I'm not investigating deeply)

[radazzouz]

Hello @mala,

Thanks for taking the time to write in for reporting this issue 👍

The issue that you discovered is in fact made out of two issues:

• The issue where a malicious web page could pre-define com_agilebits_onepassword_fill.
• The issue where we send an array of all the nakedDomains of the selected item, which would affect users who save multiple and unrelated URLs for the same 1Password Login.

We’ve identified the fixes and are currently testing them in development. We’ll be sure to update you by commenting on this issue, and close it once the fix is out.

Cheers!

[mala]

@radazzouz @jxpx777

Hi, I think that defense on js side is meaningless, because there are other ways to get js arguments.
#261

It would be better to use a local variables. (if there are no reasons to use document's property)
and raw_data to minimum data for fillin script.

(function(a){
 var fillin = function(){ ... }
 ...
 fillin();
})(stripped_data_for_fillin_script)

[jxpx777]

Thanks so much, @mala! It took @radazzouz and me the better part of today fighting with our build process but I think we have a good solution. I've updated #261 with the latest changes. I would love if you could take a look and let us know your thoughts.

[mala]

@jxpx777
Hi, LGTM about js code. limitation of sending data is 1password app side?
I can't find limitation code, so please review it by your team.

Thanks.

[jxpx777]

Yes, that's right. On iOS, the domains list is handled by the app and not the JS. We can let you know when this is released so you can test further if you like.

Jamie Phelps
Code Wrangler @ AgileBits

On Sep 19, 2015, at 07:50, mala notifications@github.com wrote:

@jxpx777
Hi, LGTM about js code. limitation of sending data is 1password app side?
I can't find limitation code, so please review it by your team.

Thanks.

—
Reply to this email directly or view it on GitHub.

[mala]

ok, savedUrl and url also should be restricted, please take care.

[radazzouz]

Thanks @mala! We are no longer sending the properties you mentioned (nakedDomains, url and savedUrl) in 1Password 6.0.1. Please make sure that you check for updates in the App Store 👍

Once again, thank you so much for the assist here!