SECURITY: Multiple commits for Version bump 3.1.0.beta3 (#20707)

* SECURITY: Fix XSS in full name composer reply

We are using htmlSafe when rendering the name field so we need to escape
any html being passed in.

* SECURITY: Monkey-patch web-push gem to use safer HTTP client

`FinalDestination::HTTP` is our patch of `Net::HTTP` which defend us
against SSRF and DNS rebinding attacks.

* SECURITY: SSRF protection bypass with IPv4-mapped IPv6 addresses

As part of this commit, we've also expanded our list of private IP
ranges based on
https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
and https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml

* SECURITY: XSS on chat excerpts

Non-markdown tags weren't being escaped in chat excerpts. This could be
triggered by editing a chat message containing a tag (self XSS), or by
replying to a chat message with a tag (XSS).

Co-authored-by: Jan Cernik <jancernik12@gmail.com>

* FIX: Escaped mentions in chat excerpts

Mentions are now displayed as using the non-cooked message which fixes
the problem. This is not ideal. I think we might want to rework how
these excerpts are created and rendered in the near future.

Co-authored-by: Jan Cernik <jancernik12@gmail.com>

* SECURITY: Add FinalDestination::FastImage that's SSRF safe

---------

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Co-authored-by: Jan Cernik <jancernik12@gmail.com>
Co-authored-by: Ted Johansson <ted@discourse.org>
This commit is contained in:
Blake Erickson 2023-03-16 14:04:46 -06:00 committed by GitHub
parent 7dd317b875
commit fb019d1712
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 315 additions and 53 deletions

View File

@ -30,7 +30,7 @@
/>
<TopicStatus @topic={{t}} @disableActions={{true}} />
<span class="topic-title">
{{replace-emoji t.fancy_title}}
{{replace-emoji t.title}}
</span>
<span class="topic-categories">
{{bound-category-link

View File

@ -12,6 +12,7 @@ import { alias } from "@ember/object/computed";
import discourseComputed from "discourse-common/utils/decorators";
import { iconHTML } from "discourse-common/lib/icon-library";
import { htmlSafe } from "@ember/template";
import { escape } from "pretty-text/sanitizer";
const TITLES = {
[PRIVATE_MESSAGE]: "topic.private_message",
@ -84,7 +85,9 @@ export default Component.extend({
},
_formatReplyToUserPost(avatar, link) {
const htmlLink = `<a class="user-link" href="${link.href}">${link.anchor}</a>`;
const htmlLink = `<a class="user-link" href="${link.href}">${escape(
link.anchor
)}</a>`;
return htmlSafe(`${avatar}${htmlLink}`);
},
});

View File

@ -1,7 +1,9 @@
import { emojiUnescape } from "discourse/lib/text";
import { htmlSafe } from "@ember/template";
import { htmlSafe, isHTMLSafe } from "@ember/template";
import { registerUnbound } from "discourse-common/lib/helpers";
import { escapeExpression } from "discourse/lib/utilities";
registerUnbound("replace-emoji", (text, options) => {
text = isHTMLSafe(text) ? text.toString() : escapeExpression(text);
return htmlSafe(emojiUnescape(text, options));
});

View File

@ -21,7 +21,7 @@
data-title={{result.fancy_title}}
>
<TopicStatus @topic={{result}} @disableActions={{true}} />
{{replace-emoji result.fancy_title}}
{{replace-emoji result.title}}
<div class="search-category">
{{#if result.category.parentCategory}}
{{category-link result.category.parentCategory}}

View File

@ -16,7 +16,7 @@
href={{rt.relative_url}}
rel="noopener noreferrer"
target="_blank"
>{{replace-emoji rt.fancy_title}}</a>
>{{replace-emoji rt.title}}</a>
</div>
</td>
<td class="reviewable-count">

View File

@ -518,11 +518,11 @@ acceptance("Prioritize Full Name", function (needs) {
test("Reply to post use full name", async function (assert) {
await visit("/t/short-topic-with-two-posts/54079");
await click("article#post_2 button.reply");
await click("article#post_3 button.reply");
assert.strictEqual(
query(".action-title .user-link").innerText.trim(),
"james, john, the third"
query(".action-title .user-link").innerHTML.trim(),
"&lt;h1&gt;Tim Stone&lt;/h1&gt;"
);
});

View File

@ -6497,7 +6497,7 @@ export default {
},
{
id: 419,
name: "Tim Stone",
name: "<h1>Tim Stone</h1>",
username: "tms",
avatar_template: "/letter_avatar_proxy/v4/letter/t/3be4f8/{size}.png",
uploaded_avatar_id: 40181,

View File

@ -0,0 +1,29 @@
import { module, test } from "qunit";
import { setupRenderingTest } from "discourse/tests/helpers/component-test";
import { render } from "@ember/test-helpers";
import { hbs } from "ember-cli-htmlbars";
module("Integration | Helper | replace-emoji", function (hooks) {
setupRenderingTest(hooks);
test("it replaces the emoji", async function (assert) {
await render(hbs`<span>{{replace-emoji "some text :heart:"}}</span>`);
assert.dom(`span`).includesText("some text");
assert.dom(`.emoji[title="heart"]`).exists();
});
test("it escapes the text", async function (assert) {
await render(
hbs`<span>{{replace-emoji "<style>body: {background: red;}</style>"}}</span>`
);
assert.dom(`span`).hasText("<style>body: {background: red;}</style>");
});
test("it renders html-safe text", async function (assert) {
await render(hbs`<span>{{replace-emoji (html-safe "safe text")}}</span>`);
assert.dom(`span`).hasText("safe text");
});
});

View File

@ -1,5 +1,5 @@
<TopicStatus @topic={{this.item}} @disableActions={{true}} />
<div class="topic-title">{{replace-emoji this.item.fancy_title}}</div>
<div class="topic-title">{{replace-emoji this.item.title}}</div>
<div class="topic-categories">
{{bound-category-link
this.item.category

View File

@ -193,7 +193,7 @@ module CookedProcessorMixin
if upload && upload.width && upload.width > 0
@size_cache[url] = [upload.width, upload.height]
else
@size_cache[url] = FastImage.size(absolute_url)
@size_cache[url] = FinalDestination::FastImage.size(absolute_url)
end
rescue Zlib::BufError, URI::Error, OpenSSL::SSL::SSLError
# FastImage.size raises BufError for some gifs, leave it.

View File

@ -0,0 +1,23 @@
# frozen_string_literal: true
class FinalDestination::FastImage < ::FastImage
def initialize(url, options = {})
uri = URI(normalized_url(url))
options.merge!(http_header: { "Host" => uri.hostname })
uri.hostname = resolved_ip(uri)
super(uri.to_s, options)
rescue FinalDestination::SSRFDetector::DisallowedIpError, SocketError, Timeout::Error
super("")
end
private
def resolved_ip(uri)
FinalDestination::SSRFDetector.lookup_and_filter_ips(uri.hostname).first
end
def normalized_url(uri)
UrlHelper.normalized_encode(uri)
end
end

View File

@ -7,18 +7,47 @@ class FinalDestination
class LookupFailedError < SocketError
end
def self.standard_private_ranges
@private_ranges ||= [
IPAddr.new("0.0.0.0/8"),
IPAddr.new("127.0.0.1"),
IPAddr.new("172.16.0.0/12"),
IPAddr.new("192.168.0.0/16"),
IPAddr.new("10.0.0.0/8"),
IPAddr.new("::1"),
IPAddr.new("fc00::/7"),
IPAddr.new("fe80::/10"),
]
end
# This is a list of private IPv4 IP ranges that are not allowed to be globally reachable as given by
# https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml.
PRIVATE_IPV4_RANGES = [
IPAddr.new("0.0.0.0/8"),
IPAddr.new("10.0.0.0/8"),
IPAddr.new("100.64.0.0/10"),
IPAddr.new("127.0.0.0/8"),
IPAddr.new("169.254.0.0/16"),
IPAddr.new("172.16.0.0/12"),
IPAddr.new("192.0.0.0/24"),
IPAddr.new("192.0.0.0/29"),
IPAddr.new("192.0.0.8/32"),
IPAddr.new("192.0.0.170/32"),
IPAddr.new("192.0.0.171/32"),
IPAddr.new("192.0.2.0/24"),
IPAddr.new("192.168.0.0/16"),
IPAddr.new("192.175.48.0/24"),
IPAddr.new("198.18.0.0/15"),
IPAddr.new("198.51.100.0/24"),
IPAddr.new("203.0.113.0/24"),
IPAddr.new("240.0.0.0/4"),
IPAddr.new("255.255.255.255/32"),
]
# This is a list of private IPv6 IP ranges that are not allowed to be globally reachable as given by
# https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml.
#
# ::ffff:0:0/96 is excluded from the list because it is used for IPv4-mapped IPv6 addresses which is something we want to allow.
PRIVATE_IPV6_RANGES = [
IPAddr.new("::1/128"),
IPAddr.new("::/128"),
IPAddr.new("64:ff9b:1::/48"),
IPAddr.new("100::/64"),
IPAddr.new("2001::/23"),
IPAddr.new("2001:2::/48"),
IPAddr.new("2001:db8::/32"),
IPAddr.new("fc00::/7"),
IPAddr.new("fe80::/10"),
]
PRIVATE_IP_RANGES = PRIVATE_IPV4_RANGES + PRIVATE_IPV6_RANGES
def self.blocked_ip_blocks
SiteSetting
@ -54,10 +83,9 @@ class FinalDestination
def self.ip_allowed?(ip)
ip = ip.is_a?(IPAddr) ? ip : IPAddr.new(ip)
ip = ip.native
if ip_in_ranges?(ip, blocked_ip_blocks) || ip_in_ranges?(ip, standard_private_ranges)
return false
end
return false if ip_in_ranges?(ip, blocked_ip_blocks) || ip_in_ranges?(ip, PRIVATE_IP_RANGES)
true
end

View File

@ -0,0 +1,27 @@
# frozen_string_literal: true
# This is a patch to avoid the direct use of `Net::HTTP` in the `webpush` gem and instead rely on `FinalDestination::HTTP`
# which protects us from DNS rebinding attacks as well as server side forgery requests.
#
# This patch is considered temporary until we can decide on a longer term solution. In the meantime, we need to patch
# the SSRF vulnerability being exposed by this gem.
module WebPushPatch
def perform
http = FinalDestination::HTTP.new(uri.host, uri.port, *proxy_options)
http.use_ssl = true
http.ssl_timeout = @options[:ssl_timeout] unless @options[:ssl_timeout].nil?
http.open_timeout = @options[:open_timeout] unless @options[:open_timeout].nil?
http.read_timeout = @options[:read_timeout] unless @options[:read_timeout].nil?
req = FinalDestination::HTTP::Post.new(uri.request_uri, headers)
req.body = body
resp = http.request(req)
verify_response(resp)
resp
end
end
klass = defined?(WebPush) ? WebPush : Webpush
klass::Request.prepend(WebPushPatch)

View File

@ -90,7 +90,7 @@ class ChatMessage < ActiveRecord::Base
return uploads.first.original_filename if cooked.blank? && uploads.present?
# this may return blank for some complex things like quotes, that is acceptable
PrettyText.excerpt(cooked, max_length, { text_entities: true })
PrettyText.excerpt(message, max_length, { text_entities: true })
end
def cooked_for_excerpt

View File

@ -31,7 +31,7 @@
</label>
<div class="chat-form__control">
<div class="channel-info-about-view__name">
{{replace-emoji this.channel.escapedTitle}}
{{replace-emoji this.channel.title}}
</div>
<div class="channel-info-about-view__slug">
{{this.channel.slug}}

View File

@ -15,7 +15,7 @@
class="chat-channel-card__name-container"
>
<span class="chat-channel-card__name">
{{replace-emoji @channel.escapedTitle}}
{{replace-emoji @channel.title}}
</span>
{{#if @channel.chatable.read_restricted}}
{{d-icon "lock" class="chat-channel-card__read-restricted"}}
@ -47,7 +47,7 @@
{{#if @channel.description}}
<div class="chat-channel-card__description">
{{replace-emoji @channel.escapedDescription}}
{{replace-emoji @channel.description}}
</div>
{{/if}}

View File

@ -59,7 +59,7 @@
{{/if}}
</span>
<span class="chat-channel-title__name">
{{replace-emoji this.channel.escapedTitle}}
{{replace-emoji this.channel.title}}
</span>
{{#if (has-block)}}

View File

@ -287,24 +287,6 @@ describe ChatMessage do
COOKED
)
expect(message.excerpt).to eq("https://twitter.com/EffinBirds/status/1518743508378697729")
message =
Fabricate.build(
:chat_message,
message:
"wow check out these birbs https://twitter.com/EffinBirds/status/1518743508378697729",
)
expect(message.excerpt).to eq(
"wow check out these birbs <a href=\"https://twitter.com/EffinBirds/status/1518743508378697729\" class=\"inline-onebox-loading\" rel=\"noopener nofollow ugc\">https://twitter.com/Effi...</a>",
)
end
it "returns an empty string if PrettyText.excerpt returns empty string" do
message = Fabricate(:chat_message, message: <<~MSG)
[quote="martin, post:30, topic:3179, full:true"]
This is a real **quote** topic with some *markdown* in it I can quote.
[/quote]
MSG
expect(message.excerpt).to eq("")
end
it "excerpts upload file name if message is empty" do

View File

@ -189,6 +189,34 @@ RSpec.describe "Chat channel", type: :system, js: true do
end
end
context "when replying to message that has tags" do
fab!(:other_user) { Fabricate(:user) }
fab!(:message_2) do
Fabricate(
:chat_message,
user: other_user,
chat_channel: channel_1,
message: "<mark>not marked</mark>",
)
end
before do
Fabricate(:chat_message, user: other_user, chat_channel: channel_1)
Fabricate(:chat_message, in_reply_to: message_2, user: current_user, chat_channel: channel_1)
channel_1.add(other_user)
channel_1.add(current_user)
sign_in(current_user)
end
it "escapes the reply-to line" do
chat.visit_channel(channel_1)
expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq(
"&lt;mark&gt;not marked&lt;/mark&gt;",
)
end
end
context "when messages are separated by a day" do
before do
Fabricate(:chat_message, chat_channel: channel_1, created_at: 2.days.ago)

View File

@ -0,0 +1,70 @@
# frozen_string_literal: true
describe FinalDestination::FastImage do
before do
# We need to test low-level stuff, switch off WebMock for FastImage
WebMock.enable!(except: [:net_http])
Socket.stubs(:tcp).never
TCPSocket.stubs(:open).never
Addrinfo.stubs(:getaddrinfo).never
end
after { WebMock.enable! }
def expect_tcp_and_abort(stub_addr, &blk)
success = Class.new(StandardError)
TCPSocket.stubs(:open).with { |addr| stub_addr == addr }.once.raises(success)
begin
yield
rescue success
end
end
def stub_ip_lookup(stub_addr, ips)
FinalDestination::SSRFDetector.stubs(:lookup_ips).with { |addr| stub_addr == addr }.returns(ips)
end
def stub_tcp_to_raise(stub_addr, exception)
TCPSocket.stubs(:open).with { |addr| addr == stub_addr }.once.raises(exception)
end
it "uses the first resolved IP" do
stub_ip_lookup("example.com", %w[1.1.1.1 2.2.2.2 3.3.3.3])
expect_tcp_and_abort("1.1.1.1") do
FinalDestination::FastImage.size(URI("https://example.com/img.jpg"))
end
end
it "ignores private IPs" do
stub_ip_lookup("example.com", %w[0.0.0.0 2.2.2.2])
expect_tcp_and_abort("2.2.2.2") do
FinalDestination::FastImage.size(URI("https://example.com/img.jpg"))
end
end
it "returns a null object when all IPs are private" do
stub_ip_lookup("example.com", %w[0.0.0.0 127.0.0.1])
expect(FinalDestination::FastImage.size(URI("https://example.com/img.jpg"))).to eq(nil)
end
it "returns a null object if all IPs are blocked" do
SiteSetting.blocked_ip_blocks = "98.0.0.0/8|78.13.47.0/24|9001:82f3::/32"
stub_ip_lookup("ip6.example.com", %w[9001:82f3:8873::3])
stub_ip_lookup("ip4.example.com", %w[98.23.19.111])
expect(FinalDestination::FastImage.size(URI("https://ip4.example.com/img.jpg"))).to eq(nil)
expect(FinalDestination::FastImage.size(URI("https://ip6.example.com/img.jpg"))).to eq(nil)
end
it "allows specified hosts to bypass IP checks" do
SiteSetting.blocked_ip_blocks = "98.0.0.0/8|78.13.47.0/24|9001:82f3::/32"
SiteSetting.allowed_internal_hosts = "internal.example.com|blocked-ip.example.com"
stub_ip_lookup("internal.example.com", %w[0.0.0.0 127.0.0.1])
stub_ip_lookup("blocked-ip.example.com", %w[98.23.19.111])
expect_tcp_and_abort("0.0.0.0") do
FinalDestination::FastImage.size(URI("https://internal.example.com/img.jpg"))
end
expect_tcp_and_abort("98.23.19.111") do
FinalDestination::FastImage.size(URI("https://blocked-ip.example.com/img.jpg"))
end
end
end

View File

@ -43,9 +43,19 @@ describe FinalDestination::SSRFDetector do
expect(subject.ip_allowed?("9001:82f3:8873::3")).to eq(false)
end
it "returns false for standard internal IPs" do
expect(subject.ip_allowed?("172.31.100.31")).to eq(false)
expect(subject.ip_allowed?("fd02:77fa:ffea::f")).to eq(false)
%w[0.0.0.0 10.0.0.0 127.0.0.0 172.31.100.31 255.255.255.255 ::1 ::].each do |internal_ip|
it "returns false for '#{internal_ip}'" do
expect(subject.ip_allowed?(internal_ip)).to eq(false)
end
end
it "returns false for private IPv4-mapped IPv6 addresses" do
expect(subject.ip_allowed?("::ffff:172.31.100.31")).to eq(false)
expect(subject.ip_allowed?("::ffff:0.0.0.0")).to eq(false)
end
it "returns true for public IPv4-mapped IPv6 addresses" do
expect(subject.ip_allowed?("::ffff:52.52.167.244")).to eq(true)
end
end

View File

@ -0,0 +1,60 @@
# frozen_string_literal: true
klass = defined?(WebPush) ? WebPush : Webpush
RSpec.describe klass do
before do
FinalDestination::SSRFDetector.allow_ip_lookups_in_test!
WebMock.enable!(except: [:final_destination])
end
after do
WebMock.enable!
FinalDestination::SSRFDetector.disallow_ip_lookups_in_test!
end
it "should filter endpoint hostname through our SSRF detector" do
klass::Request.any_instance.expects(:encrypt_payload)
klass::Request.any_instance.expects(:headers)
stub_ip_lookup("example.com", %W[0.0.0.0])
expect do
klass.payload_send(
endpoint: "http://example.com",
message: "test",
p256dh: "somep256dh",
auth: "someauth",
vapid: {
subject: "someurl",
public_key: "somepublickey",
private_key: "someprivatekey",
},
)
end.to raise_error(FinalDestination::SSRFDetector::DisallowedIpError)
end
it "should send the right request if endpoint hostname resolves to a public ip address" do
klass::Request.any_instance.expects(:encrypt_payload)
klass::Request.any_instance.expects(:headers)
stub_ip_lookup("example.com", %W[52.125.123.12])
success = Class.new(StandardError)
TCPSocket.stubs(:open).with { |addr| "52.125.123.12" == addr }.once.raises(success)
expect do
klass.payload_send(
endpoint: "http://example.com",
message: "test",
p256dh: "somep256dh",
auth: "someauth",
vapid: {
subject: "someurl",
public_key: "somepublickey",
private_key: "someprivatekey",
},
)
end.to raise_error(success)
end
end

View File

@ -220,7 +220,7 @@ RSpec.describe SearchIndexer do
Jobs.run_immediately!
SiteSetting.max_image_width = 1
stub_request(:get, "https://meta.discourse.org/some.png").to_return(
stub_request(:get, "https://1.2.3.4/some.png").to_return(
status: 200,
body: file_from_fixtures("logo.png").read,
)