[2.2] Restrict chromium requests (opensearch-project#431)

* Fix regex validation, detect iframe, embed, object tags Signed-off-by: Joshua Li <[email protected]> * Disallow redirection to non-localhost urls Signed-off-by: Joshua Li <[email protected]> * Disallow connection to non-allowlisted urls Signed-off-by: Joshua Li <[email protected]> * Disable JIT Signed-off-by: Joshua Li <[email protected]> * Fix workflow Signed-off-by: Joshua Li <[email protected]> * Try to fix CI Signed-off-by: Joshua Li <[email protected]> * Fix localstorage logic Signed-off-by: Joshua Li <[email protected]> Signed-off-by: Joshua Li <[email protected]>
kavilla · Aug 18, 2022 · 5ede578 · 5ede578
1 parent 1597e1f
commit 5ede578
Show file tree

Hide file tree

Showing 7 changed files with 125 additions and 24 deletions.
diff --git a/.github/workflows/dashboards-reports-test-and-build-workflow.yml b/.github/workflows/dashboards-reports-test-and-build-workflow.yml
@@ -20,12 +20,12 @@ jobs:
         with:
           repository: opensearch-project/Opensearch-Dashboards
           ref: ${{ env.OPENSEARCH_VERSION }}
-          path: dashboards-reports/OpenSearch-Dashboards
+          path: OpenSearch-Dashboards
 
       - name: Get node version
         id: versions_step
         run:
-          echo "::set-output name=node_version::$(node -p "(require('./OpenSearch-Dashboards/package.json').engines.node).match(/[.0-9]+/)[0]")"
+          echo "::set-output name=node_version::$(node -p "(require('../OpenSearch-Dashboards/package.json').engines.node).match(/[.0-9]+/)[0]")"
 
       - name: Setup Node
         uses: actions/setup-node@v1
@@ -35,13 +35,13 @@ jobs:
 
 
       - name: Move Dashboards Reports to Plugins Dir
-        run: mv dashboards-reports OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
+        run: mv dashboards-reports ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
 
       - name: Add Chromium Binary to Reporting for Testing
         run: |
           sudo apt update
           sudo apt install -y libnss3-dev fonts-liberation libfontconfig1
-          cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
+          cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
           wget https://github.com/opendistro-for-elasticsearch/kibana-reports/releases/download/chromium-1.12.0.0/chromium-linux-x64.zip 
           unzip chromium-linux-x64.zip
           rm chromium-linux-x64.zip
@@ -51,25 +51,25 @@ jobs:
         with:
           timeout_minutes: 30
           max_attempts: 3
-          command: cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn osd bootstrap
+          command: cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn osd bootstrap
 
       - name: Test
         uses: nick-invision/retry@v1
         with:
           timeout_minutes: 30
           max_attempts: 3
-          command: cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn test --coverage
+          command: cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn test --coverage
 
       - name: Upload coverage
         uses: codecov/codecov-action@v1
         with:
           flags: dashboards-reports
-          directory: OpenSearch-Dashboards/plugins/
+          directory: ../OpenSearch-Dashboards/plugins/
           token: ${{ secrets.CODECOV_TOKEN }}
 
       - name: Build Artifact
         run: |
-          cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
+          cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
           yarn build
 
           cd build
@@ -103,16 +103,16 @@ jobs:
         uses: actions/upload-artifact@v1
         with:
           name: dashboards-reports-linux-x64
-          path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-x64.zip
+          path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-x64.zip
 
       - name: Upload Artifact For Linux arm64
         uses: actions/upload-artifact@v1
         with:
           name: dashboards-reports-linux-arm64
-          path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-arm64.zip
+          path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-arm64.zip
 
       - name: Upload Artifact For Windows
         uses: actions/upload-artifact@v1
         with:
           name: dashboards-reports-windows-x64
-          path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-windows-x64.zip
+          path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-windows-x64.zip
diff --git a/server/routes/utils/__tests__/visualReportHelper.test.ts b/server/routes/utils/__tests__/visualReportHelper.test.ts
@@ -55,7 +55,9 @@ describe('test create visual report', () => {
       reportParams as ReportParamsSchemaType,
       mockHtmlPath,
       mockLogger,
-      mockHeader
+      mockHeader,
+      undefined,
+      /^(data:image|file:\/\/)/
     );
     expect(fileName).toContain(`${reportParams.report_name}`);
     expect(fileName).toContain('.png');
@@ -71,7 +73,9 @@ describe('test create visual report', () => {
       reportParams as ReportParamsSchemaType,
       mockHtmlPath,
       mockLogger,
-      mockHeader
+      mockHeader,
+      undefined,
+      /^(data:image|file:\/\/)/
     );
     expect(fileName).toContain(`${reportParams.report_name}`);
     expect(fileName).toContain('.pdf');

diff --git a/server/routes/utils/constants.ts b/server/routes/utils/constants.ts
@@ -93,6 +93,8 @@ const ipv6Regex = /(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:)
 const localhostRegex = /localhost:([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])/g;
 const iframeRegex = /iframe/g;
 
+export const ALLOWED_HOSTS = /^(0|0.0.0.0|127.0.0.1|localhost|(.*\.)?(opensearch.org|aws.a2z.com))$/;
+
 export const replaceBlockedKeywords = (htmlString: string) => {
   // replace <ipv4>:<port>
   htmlString = htmlString.replace(ipv4Regex, BLOCKED_KEYWORD);

diff --git a/server/routes/utils/visual_report/style.css b/server/routes/utils/visual_report/style.css
@@ -4,6 +4,10 @@ body {
   padding: 0;
 }
 
+iframe, embed, object {
+  display: none !important;
+}
+
 /*  nice padding + matches Kibana default UI colors you could also set this to inherit if
       the wrapper gets inserted inside a kibana section. I might also remove the manual text color here as well, potentially */
 .reportWrapper {

diff --git a/server/routes/utils/visual_report/visualReportHelper.ts b/server/routes/utils/visual_report/visualReportHelper.ts
@@ -14,6 +14,7 @@ import {
   SELECTOR,
   CHROMIUM_PATH,
   SECURITY_CONSTANTS,
+  ALLOWED_HOSTS,
 } from '../constants';
 import { getFileName } from '../helpers';
 import { CreateReportResultType } from '../types';
@@ -27,7 +28,8 @@ export const createVisualReport = async (
   queryUrl: string,
   logger: Logger,
   extraHeaders: Headers,
-  timezone?: string
+  timezone?: string,
+  validRequestProtocol = /^(data:image)/,
 ): Promise<CreateReportResultType> => {
   const {
     core_params,
@@ -76,6 +78,8 @@ export const createVisualReport = async (
       '--no-zygote',
       '--single-process',
       '--font-render-hinting=none',
+      '--js-flags="--jitless --no-opt"',
+      '--disable-features=V8OptimizeJavascript',
     ],
     executablePath: CHROMIUM_PATH,
     ignoreHTTPSErrors: true,
@@ -84,6 +88,32 @@ export const createVisualReport = async (
     },
   });
   const page = await browser.newPage();
+
+  await page.setRequestInterception(true);
+  let localStorageAvailable = true;
+  page.on('request', (req) => {
+    // disallow non-allowlisted connections. urls with valid protocols do not need ALLOWED_HOSTS check
+    if (
+      !validRequestProtocol.test(req.url()) &&
+      !ALLOWED_HOSTS.test(new URL(req.url()).hostname)
+    ) {
+      if (req.isNavigationRequest() && req.redirectChain().length > 0) {
+        localStorageAvailable = false;
+        logger.error(
+          'Reporting does not allow redirections to outside of localhost, aborting. URL received: ' +
+            req.url()
+        );
+      } else {
+        logger.warn(
+          'Disabled connection to non-allowlist domains: ' + req.url()
+        );
+      }
+      req.abort();
+    } else {
+      req.continue();
+    }
+  });
+
   page.setDefaultNavigationTimeout(0);
   page.setDefaultTimeout(100000); // use 100s timeout instead of default 30s
   // Set extra headers that are needed
@@ -93,13 +123,25 @@ export const createVisualReport = async (
   logger.info(`original queryUrl ${queryUrl}`);
   await page.goto(queryUrl, { waitUntil: 'networkidle0' });
   // should add to local storage after page.goto, then access the page again - browser must have an url to register local storage item on it
-  await page.evaluate(
-    /* istanbul ignore next */
-    (key) => {
-      localStorage.setItem(key, 'false');
-    },
-    SECURITY_CONSTANTS.TENANT_LOCAL_STORAGE_KEY
-  );
+  try {
+    await page.evaluate(
+      /* istanbul ignore next */
+      (key) => {
+        try {
+          if (
+            localStorageAvailable &&
+            typeof localStorage !== 'undefined' &&
+            localStorage !== null
+          ) {
+            localStorage.setItem(key, 'false');
+          }
+        } catch (err) {}
+      },
+      SECURITY_CONSTANTS.TENANT_LOCAL_STORAGE_KEY
+    );
+  } catch (err) {
+    logger.error(err);
+  }
   await page.goto(queryUrl, { waitUntil: 'networkidle0' });
   logger.info(`page url ${page.url()}`);
 
@@ -162,9 +204,24 @@ export const createVisualReport = async (
   // wait for dynamic page content to render
   await waitForDynamicContent(page);
 
-  await addReportStyle(page);
   await addReportHeader(page, keywordFilteredHeader);
   await addReportFooter(page, keywordFilteredFooter);
+  await addReportStyle(page);
+
+  // this causes UT to fail in github CI but works locally
+  try {
+    const numDisallowedTags = await page.evaluate(
+      () =>
+        document.getElementsByTagName('iframe').length +
+        document.getElementsByTagName('embed').length +
+        document.getElementsByTagName('object').length
+    );
+    if (numDisallowedTags > 0) {
+      throw Error('Reporting does not support "iframe", "embed", or "object" tags, aborting');
+    }
+  } catch (error) {
+    logger.error(error);
+  }
 
   // create pdf or png accordingly
   if (reportFormat === FORMAT.pdf) {

diff --git a/server/utils/__tests__/validationHelper.test.ts b/server/utils/__tests__/validationHelper.test.ts
@@ -10,7 +10,7 @@ import {
   REPORT_TYPE,
   TRIGGER_TYPE,
 } from '../../routes/utils/constants';
-import { validateReport, validateReportDefinition } from '../validationHelper';
+import { isValidRelativeUrl, validateReport, validateReportDefinition } from '../validationHelper';
 
 const SAMPLE_SAVED_OBJECT_ID = '3ba638e0-b894-11e8-a6d9-e546fe2bba5f';
 const createReportDefinitionInput: ReportDefinitionSchemaType = {
@@ -152,7 +152,41 @@ describe('test input validation', () => {
       `saved object with id dashboard:${SAMPLE_SAVED_OBJECT_ID} does not exist`
     );
   });
+
+  test('validation against query_url', async () => {
+    const urls: [string, boolean][] = [
+      ['/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=', true],
+      [
+        '/_plugin/kibana/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
+        true,
+      ],
+      [
+        '/_dashboards/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
+        true,
+      ],
+      [
+        '/_dashboards/app/dashboards#/edit/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
+        true,
+      ],
+      [
+        '/app/observability-dashboards?security_tenant=private#/notebooks/NYdlPIIB0-fJ8Bh1nLdW?view=output_only',
+        true,
+      ],
+      [
+        '/app/notebooks-dashboards?view=output_only&security_tenant=private#/M4dlPIIB0-fJ8Bh1nLc7?security_tenant=private',
+        true,
+      ],
+      [
+        '/_dashboards/app/visualize&security_tenant=/.%2e/.%2e/.%2e/.%2e/_dashboards?#/view/id',
+        false,
+      ],
+    ];
+    expect(urls.map((url) => isValidRelativeUrl(url[0]))).toEqual(
+      urls.map((url) => url[1])
+    );
+  });
 });
+
 // TODO: merge this with other mock clients used in testing, to create some mock helpers file
 const mockOpenSearchClient = (mockSavedObjectIds: string[]) => {
   const client = {

diff --git a/server/utils/validationHelper.ts b/server/utils/validationHelper.ts
@@ -37,7 +37,7 @@ export const isValidRelativeUrl = (relativeUrl: string) => {
 export const regexDuration = /^(-?)P(?=\d|T\d)(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)([DW]))?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+(?:\.\d+)?)S)?)?$/;
 export const regexEmailAddress = /\S+@\S+\.\S+/;
 export const regexReportName = /^[\w\-\s\(\)\[\]\,\_\-+]+$/;
-export const regexRelativeUrl = /^\/(_plugin\/kibana\/|_dashboards\/)?app\/(dashboards|visualize|discover|observability-dashboards|notebooks-dashboards\?view=output_only)([?&]security_tenant=.+|)#\/(notebooks\/|view\/|edit\/)?[^\/]+$/;
+export const regexRelativeUrl = /^\/(_plugin\/kibana\/|_dashboards\/)?app\/(dashboards|visualize|discover|observability-dashboards|notebooks-dashboards\?view=output_only(&security_tenant=.+)?)(\?security_tenant=.+)?#\/(notebooks\/|view\/|edit\/)?[^\/]+$/;
 
 export const validateReport = async (
   client: ILegacyScopedClusterClient,