security: post OAuth token to client webapps if any #1149

rzr · 2018-06-29T07:36:06Z

When loaded from window.opener CORS is blocked by default,
so this is workaraounding security issues like:

Uncaught DOMException: Blocked a frame with origin (...)
from accessing a cross-origin frame. at (...)

For any security concern this change can be reverted anytime.

Change-Id: I42af71ae822491150c019cff9688356b1a0e2532
Signed-off-by: Philippe Coval p.coval@samsung.com

hobinjk · 2018-06-29T14:03:07Z

At this level of integration you would be better of setting up a proper OAuth service by editing src/models/oauthclients.ts as described in this setup document.

rzr · 2018-06-29T15:06:21Z

Yea I tried this once, but on some browsers was not possible from file://... origin (tizen local webapp) so I hacked around to make it work somehow, until a better solution is found.

FYI, I used this bit for PWA app:
https://vimeo.com/277663462#webthing-webapp-pwa-20180629rzr

Maybe I will refactor this proof of concept later, but I wished the code could support several platforms.

hobinjk · 2018-07-09T14:33:33Z

Yeah, unfortunately this would make it really easy for an attacker to embed a frame of the local token service in their page and trick the user into giving them complete access to their gateway.

I think it's possible to limit the message listener to only respond if the message originates from a file:// URL through ev.origin or something. That would make this PR much more secure.

rzr · 2018-07-09T15:40:48Z

Yes I was about to suggest this, meanwhile I got proper OAuth on http.
but Instead of testing file:// protocol I prefer to test location.hostname != null, in case other protocol to be used, would that be fine ?

hobinjk · 2018-07-09T16:51:33Z

I would prefer a whitelist of specific protocols so that the exact behavior is completely specified.

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Signed-off-by: Philippe Coval <p.coval@samsung.com>

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted It was tested on Tizen5 on TM1. Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Signed-off-by: Philippe Coval <p.coval@samsung.com>

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted It was tested on Tizen5 on TM1. Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

codecov-io · 2019-01-29T20:11:00Z

Codecov Report

Merging #1149 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
+ Coverage   72.32%   72.32%   +<.01%     
==========================================
  Files         126      126              
  Lines        6898     6899       +1     
  Branches     1031     1031              
==========================================
+ Hits         4989     4990       +1     
  Misses       1664     1664              
  Partials      245      245

