From 8d9bc10d8128aae03dfde62fd00075fe492ead10 Mon Sep 17 00:00:00 2001
From: Martin Pitt <mpitt@redhat.com>
Date: Tue, 14 Sep 2021 09:02:33 +0200
Subject: [PATCH] common: Restrict frame embedding to same origin

Declare `X-Frame-Options: sameorigin` [1] so that cockpit frames can
only be embedded into pages coming from the same origin. This is similar
to setting CORP in commit 2b38b8de92f9a (which applies to `<script>`,
`<img>`, etc.).

The main use case for embedding is to run cockpit-ws behind a reverse
proxy, while also serving other pages. Cross-origin embedding is
discouraged these days to prevent "clickjacking".

Cross-origin embedding already did not work in most cases: Frames would
always just show the login page.  However, this looks confusing and is
unclean. With X-Frame-Options, the browser instead shows an explanatory
error page.

Mention the same origin requirement in the embedding documentation.

Fixes #16122
https://bugzilla.redhat.com/show_bug.cgi?id=1980688
CVE-2021-3660

[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options
---
 doc/guide/embedding.xml         |  4 +++-
 pkg/base1/test-http.js          |  2 ++
 src/bridge/test-httpstream.c    |  2 +-
 src/bridge/test-packages.c      |  2 +-
 src/common/cockpitwebresponse.c |  6 ++++++
 src/common/test-webresponse.c   |  2 +-
 src/ws/test-channelresponse.c   |  2 +-
 test/verify/check-connection    |  4 +++-
 test/verify/check-embed         | 15 ++++-----------
 9 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/doc/guide/embedding.xml b/doc/guide/embedding.xml
index 99b30dcebd2c..7fe8aa962ac1 100644
--- a/doc/guide/embedding.xml
+++ b/doc/guide/embedding.xml
@@ -5,7 +5,9 @@
   <title>Embedding and Integrating Cockpit</title>
 
   <para>Cockpit can be embedded in other web applications either as a whole or specific
-    Cockpit components can be integrated.</para>
+    Cockpit components can be integrated. Due to frame security policy restrictions,
+    this only works if Cockpit and the web application have the <emphasis>same origin</emphasis>;
+    this is commonly achieved by running both from a common reverse proxy.</para>
 
   <section id="embedding-full">
     <title>Embedding the Cockpit Interface</title>
diff --git a/pkg/base1/test-http.js b/pkg/base1/test-http.js
index 9d38abcb8a77..6d86a8f33356 100644
--- a/pkg/base1/test-http.js
+++ b/pkg/base1/test-http.js
@@ -208,6 +208,7 @@ QUnit.test("headers", function (assert) {
                     "Referrer-Policy": "no-referrer",
                     "X-DNS-Prefetch-Control": "off",
                     "X-Content-Type-Options": "nosniff",
+                    "X-Frame-Options": "sameorigin",
                     "Cross-Origin-Resource-Policy": "same-origin",
                 }, "got back headers");
             })
@@ -250,6 +251,7 @@ QUnit.test("connection headers", function (assert) {
                     "Referrer-Policy": "no-referrer",
                     "X-DNS-Prefetch-Control": "off",
                     "X-Content-Type-Options": "nosniff",
+                    "X-Frame-Options": "sameorigin",
                     "Cross-Origin-Resource-Policy": "same-origin",
                 }, "got back combined headers");
             })
diff --git a/src/bridge/test-httpstream.c b/src/bridge/test-httpstream.c
index d6aabc968705..274fe0776035 100644
--- a/src/bridge/test-httpstream.c
+++ b/src/bridge/test-httpstream.c
@@ -34,7 +34,7 @@
 extern gboolean cockpit_webserver_want_certificate;
 
 /* JSON dict snippet for headers that are present in every request */
