SECURITY: Expand and improve SSRF Protections (#18815)

See https://github.com/discourse/discourse/security/advisories/GHSA-rcc5-28r3-23rr Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com> Co-authored-by: Daniel Waterworth <me@danielwaterworth.com>
2025-02-25 18:55:32 -06:00 · 2022-11-01 16:33:17 +00:00
parent 695b44269b
commit 68b4fe4cf8
42 changed files with 1164 additions and 443 deletions
--- a/app/assets/javascripts/admin/addon/controllers/admin-web-hooks-show.js
+++ b/app/assets/javascripts/admin/addon/controllers/admin-web-hooks-show.js
@@ -3,8 +3,6 @@ import EmberObject from "@ember/object";
 import I18n from "I18n";
 import { alias } from "@ember/object/computed";
 import discourseComputed from "discourse-common/utils/decorators";
-import { extractDomainFromUrl } from "discourse/lib/utilities";
-import { isAbsoluteURL } from "discourse-common/lib/get-url";
 import { isEmpty } from "@ember/utils";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { inject as service } from "@ember/service";
@@ -89,38 +87,20 @@ export default Controller.extend({
  actions: {
    save() {
      this.set("saved", false);
-      const url = this.get("model.payload_url");
-      const domain = extractDomainFromUrl(url);
      const model = this.model;
      const isNew = model.get("isNew");

-      const saveWebHook = () => {
-        return model
-          .save()
-          .then(() => {
-            this.set("saved", true);
-            this.adminWebHooks.get("model").addObject(model);
+      return model
+        .save()
+        .then(() => {
+          this.set("saved", true);
+          this.adminWebHooks.get("model").addObject(model);

-            if (isNew) {
-              this.transitionToRoute("adminWebHooks.show", model.get("id"));
-            }
-          })
-          .catch(popupAjaxError);
-      };
-
-      if (
-        domain === "localhost" ||
-        domain.match(/192\.168\.\d+\.\d+/) ||
-        domain.match(/127\.\d+\.\d+\.\d+/) ||
-        isAbsoluteURL(url)
-      ) {
-        return this.dialog.yesNoConfirm({
-          message: I18n.t("admin.web_hooks.warn_local_payload_url"),
-          didConfirm: () => saveWebHook(),
-        });
-      }
-
-      return saveWebHook();
+          if (isNew) {
+            this.transitionToRoute("adminWebHooks.show", model.get("id"));
+          }
+        })
+        .catch(popupAjaxError);
    },

    destroy() {
--- a/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js
+++ b/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js
@@ -31,6 +31,7 @@ export default Controller.extend(ModalFunctionality, {
  advancedVisible: false,
  selectedType: alias("themesController.currentTab"),
  component: equal("selectedType", COMPONENTS),
+  urlPlaceholder: "https://github.com/discourse/sample_theme",

  init() {
    this._super(...arguments);
@@ -79,29 +80,6 @@ export default Controller.extend(ModalFunctionality, {
    );
  },

-  @discourseComputed("privateChecked")
-  urlPlaceholder(privateChecked) {
-    return privateChecked
-      ? "git@github.com:discourse/sample_theme.git"
-      : "https://github.com/discourse/sample_theme";
-  },
-
-  @observes("privateChecked")
-  privateWasChecked() {
-    const checked = this.privateChecked;
-    if (checked && !this._keyLoading) {
-      this._keyLoading = true;
-      ajax(this.keyGenUrl, { type: "POST" })
-        .then((pair) => {
-          this.set("publicKey", pair.public_key);
-        })
-        .catch(popupAjaxError)
-        .finally(() => {
-          this._keyLoading = false;
-        });
-    }
-  },
-
  @discourseComputed("name")
  nameTooShort(name) {
    return !name || name.length < MIN_NAME_LENGTH;
@@ -116,6 +94,22 @@ export default Controller.extend(ModalFunctionality, {
    }
  },

+  @observes("checkPrivate")
+  privateWasChecked() {
+    const checked = this.checkPrivate;
+    if (checked && !this._keyLoading && !this.publicKey) {
+      this._keyLoading = true;
+      ajax(this.keyGenUrl, { type: "POST" })
+        .then((pair) => {
+          this.set("publicKey", pair.public_key);
+        })
+        .catch(popupAjaxError)
+        .finally(() => {
+          this._keyLoading = false;
+        });
+    }
+  },
+
  @discourseComputed("selection", "themeCannotBeInstalled")
  submitLabel(selection, themeCannotBeInstalled) {
    if (themeCannotBeInstalled) {
@@ -127,15 +121,14 @@ export default Controller.extend(ModalFunctionality, {
    }`;
  },

-  @discourseComputed("privateChecked", "checkPrivate", "publicKey")
-  showPublicKey(privateChecked, checkPrivate, publicKey) {
-    return privateChecked && checkPrivate && publicKey;
+  @discourseComputed("checkPrivate", "publicKey")
+  showPublicKey(checkPrivate, publicKey) {
+    return checkPrivate && publicKey;
  },

  onClose() {
    this.setProperties({
      duplicateRemoteThemeWarning: null,
-      privateChecked: false,
      localFile: null,
      uploadUrl: null,
      publicKey: null,
@@ -209,11 +202,8 @@ export default Controller.extend(ModalFunctionality, {
        options.data = {
          remote: this.uploadUrl,
          branch: this.branch,
+          public_key: this.publicKey,
        };
-
-        if (this.privateChecked) {
-          options.data.public_key = this.publicKey;
-        }
      }

      // User knows that theme cannot be installed, but they want to continue
--- a/app/assets/javascripts/admin/addon/templates/modal/admin-install-theme.hbs
+++ b/app/assets/javascripts/admin/addon/templates/modal/admin-install-theme.hbs
@@ -61,25 +61,15 @@
            <div class="label">{{i18n "admin.customize.theme.remote_branch"}}</div>
            <Input @value={{this.branch}} placeholder="main" />
          </div>
+        {{/if}}

-          <div class="check-private">
-            <label>
-              <Input @type="checkbox" @checked={{this.privateChecked}} />
-              {{i18n "admin.customize.theme.is_private"}}
-            </label>
-          </div>
-          {{#if this.showPublicKey}}
-            <div class="public-key">
-              <div class="label">{{i18n "admin.customize.theme.public_key"}}</div>
-              <div class="public-key-text-wrapper">
-                <Textarea class="public-key-value" readonly={{true}} @value={{this.publicKey}} />                <CopyButton @selector="textarea.public-key-value" />
-              </div>
+        {{#if this.showPublicKey}}
+          <div class="public-key">
+            <div class="label">{{i18n "admin.customize.theme.public_key"}}</div>
+            <div class="public-key-text-wrapper">
+              <Textarea class="public-key-value" readonly={{true}} @value={{this.publicKey}} />                <CopyButton @selector="textarea.public-key-value" />
            </div>
-          {{else}}
-            {{#if this.privateChecked}}
-              <div class="public-key-note">{{i18n "admin.customize.theme.public_key_note"}}</div>
-            {{/if}}
-          {{/if}}
+          </div>
        {{/if}}
      </div>
    {{/if}}
--- a/app/assets/javascripts/discourse/tests/acceptance/admin-install-theme-modal-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/admin-install-theme-modal-test.js
@@ -8,7 +8,6 @@ acceptance("Admin - Themes - Install modal", function (needs) {
  test("closing the modal resets the modal inputs", async function (assert) {
    const urlInput = ".install-theme-content .repo input";
    const branchInput = ".install-theme-content .branch input";
-    const privateRepoCheckbox = ".install-theme-content .check-private input";
    const publicKey = ".install-theme-content .public-key textarea";

    const themeUrl = "git@github.com:discourse/discourse.git";
@@ -19,17 +18,12 @@ acceptance("Admin - Themes - Install modal", function (needs) {
    await fillIn(urlInput, themeUrl);
    await click(".install-theme-content .inputs .advanced-repo");
    await fillIn(branchInput, "tests-passed");
-    await click(privateRepoCheckbox);
    assert.strictEqual(query(urlInput).value, themeUrl, "url input is filled");
    assert.strictEqual(
      query(branchInput).value,
      "tests-passed",
      "branch input is filled"
    );
-    assert.ok(
-      query(privateRepoCheckbox).checked,
-      "private repo checkbox is checked"
-    );
    assert.ok(query(publicKey), "shows public key");

    await click(".modal-footer .d-modal-cancel");
@@ -38,16 +32,11 @@ acceptance("Admin - Themes - Install modal", function (needs) {
    await click("#remote");
    assert.strictEqual(query(urlInput).value, "", "url input is reset");
    assert.strictEqual(query(branchInput).value, "", "branch input is reset");
-    assert.ok(
-      !query(privateRepoCheckbox).checked,
-      "private repo checkbox unchecked"
-    );
    assert.notOk(query(publicKey), "hide public key");
  });

  test("show public key for valid ssh theme urls", async function (assert) {
    const urlInput = ".install-theme-content .repo input";
-    const privateRepoCheckbox = ".install-theme-content .check-private input";
    const publicKey = ".install-theme-content .public-key textarea";

    // Supports backlog repo ssh url format
@@ -59,12 +48,7 @@ acceptance("Admin - Themes - Install modal", function (needs) {
    await click("#remote");
    await fillIn(urlInput, themeUrl);
    await click(".install-theme-content .inputs .advanced-repo");
-    await click(privateRepoCheckbox);
    assert.strictEqual(query(urlInput).value, themeUrl, "url input is filled");
-    assert.ok(
-      query(privateRepoCheckbox).checked,
-      "private repo checkbox is checked"
-    );
    assert.ok(query(publicKey), "shows public key");

    // Supports AWS CodeCommit style repo URLs
--- a/app/assets/javascripts/discourse/tests/acceptance/themes-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/themes-test.js
@@ -188,7 +188,6 @@ acceptance("Theme", function (needs) {
      "git@github.com:discourse/discourse-inexistent-theme.git"
    );
    await click(".install-theme-content button.advanced-repo");
-    await click(".install-theme-content .check-private input");

    assert.notOk(
      exists(".admin-install-theme-modal .modal-footer .install-theme-warning"),
--- a/app/controllers/admin/themes_controller.rb
+++ b/app/controllers/admin/themes_controller.rb
@@ -102,8 +102,10 @@ class Admin::ThemesController < Admin::AdminController
        private_key = params[:public_key] ? Discourse.redis.get("ssh_key_#{params[:public_key]}") : nil
        return render_json_error I18n.t("themes.import_error.ssh_key_gone") if params[:public_key].present? && private_key.blank?

-        @theme = RemoteTheme.import_theme(remote, theme_user, private_key: private_key, branch: branch)
-        render json: @theme, status: :created
+        hijack do
+          @theme = RemoteTheme.import_theme(remote, theme_user, private_key: private_key, branch: branch)
+          render json: @theme, status: :created
+        end
      rescue RemoteTheme::ImportError => e
        if params[:force]
          theme_name = params[:remote].gsub(/.git$/, "").split("/").last
--- a/app/controllers/admin/web_hooks_controller.rb
+++ b/app/controllers/admin/web_hooks_controller.rb
@@ -84,22 +84,12 @@ class Admin::WebHooksController < Admin::AdminController
  end

  def redeliver_event
-    web_hook_event = WebHookEvent.find(params[:event_id])
+    web_hook_event = WebHookEvent.find_by(id: params[:event_id])

    if web_hook_event
      web_hook = web_hook_event.web_hook
-      conn = Excon.new(URI(web_hook.payload_url).to_s,
-                       ssl_verify_peer: web_hook.verify_certificate,
-                       retry_limit: 0)
-
-      now = Time.zone.now
-      response = conn.post(headers: MultiJson.load(web_hook_event.headers), body: web_hook_event.payload)
-      web_hook_event.update!(
-        status: response.status,
-        response_headers: MultiJson.dump(response.headers),
-        response_body: response.body,
-        duration: ((Time.zone.now - now) * 1000).to_i
-      )
+      emitter = WebHookEmitter.new(web_hook, web_hook_event)
+      emitter.emit!(headers: MultiJson.load(web_hook_event.headers), body: web_hook_event.payload)
      render_serialized(web_hook_event, AdminWebHookEventSerializer, root: 'web_hook_event')
    else
      render json: failed_json
--- a/app/jobs/regular/emit_web_hook_event.rb
+++ b/app/jobs/regular/emit_web_hook_event.rb
@@ -9,7 +9,6 @@ module Jobs
    PING_EVENT = 'ping'
    MAX_RETRY_COUNT = 4
    RETRY_BACKOFF = 5
-    REQUEST_TIMEOUT = 20

    def execute(args)
      @arguments = args
@@ -43,39 +42,13 @@ module Jobs
    end

    def send_webhook!
-      uri = URI(@web_hook.payload_url.strip)
-      conn = Excon.new(
-        uri.to_s,
-        ssl_verify_peer: @web_hook.verify_certificate,
-        retry_limit: 0,
-        write_timeout: REQUEST_TIMEOUT,
-        read_timeout: REQUEST_TIMEOUT,
-        connect_timeout: REQUEST_TIMEOUT
-      )
-
      web_hook_body = build_webhook_body
      web_hook_event = create_webhook_event(web_hook_body)
+      uri = URI(@web_hook.payload_url.strip)
      web_hook_headers = build_webhook_headers(uri, web_hook_body, web_hook_event)
-      web_hook_response = nil

-      begin
-        now = Time.zone.now
-        web_hook_response = conn.post(headers: web_hook_headers, body: web_hook_body)
-        web_hook_event.update!(
-          headers: MultiJson.dump(web_hook_headers),
-          status: web_hook_response.status,
-          response_headers: MultiJson.dump(web_hook_response.headers),
-          response_body: web_hook_response.body,
-          duration: ((Time.zone.now - now) * 1000).to_i
-        )
-      rescue => e
-        web_hook_event.update!(
-          headers: MultiJson.dump(web_hook_headers),
-          status: -1,
-          response_headers: MultiJson.dump(error: e),
-          duration: ((Time.zone.now - now) * 1000).to_i
-        )
-      end
+      emitter = WebHookEmitter.new(@web_hook, web_hook_event)
+      web_hook_response = emitter.emit!(headers: web_hook_headers, body: web_hook_body)

      publish_webhook_event(web_hook_event)
      process_webhook_response(web_hook_response)
@@ -151,12 +124,12 @@ module Jobs
      headers = {
        'Accept' => '*/*',
        'Connection' => 'close',
-        'Content-Length' => web_hook_body.bytesize,
+        'Content-Length' => web_hook_body.bytesize.to_s,
        'Content-Type' => content_type,
        'Host' => uri.host,
        'User-Agent' => "Discourse/#{Discourse::VERSION::STRING}",
        'X-Discourse-Instance' => Discourse.base_url,
-        'X-Discourse-Event-Id' => web_hook_event.id,
+        'X-Discourse-Event-Id' => web_hook_event.id.to_s,
        'X-Discourse-Event-Type' => @arguments[:event_type]
      }

--- a/app/models/remote_theme.rb
+++ b/app/models/remote_theme.rb
@@ -15,7 +15,7 @@ class RemoteTheme < ActiveRecord::Base
  ALLOWED_FIELDS = %w{scss embedded_scss head_tag header after_header body_tag footer}

  GITHUB_REGEXP = /^https?:\/\/github\.com\//
-  GITHUB_SSH_REGEXP = /^git@github\.com:/
+  GITHUB_SSH_REGEXP = /^ssh:\/\/git@github\.com:/

  has_one :theme, autosave: false
  scope :joined_remotes, -> {
@@ -25,8 +25,10 @@ class RemoteTheme < ActiveRecord::Base
  validates_format_of :minimum_discourse_version, :maximum_discourse_version, with: Discourse::VERSION_REGEXP, allow_nil: true

  def self.extract_theme_info(importer)
-    JSON.parse(importer["about.json"])
-  rescue TypeError, JSON::ParserError
+    json = JSON.parse(importer["about.json"])
+    json.fetch("name")
+    json
+  rescue TypeError, JSON::ParserError, KeyError
    raise ImportError.new I18n.t("themes.import_error.about_json")
  end

@@ -80,6 +82,7 @@ class RemoteTheme < ActiveRecord::Base
    importer.import!

    theme_info = RemoteTheme.extract_theme_info(importer)
+
    component = [true, "true"].include?(theme_info["component"])
    theme = Theme.new(user_id: user&.id || -1, name: theme_info["name"], component: component)
    theme.child_components = theme_info["components"].presence || []
--- a/app/models/web_hook.rb
+++ b/app/models/web_hook.rb
@@ -15,6 +15,7 @@ class WebHook < ActiveRecord::Base
  validates_presence_of :content_type
  validates_presence_of :last_delivery_status
  validates_presence_of :web_hook_event_types, unless: :wildcard_web_hook?
+  validate :ensure_payload_url_allowed, if: :payload_url_changed?

  before_save :strip_url

@@ -113,6 +114,23 @@ class WebHook < ActiveRecord::Base
  def self.guardian
    Guardian.new(Discourse.system_user)
  end
+
+  # This check is to improve UX
+  # IPs are re-checked at request time
+  def ensure_payload_url_allowed
+    return if payload_url.blank?
+    uri = URI(payload_url.strip)
+
+    allowed = begin
+      FinalDestination::SSRFDetector.lookup_and_filter_ips(uri.hostname).present?
+    rescue FinalDestination::SSRFDetector::DisallowedIpError
+      false
+    end
+
+    if !allowed
+      self.errors.add(:base, I18n.t("webhooks.payload_url.blocked_or_internal"))
+    end
+  end
 end

 # == Schema Information
--- a/app/services/web_hook_emitter.rb
+++ b/app/services/web_hook_emitter.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+class WebHookEmitter
+  REQUEST_TIMEOUT = 20
+
+  def initialize(webhook, webhook_event)
+    @webhook = webhook
+    @webhook_event = webhook_event
+  end
+
+  def emit!(headers:, body:)
+    uri = URI(@webhook.payload_url.strip)
+
+    connection_opts = {
+      request: {
+        write_timeout: REQUEST_TIMEOUT,
+        read_timeout: REQUEST_TIMEOUT,
+        open_timeout: REQUEST_TIMEOUT
+      },
+    }
+
+    if !@webhook.verify_certificate
+      connection_opts[:ssl] = { verify: false }
+    end
+
+    conn = Faraday.new(nil, connection_opts) do |f|
+      f.adapter FinalDestination::FaradayAdapter
+    end
+
+    start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond)
+    error = nil
+    response = nil
+    begin
+      response = conn.post(uri.to_s, body, headers)
+    rescue => e
+      error = e
+    end
+    duration = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) - start
+    event_update_args = {
+      headers: MultiJson.dump(headers),
+      duration: duration,
+    }
+    if response
+      event_update_args[:response_headers] = MultiJson.dump(response.headers)
+      event_update_args[:response_body] = response.body
+      event_update_args[:status] = response.status
+    else
+      event_update_args[:status] = -1
+      if error.is_a?(Faraday::Error) && error.wrapped_exception.is_a?(FinalDestination::SSRFDetector::DisallowedIpError)
+        error = I18n.t("webhooks.payload_url.blocked_or_internal")
+      end
+      event_update_args[:response_headers] = MultiJson.dump(error: error)
+    end
+    @webhook_event.update!(**event_update_args)
+    response
+  end
+end