CVE-2021-3660: Improper Restriction of Rendered UI Layers or Frames (CWE-1021)
value: Cockpit (and its plugins) do not seem to protect itself against clickjacking. It is possible to render a page from a cockpit server via another website, inside an <iFrame> HTML entry. This may be used by a malicious website in clickjacking or similar attacks.
CWE description: The web application does not restrict or incorrectly restricts frame objects or UI layers that belong to another application or domain, which can lead to user confusion about which interface the user is interacting with.
CVSS v3 base score: 4.3
Published: 2022-03-10T17:42Z
Repository: https://github.com/cockpit-project/cockpit
Language: C
Files changed: doc/guide/embedding.xml, pkg/base1/test-http.js, src/bridge/test-httpstream.c, src/bridge/test-packages.c, src/common/cockpitwebresponse.c, src/common/test-webresponse.c, src/ws/test-channelresponse.c, test/verify/check-connection, test/verify/check-embed
Commit date: 2021-09-20 16:14:11 +0300
Commit message: 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

Diff: diff --git a/doc/guide/embedding.xml b/doc/guide/embedding.xml
index 99b30dceb..7fe8aa962 100644
--- a/doc/guide/embedding.xml
+++ b/doc/guide/embedding.xml
@@ -3,11 +3,13 @@
 	"http://www.oasis-open.org/docbook/xml/4.3/docbookx.dtd">
 <chapter id="embedding">
   <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>
 
     <para>Cockpit can be embedded into a larger web page as a frame. To embed
diff --git a/pkg/base1/test-http.js b/pkg/base1/test-http.js
index 9d38abcb8..6d86a8f33 100644
--- a/pkg/base1/test-http.js
+++ b/pkg/base1/test-http.js
@@ -206,10 +206,11 @@ QUnit.test("headers", function (assert) {
                     Header3: "three",
                     Header4: "marmalade",
                     "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");
             })
             .always(function() {
                 assert.equal(this.state(), "resolved", "split response didn't fail");
@@ -248,10 +249,11 @@ QUnit.test("connection headers", function (assert) {
                     Header3: "three",
                     Header4: "marmalade",
                     "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");
             })
             .always(function() {
                 assert.equal(this.state(), "resolved", "split response didn't fail");
diff --git a/src/bridge/test-httpstream.c b/src/bridge/test-httpstream.c
index d6aabc968..274fe0776 100644
--- a/src/bridge/test-httpstream.c
+++ b/src/bridge/test-httpstream.c
@@ -32,11 +32,11 @@
 
 /* Declared in cockpitwebserver.c */
 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,
                     const gchar *problem,
                     gpointer user_data)
diff --git a/src/bridge/test-packages.c b/src/bridge/test-packages.c
index 8c90f9360..b11a8c834 100644
--- a/src/bridge/test-packages.c
+++ b/src/bridge/test-packages.c
@@ -45,11 +45,11 @@
 #define CHECKSUM_RELOAD_NEW     "eae62ca12c4a92b4ae7f6b0d2f41cb20be0005a6fc62466fccda1ebe0532cc23"
 #define CHECKSUM_RELOAD_UPDATED "0d1c0b7c6133cc7c3956197fd8a76bef68b158bd78beac75cfa80b75c36aa827"
 #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;
 extern const gchar *cockpit_bridge_local_address;
 extern gint cockpit_bridge_packages_port;
diff --git a/src/common/cockpitwebresponse.c b/src/common/cockpitwebresponse.c
index fb09d22a6..671f1c508 100644
--- a/src/common/cockpitwebresponse.c
+++ b/src/common/cockpitwebresponse.c
@@ -745,10 +745,11 @@ enum {
     HEADER_CACHE_CONTROL = 1 << 3,
     HEADER_DNS_PREFETCH_CONTROL = 1 << 4,
     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 *
 begin_headers (CockpitWebResponse *response,
                guint status,
@@ -787,10 +788,12 @@ append_header (GString *string,
     return HEADER_REFERRER_POLICY;
   if (g_ascii_strcasecmp ("X-Content-Type-Options", name) == 0)
     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)
     {
       g_critical ("Don't set %s header manually. This is a programmer error.", name);
@@ -898,10 +901,13 @@ finish_headers (CockpitWebResponse *self,
     g_string_append (string, "X-Content-Type-Options: nosniff\r\n");
   /* Be very strict here -- there is no reason that external web sites should
    * 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 53b7a1563..26054386c 100644
--- a/src/common/test-webresponse.c
+++ b/src/common/test-webresponse.c
@@ -33,11 +33,11 @@
 
 #include <stdlib.h>
 #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 {
     CockpitWebResponse *response;
     GOutputStream *output;
diff --git a/src/ws/test-channelresponse.c b/src/ws/test-channelresponse.c
index 9ac923904..f9e9abacc 100644
--- a/src/ws/test-channelresponse.c
+++ b/src/ws/test-channelresponse.c
@@ -51,11 +51,11 @@
 #define CHECKSUM "$025e419da72456a49bc60c042b2e983a4a7a44e78eac1fffd062e046e8d4cf3b"
 
 #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;
   GIOStream *io;
   GMemoryOutputStream *output;
diff --git a/test/verify/check-connection b/test/verify/check-connection
index 5623fde0c..c48a574a1 100755
--- a/test/verify/check-connection
+++ b/test/verify/check-connection
@@ -409,12 +409,13 @@ class TestConnection(MachineCase):
             base64.b64encode(b"admin:foobar").decode(), ))
         headers = m.execute("curl -k --head -b cockpit.jar -s https://127.0.0.1:9090/")
         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",
             ".*Peer sent fatal TLS alert:.*",
             ".*invalid base64 data in Basic header",
@@ -625,10 +626,11 @@ class TestConnection(MachineCase):
         # static handler
         headers = m.execute("curl -s --head http://172.27.0.15:9090/cockpit/static/login.html")
         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])
         self.assertGreater(length, 5000)
         self.assertLess(length, 100000)
diff --git a/test/verify/check-embed b/test/verify/check-embed
index 5d7a370ff..75016cedf 100755
--- a/test/verify/check-embed
+++ b/test/verify/check-embed
@@ -92,23 +92,16 @@ Shell=/shell/index.html
         self.login_and_go()
 
         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__':
     test_main()

Vulnerable code:     Cockpit components can be integrated.</para>
#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 "\"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\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-Content-Type-Options: nosniff\r\nX-DNS-Prefetch-Control: off\r\nReferrer-Policy: no-referrer\r\nCross-Origin-Resource-Policy: same-origin\r\n"
        # CORP is also set for dynamic paths
        # FIXME (#16122): we should not even get that far, frame loading should already be blocked here
        # 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")
Fixed code:     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>
                    "X-Frame-Options": "sameorigin",
                    "X-Frame-Options": "sameorigin",
#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\""
#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\""
    HEADER_X_FRAME_OPTIONS = 1 << 8,
  if (g_ascii_strcasecmp ("X-Frame-Options", name) == 0)
    return HEADER_X_FRAME_OPTIONS;
  /* 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");
#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"
#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"
        # CORP and Frame-Options are also set for dynamic paths
        self.assertIn("X-Frame-Options: sameorigin", headers)
        self.assertIn("X-Frame-Options: sameorigin\r\n", headers)
        # X-Frame-Options sameorigin blocks frame
        if b.cdp.browser == "firefox":
            b.wait_visible("body.neterror")
        self.assertFalse(b.is_present("#login"))