-#define STATIC_HEADERS "\"Cross-Origin-Resource-Policy\":\"same-origin\",\"Referrer-Policy\":\"no-referrer\",\"X-Content-Type-Options\":\"nosniff\",\"X-DNS-Prefetch-Control\":\"off\""
+#define STATIC_HEADERS "\"Cross-Origin-Resource-Policy\":\"same-origin\",\"Referrer-Policy\":\"no-referrer\",\"X-Content-Type-Options\":\"nosniff\",\"X-DNS-Prefetch-Control\":\"off\",\"X-Frame-Options\":\"sameorigin\""
 
 static void
 on_closed_set_flag (CockpitChannel *channel,
diff --git a/src/bridge/test-packages.c b/src/bridge/test-packages.c
index 8c90f936063f..b11a8c8341ce 100644
--- a/src/bridge/test-packages.c
+++ b/src/bridge/test-packages.c
@@ -47,7 +47,7 @@
 #define CHECKSUM_CSP            "80921dc3cde9ff9f2acd2a5851f9b2a3b25ea7b4577128461d9e32fbdd671e16"
 
 /* JSON dict snippet for headers that are present in every request */
-#define STATIC_HEADERS "\"X-DNS-Prefetch-Control\":\"off\",\"Referrer-Policy\":\"no-referrer\",\"X-Content-Type-Options\":\"nosniff\",\"Cross-Origin-Resource-Policy\": \"same-origin\""
+#define STATIC_HEADERS "\"X-DNS-Prefetch-Control\":\"off\",\"Referrer-Policy\":\"no-referrer\",\"X-Content-Type-Options\":\"nosniff\",\"Cross-Origin-Resource-Policy\": \"same-origin\",\"X-Frame-Options\": \"sameorigin\""
 #define STATIC_HEADERS_CACHECONTROL STATIC_HEADERS ",\"Cache-Control\":\"no-cache, no-store\""
 
 extern const gchar **cockpit_bridge_data_dirs;
diff --git a/src/common/cockpitwebresponse.c b/src/common/cockpitwebresponse.c
index fb09d22a6957..671f1c50821f 100644
--- a/src/common/cockpitwebresponse.c
+++ b/src/common/cockpitwebresponse.c
@@ -747,6 +747,7 @@ enum {
     HEADER_REFERRER_POLICY = 1 << 5,
     HEADER_CONTENT_TYPE_OPTIONS = 1 << 6,
     HEADER_CROSS_ORIGIN_RESOURCE_POLICY = 1 << 7,
+    HEADER_X_FRAME_OPTIONS = 1 << 8,
 };
 
 static GString *
@@ -789,6 +790,8 @@ append_header (GString *string,
     return HEADER_CONTENT_TYPE_OPTIONS;
   if (g_ascii_strcasecmp ("Cross-Origin-Resource-Policy", name) == 0)
     return HEADER_CROSS_ORIGIN_RESOURCE_POLICY;
+  if (g_ascii_strcasecmp ("X-Frame-Options", name) == 0)
+    return HEADER_X_FRAME_OPTIONS;
   if (g_ascii_strcasecmp ("Content-Length", name) == 0 ||
       g_ascii_strcasecmp ("Transfer-Encoding", name) == 0 ||
       g_ascii_strcasecmp ("Connection", name) == 0)
@@ -900,6 +903,9 @@ finish_headers (CockpitWebResponse *self,
    * be able to read any resource. This does *not* affect embedding with <iframe> */
   if ((seen & HEADER_CROSS_ORIGIN_RESOURCE_POLICY) == 0)
     g_string_append (string, "Cross-Origin-Resource-Policy: same-origin\r\n");
+  /* This is the counterpart for iframe embedding, line of defence against clickjacking */
+  if ((seen & HEADER_X_FRAME_OPTIONS) == 0)
+    g_string_append (string, "X-Frame-Options: sameorigin\r\n");
 
   g_string_append (string, "\r\n");
   return g_string_free_to_bytes (string);
diff --git a/src/common/test-webresponse.c b/src/common/test-webresponse.c
index 53b7a1563f4f..26054386cd36 100644
--- a/src/common/test-webresponse.c
+++ b/src/common/test-webresponse.c
@@ -35,7 +35,7 @@
 #include <string.h>
 
 /* headers that are present in every request */
-#define STATIC_HEADERS "X-DNS-Prefetch-Control: off\r\nReferrer-Policy: no-referrer\r\nX-Content-Type-Options: nosniff\r\nCross-Origin-Resource-Policy: same-origin\r\n\r\n"
+#define STATIC_HEADERS "X-DNS-Prefetch-Control: off\r\nReferrer-Policy: no-referrer\r\nX-Content-Type-Options: nosniff\r\nCross-Origin-Resource-Policy: same-origin\r\nX-Frame-Options: sameorigin\r\n\r\n"
 static gchar *srcdir;
 
 typedef struct {
diff --git a/src/ws/test-channelresponse.c b/src/ws/test-channelresponse.c
index 9ac923904a90..f9e9abacc2d3 100644
--- a/src/ws/test-channelresponse.c
+++ b/src/ws/test-channelresponse.c
@@ -53,7 +53,7 @@
 #define PASSWORD "this is the password"
 
 /* headers that are present in every request */
-#define STATIC_HEADERS "X-Content-Type-Options: nosniff\r\nX-DNS-Prefetch-Control: off\r\nReferrer-Policy: no-referrer\r\nCross-Origin-Resource-Policy: same-origin\r\n"
+#define STATIC_HEADERS "X-Content-Type-Options: nosniff\r\nX-DNS-Prefetch-Control: off\r\nReferrer-Policy: no-referrer\r\nCross-Origin-Resource-Policy: same-origin\r\nX-Frame-Options: sameorigin\r\n"
 
 typedef struct {
   CockpitWebService *service;
diff --git a/test/verify/check-connection b/test/verify/check-connection
index 5623fde0ceab..c48a574a14f4 100755
--- a/test/verify/check-connection
+++ b/test/verify/check-connection
@@ -411,8 +411,9 @@ class TestConnection(MachineCase):
         self.assertIn(
             "default-src 'self' https://127.0.0.1:9090; connect-src 'self' https://127.0.0.1:9090 wss://127.0.0.1:9090", headers)
         self.assertIn("Access-Control-Allow-Origin: https://127.0.0.1:9090", headers)
-        # CORP is also set for dynamic paths
+        # CORP and Frame-Options are also set for dynamic paths
         self.assertIn("Cross-Origin-Resource-Policy: same-origin", headers)
+        self.assertIn("X-Frame-Options: sameorigin", headers)
 
         self.allow_journal_messages(
             ".*Peer failed to perform TLS handshake",
@@ -627,6 +628,7 @@ class TestConnection(MachineCase):
         self.assertIn("HTTP/1.1 200 OK\r\n", headers)
         self.assertIn("Content-Type: text/html\r\n", headers)
         self.assertIn("Cross-Origin-Resource-Policy: same-origin\r\n", headers)
+        self.assertIn("X-Frame-Options: sameorigin\r\n", headers)
         # login.html is not always accessible as a file (e.g. on CoreOS), so just assert a reasonable content length
         self.assertIn("Content-Length: ", headers)
         length = int(headers.split('Content-Length: ', 1)[1].split()[0])
diff --git a/test/verify/check-embed b/test/verify/check-embed
index 5d7a370ffecf..75016cedfbfa 100755
--- a/test/verify/check-embed
+++ b/test/verify/check-embed
@@ -94,20 +94,13 @@ Shell=/shell/index.html
         b.open("http://localhost:12346/index.html")
         b.set_val("#embed-address", "http://{0}:{1}".format(m.web_address, m.web_port))
         b.click("#embed-full")
-        # FIXME (#16122): we should not even get that far, frame loading should already be blocked here
         b.wait_visible("iframe[name='embed-full'][loaded]")
         b.switch_to_frame("embed-full")
 
-        # second line of defense: existing login cookie does not work (default browser protection)
-        b.wait_visible("#login")
-        b.set_val("#login-user-input", "admin")
-        b.set_val("#login-password-input", "foobar")
-        b.click('#login-button')
-        b.expect_load_frame("embed-full")
-
-        # third line of defense: login succeeds and creates a PAM session; but loading session UI does not
-        # (again, due to default browser protection)
-        b.wait_visible("#login")
+        # X-Frame-Options sameorigin blocks frame
+        if b.cdp.browser == "firefox":
+            b.wait_visible("body.neterror")
+        self.assertFalse(b.is_present("#login"))
 
 
 if __name__ == '__main__':