Impacted Files	Coverage Δ
config/default.js	`100% <ø> (ø)`	⬆️
src/router.js	`98.52% <100%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3198c40...14d3bd2. Read the comment docs.

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted It was tested on Tizen5 on TM1. Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

mrstegeman · 2019-04-04T19:50:03Z

@rzr @hobinjk Is this still needed for anything?

rzr · 2019-04-05T07:26:58Z

I think so it was needed for an "offline" webapp PoC... (loaded from file:// protocol), I made it evolve to support various frameworks (eg: aframe) , so it would be nice to have it in.

hobinjk · 2019-04-05T14:34:39Z

👍, after #1712 this just needs an additional modification to https://github.com/mozilla-iot/gateway/blob/master/src/router.js#L46 to have 'filesystem:' instead of 'none'

hobinjk · 2019-04-05T14:54:20Z

Additionally, since this adds the ability for any random file:// page to clickjack the gateway, the modification to the content-security policy should be guarded by a config option (like 7fbaf56)

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted It was tested on Tizen5 on TM1: BUILD_ID=tizen-unified_20180528.1_mobile-wayland-armv7l-tm1 Relate-to: https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage# Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

mrstegeman · 2019-04-10T17:25:06Z

config/default.js

@@ -74,5 +74,6 @@ module.exports = {
      ssid_base: 'WebThings Gateway',
    },
  },
+  oauthPostToken: !false,


This should default to false.

yes, I used !false as reminder

mrstegeman · 2019-04-10T17:25:35Z

src/views/local-token-service.handlebars

@@ -123,6 +123,15 @@ let text = client.get("<span class="origin">https://gateway.local</span>/things"
        </div>
      </div>
    </section>
+    <script>
+/* postToken to client apps if any */
+if (config.get('oauthPostToken') && (window.location.protocol === "file:") {


This is a handlebars template that gets used in the UI. Instead of using config, you'll need to pass a value into the template.

You're also missing a paren at the end of this line.

yes that code didnt work actually I figured out I used other hack to get token, next version works anyway

hobinjk

Thanks for the update! There are just a few minor changes then this LGTM

hobinjk · 2019-04-10T17:26:09Z

src/views/local-token-service.handlebars

@@ -123,6 +123,15 @@ let text = client.get("<span class="origin">https://gateway.local</span>/things"
        </div>
      </div>
    </section>
+    <script>
+/* postToken to client apps if any */
+if (config.get('oauthPostToken') && (window.location.protocol === "file:") {


The Content-Security-Policy setting means we'll only be embedded in trusted situations so this doesn't need the config.get here

I still use oauthPostToken to prevent a not wanted post action
see next revison

hobinjk · 2019-04-10T17:26:58Z

config/default.js

@@ -74,5 +74,6 @@ module.exports = {
      ssid_base: 'WebThings Gateway',
    },
  },
+  oauthPostToken: !false,


This should be false

hobinjk · 2019-04-10T17:28:12Z

src/views/local-token-service.handlebars

+    if (ev.data.message === "token") {
+      ev.source.postMessage({ message: { token: "{{token}}" }}, "*");
+    }
+});


The indentation of this is a bit off and the close brace from the initial if statement is missing

When client is loaded from file oauth can't be used because location.hostname is null. So we fallback by using postMessage API, instead of parsing the token page. Test could have been done on location.hostname, but for security concerns only file:// protocol is whitelisted It was tested on Tizen5 on TM1: BUILD_ID=tizen-unified_20180528.1_mobile-wayland-armv7l-tm1 Relate-to: https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage# Change-Id: I42af71ae822491150c019cff9688356b1a0e2532 Bug: WebThingsIO#1149 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

mrstegeman

Looks good to me, other than the one comment.

mrstegeman · 2019-04-11T12:27:57Z

src/router.js

-      response.set('Content-Security-Policy', 'frame-ancestors \'none\'');
+      response.set('Content-Security-Policy',
+                   config.get('oauthPostToken') ?
+                     'frame-ancestors filesystem:' :


I believe filesystem: needs single quotes around it: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors

Seems not , well with quotes I got an alert that they were ignored:

Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0 Content Security Policy: Couldn’t parse invalid host 'filesystem:'

On Tizen:

D/ConsoleMessage( 1681): [WebThings0] file:///js/index.js:438: Mozilla/5.0 (Linux; Tizen 5.0; SAMSUNG TM1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/ Mobile Safari/537.36 The source list for Content Security Policy directive 'frame-ancestors' contains an invalid source: ''filesystem:''. It will be ignored.

... now i am unsure if scheme should be quoted or not, see:

https://www.w3.org/TR/CSP2/#scheme_source

https://tools.ietf.org/html/rfc3986#section-3.1

edit: I will file a bug on MDN

I think you're right. Confusing docs on MDN.

Yes I fixed MDN with help of @sheppy (thx)

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors$compare?locale=en-US&to=1535837&from=1534635

mrstegeman · 2019-04-11T14:30:16Z

@hobinjk Any other objections?

mrstegeman requested a review from hobinjk June 29, 2018 11:39

mrstegeman added the review label Jun 29, 2018

rzr force-pushed the sandbox/rzr/review/master branch from 6965369 to c7d0f4f Compare July 9, 2018 18:51

rzr force-pushed the sandbox/rzr/review/master branch from c7d0f4f to 99336c0 Compare July 23, 2018 10:42

rzr changed the title ~~WIP: security: post OAuth token to client webapps if any~~ security: post OAuth token to client webapps if any Jul 23, 2018

rzr force-pushed the sandbox/rzr/review/master branch from 99336c0 to 3689ead Compare August 3, 2018 09:05

rzr force-pushed the sandbox/rzr/review/master branch from 3689ead to ab96de7 Compare October 24, 2018 09:42

mrstegeman force-pushed the sandbox/rzr/review/master branch from ab96de7 to a0fbcdc Compare January 29, 2019 20:00

ghost assigned mrstegeman Jan 29, 2019

WebThingsIO deleted a comment from codecov-io Jan 29, 2019

mrstegeman removed their assignment Jan 29, 2019

rzr force-pushed the sandbox/rzr/review/master branch from a0fbcdc to 0f70e4e Compare February 14, 2019 09:43

rzr mentioned this pull request Feb 15, 2019

Feature: Seemless login SamsungInternet/webthings-webapp#2

Open

mrstegeman removed the review label Apr 3, 2019

rzr force-pushed the sandbox/rzr/review/master branch from 0f70e4e to 23a1ab2 Compare April 10, 2019 13:35

mrstegeman suggested changes Apr 10, 2019

View reviewed changes

hobinjk suggested changes Apr 10, 2019

View reviewed changes

rzr force-pushed the sandbox/rzr/review/master branch from 23a1ab2 to 14d3bd2 Compare April 11, 2019 11:06

mrstegeman suggested changes Apr 11, 2019

View reviewed changes

mrstegeman approved these changes Apr 11, 2019

View reviewed changes

hobinjk approved these changes Apr 11, 2019

View reviewed changes

mrstegeman merged commit 151f7d3 into WebThingsIO:master Apr 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: post OAuth token to client webapps if any #1149

security: post OAuth token to client webapps if any #1149

rzr commented Jun 29, 2018

hobinjk commented Jun 29, 2018

rzr commented Jun 29, 2018

hobinjk commented Jul 9, 2018

rzr commented Jul 9, 2018

hobinjk commented Jul 9, 2018

codecov-io commented Jan 29, 2019 •

edited

Loading

mrstegeman commented Apr 4, 2019

rzr commented Apr 5, 2019

hobinjk commented Apr 5, 2019

hobinjk commented Apr 5, 2019

mrstegeman Apr 10, 2019

rzr Apr 11, 2019

mrstegeman Apr 10, 2019 •

edited

Loading

rzr Apr 11, 2019

hobinjk left a comment

hobinjk Apr 10, 2019

rzr Apr 11, 2019

hobinjk Apr 10, 2019

rzr Apr 11, 2019

hobinjk Apr 10, 2019

rzr Apr 11, 2019

mrstegeman left a comment

mrstegeman Apr 11, 2019

rzr Apr 11, 2019 •

edited

Loading

mrstegeman Apr 11, 2019

rzr Apr 11, 2019 •

edited

Loading

mrstegeman commented Apr 11, 2019

security: post OAuth token to client webapps if any #1149

security: post OAuth token to client webapps if any #1149

Conversation

rzr commented Jun 29, 2018

hobinjk commented Jun 29, 2018

rzr commented Jun 29, 2018

hobinjk commented Jul 9, 2018

rzr commented Jul 9, 2018

hobinjk commented Jul 9, 2018

codecov-io commented Jan 29, 2019 • edited Loading

Codecov Report

mrstegeman commented Apr 4, 2019

rzr commented Apr 5, 2019

hobinjk commented Apr 5, 2019

hobinjk commented Apr 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrstegeman Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hobinjk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrstegeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzr Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzr Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

mrstegeman commented Apr 11, 2019

codecov-io commented Jan 29, 2019 •

edited

Loading

mrstegeman Apr 10, 2019 •

edited

Loading

rzr Apr 11, 2019 •

edited

Loading

rzr Apr 11, 2019 •

edited

Loading