Security Considerations with External Messaging

[tshaddix]

I'd like us to consider the possible issues that could arise with supporting onMessageExternal. We currently don't do any sort of check on the port.sender property, meaning any other extension or website script could send messages straight to the background store.

What are the possible implications of this? Should we include something in the package to run validation on an external connection? Should we even support external messaging?

I feel like the support for external messaging was a bit premature, honestly (my bad). I feel like we could have solved #47 by simply suggesting listeners in the background page that push the messages to store.dispatch after the sender has been checked.

Maybe we can solve this by adding an option of senderIds which is an array of allowed external sender ids. We could also have an allowExternal boolean which sets up external listeners.

[tshaddix]

@vhmth @paulius005

[vhmth]

Will look into this tonight - waiting for link expanding on GH to land in Loom first. :-)

[tshaddix]

@vhmth Oh please ignore everything I've ever tagged you in until that's done :P Thank you so much for adding that!

[vhmth]

https://www.useloom.com/share/620cdc80f73b11e6bca7777543759035

[paulius005]

@tshaddix I like the idea of allowExternal over using senderIds since those can be found fairly easily by just inspecting the code.

If support is continued for onMessageExternal, I would say a README mention of the externally_connectable manifest.json file option found here: https://developer.chrome.com/extensions/manifest/externally_connectable would be a good move to keep the possible security issues top of mind for people using react-chrome-redux.

cc @vhmth

[vhmth]

@paulius005 fully agree that a note in the README about externally_connectable is a good move.

[tshaddix]

@vhmth @paulius005 Embedded Loom vids look soooooo slick.

Great call with externally_connectable! I was not aware of that property. That makes me feel a lot better.

I proposed senderIds as a way to explicitly declare where external connections can come from as it allows a dev to take confidence in how open their extension is. I'm not worried about the ids being available through inspection (extension/app ids are not secret anyways) as the security comes from checking the message sender property to make sure it is allowed. An app/extension can not fake this sender property, making it a decent security measurement.

This is actually how chome suggest doing it here:

// For simple requests:
chrome.runtime.onMessageExternal.addListener(
  function(request, sender, sendResponse) {
    if (sender.id == blacklistedExtension)
      return;  // don't allow this extension access
    else if (request.getTargetData)
      sendResponse({targetData: targetData});
    else if (request.activateLasers) {
      var success = activateLasers();
      sendResponse({activateLasers: success});
    }
  });
});

Your right, however, that this only solves extension/app connections. This doesn't help in the realm of website connections...

I think a simple security fix would be to move forward with allowExternal and cross the bridge of explicit app/extension id declaration when we come to it.

[vhmth]

I completely agree with doing the simple thing first and leveraging allowExternal for now as a step in the right direction. They're both things that should be leveraged by the user actually. :-) Good call.

[vhmth]

Glad you like the Loom videos! :-)

[tshaddix]

@vhmth Love the loom videos :)

Cool, so I've re-labled this as an enhancement so we can make sure to put it on our roadmap of todos