diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index bf5e5f8..65fe9d2 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -7,11 +7,11 @@ class Users::PushSubscriptionsController < ApplicationController def create if subscription = @push_subscriptions.find_by(push_subscription_params) subscription.touch + head :ok else - @push_subscriptions.create! push_subscription_params.merge(user_agent: request.user_agent) + subscription = @push_subscriptions.create push_subscription_params.merge(user_agent: request.user_agent) + head subscription.persisted? ? :ok : :unprocessable_entity end - - head :ok end def destroy diff --git a/app/models/push/subscription.rb b/app/models/push/subscription.rb index f972941..b40212e 100644 --- a/app/models/push/subscription.rb +++ b/app/models/push/subscription.rb @@ -1,7 +1,73 @@ class Push::Subscription < ApplicationRecord + # Web push endpoints only ever point at a browser vendor's push service. An + # allowlist keeps a user-supplied endpoint from turning delivery into an SSRF + # sink, and pinning the resolved public IP on every delivery closes the + # DNS-rebinding gap the way Opengraph::Fetch does for unfurls. + PERMITTED_ENDPOINT_HOSTS = %w[ + jmt17.google.com + fcm.googleapis.com + updates.push.services.mozilla.com + web.push.apple.com + notify.windows.com + ].freeze + belongs_to :user + validates :endpoint, presence: true + validate :validate_endpoint_url + def notification(**params) - WebPush::Notification.new(**params, badge: user.memberships.unread.count, endpoint: endpoint, p256dh_key: p256dh_key, auth_key: auth_key) + WebPush::Notification.new(**params, badge: user.memberships.unread.count, endpoint: endpoint, endpoint_ip: resolved_endpoint_ip, p256dh_key: p256dh_key, auth_key: auth_key) end + + # The public address to pin this delivery to, or nil when the endpoint is not a + # permitted push service or doesn't resolve to a public IP. Enforced here, not + # only at save time, so a row that predates validation (or was inserted around + # it) still can't drive delivery at a non-allowlisted or private target. + # Re-resolved on every call so each delivery pins a freshly looked-up address + # rather than trusting the host to still resolve the way it did at sign-up. + def resolved_endpoint_ip + Surfguard.resolve_public_ips(endpoint_uri.host).first if permitted_endpoint_uri? + rescue Surfguard::Unresolvable + # A host that resolves to nothing has no usable public IP -- the same outcome + # as one whose only addresses are blocked: no endpoint IP to pin, which fails + # endpoint validation. Push has no lookup-failed surface to distinguish. + nil + end + + private + # The full shape a deliverable endpoint must have: HTTPS on the default port, + # pointing at a permitted push service. Gating resolution on this (not just + # the host) keeps a row that slipped past save-time validation from driving + # delivery to an odd port or scheme on a permitted vendor's address. + def permitted_endpoint_uri? + endpoint_uri&.scheme == "https" && endpoint_uri.port == 443 && permitted_endpoint_host? + end + + def endpoint_uri + URI.parse(endpoint) if endpoint.present? + rescue URI::InvalidURIError + nil + end + + def validate_endpoint_url + if endpoint_uri.nil? + errors.add(:endpoint, "is not a valid URL") + elsif endpoint_uri.scheme != "https" + errors.add(:endpoint, "must use HTTPS") + elsif endpoint_uri.port != 443 + errors.add(:endpoint, "must use the default HTTPS port") + elsif !permitted_endpoint_host? + errors.add(:endpoint, "is not a permitted push service") + elsif resolved_endpoint_ip.nil? + errors.add(:endpoint, "resolves to a private or invalid IP address") + end + end + + def permitted_endpoint_host? + host = endpoint_uri&.host&.downcase + host.present? && PERMITTED_ENDPOINT_HOSTS.any? do |permitted| + host == permitted || host.end_with?(".#{permitted}") + end + end end diff --git a/config/initializers/web_push.rb b/config/initializers/web_push.rb index ac96046..30f9ec7 100644 --- a/config/initializers/web_push.rb +++ b/config/initializers/web_push.rb @@ -17,7 +17,18 @@ end module WebPush::PersistentRequest def perform - if @options[:connection] + if endpoint_ip = @options[:endpoint_ip] + # Pin the connection to the public IP resolved (and guarded) by + # Push::Subscription so delivery can't be rebound to a private address + # between resolution and connect. Bypasses the shared persistent pool, + # which would re-resolve the host itself. + http = Net::HTTP.new(uri.host, uri.port) + http.ipaddr = endpoint_ip + 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? + elsif @options[:connection] http = @options[:connection] else http = Net::HTTP.new(uri.host, uri.port, *proxy_options) diff --git a/lib/web_push/notification.rb b/lib/web_push/notification.rb index 2ad156c..f1ba1df 100644 --- a/lib/web_push/notification.rb +++ b/lib/web_push/notification.rb @@ -1,16 +1,23 @@ class WebPush::Notification - def initialize(title:, body:, path:, badge:, endpoint:, p256dh_key:, auth_key:) + def initialize(title:, body:, path:, badge:, endpoint:, endpoint_ip:, p256dh_key:, auth_key:) @title, @body, @path, @badge = title, body, path, badge - @endpoint, @p256dh_key, @auth_key = endpoint, p256dh_key, auth_key + @endpoint, @endpoint_ip, @p256dh_key, @auth_key = endpoint, endpoint_ip, p256dh_key, auth_key end + # @endpoint_ip is the public address Push::Subscription resolved and guarded for + # this delivery. When it is nil the host resolved to nothing or to a blocked + # (private) address, so we skip delivery rather than let the request fall back + # to re-resolving the raw host -- which is what would reopen the SSRF for a + # subscription that slipped in before endpoint validation existed. def deliver(connection: nil) - WebPush.payload_send \ - message: encoded_message, - endpoint: @endpoint, p256dh: @p256dh_key, auth: @auth_key, - vapid: vapid_identification, - connection: connection, - urgency: "high" + if @endpoint_ip + WebPush.payload_send \ + message: encoded_message, + endpoint: @endpoint, endpoint_ip: @endpoint_ip, p256dh: @p256dh_key, auth: @auth_key, + vapid: vapid_identification, + connection: connection, + urgency: "high" + end end private diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb index 0b90513..449840e 100644 --- a/test/controllers/users/push_subscriptions_controller_test.rb +++ b/test/controllers/users/push_subscriptions_controller_test.rb @@ -3,10 +3,11 @@ require "test_helper" class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest setup do sign_in :david + stub_web_push_dns_resolution end test "create new push subscription" do - subscription_params = { "endpoint" => "https://apple", "p256dh_key" => "123", "auth_key" => "456" } + subscription_params = { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" } post user_push_subscriptions_url, params: { push_subscription: subscription_params }, headers: { "HTTP_USER_AGENT" => "Mozilla/5.0" } @@ -29,6 +30,27 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest assert_response :ok end + test "rejects subscription with non-permitted endpoint" do + subscription_params = { "endpoint" => "https://attacker.example.com/steal", "p256dh_key" => "123", "auth_key" => "456" } + + assert_no_difference -> { Push::Subscription.count } do + post user_push_subscriptions_url, params: { push_subscription: subscription_params } + end + + assert_response :unprocessable_entity + end + + test "rejects subscription with endpoint resolving to a private IP" do + stub_dns_resolution("169.254.169.254") + subscription_params = { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" } + + assert_no_difference -> { Push::Subscription.count } do + post user_push_subscriptions_url, params: { push_subscription: subscription_params } + end + + assert_response :unprocessable_entity + end + test "destroy a push subscription via dev mode" do assert_difference -> { Push::Subscription.count }, -1 do delete user_push_subscription_url(push_subscriptions(:david_chrome)) diff --git a/test/lib/web_push/persistent_request_test.rb b/test/lib/web_push/persistent_request_test.rb new file mode 100644 index 0000000..c0f4742 --- /dev/null +++ b/test/lib/web_push/persistent_request_test.rb @@ -0,0 +1,26 @@ +require "test_helper" + +class WebPush::PersistentRequestTest < ActiveSupport::TestCase + ENDPOINT = "https://fcm.googleapis.com/fcm/send/test123" + + # The delivery must connect to the public IP resolved and guarded by + # Push::Subscription, never re-resolve the raw endpoint host at connect time -- + # otherwise a rebind between resolution and delivery reopens the SSRF. An empty + # message keeps the request past encryption and onto the socket we assert on. + test "pins delivery to endpoint_ip instead of re-resolving the host" do + host = URI(ENDPOINT).host + WebMock.disable_net_connect! allow: [ host ] + + TCPSocket.expects(:open).with { |*args, **| args.first == host }.never + TCPSocket.expects(:open).with { |*args, **| args.first == DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP && args[1] == 443 }.throws(:pinned_to_ip) + + assert_throws :pinned_to_ip do + WebPush.payload_send \ + message: "", + endpoint: ENDPOINT, + endpoint_ip: DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP, + p256dh: "", auth: "", vapid: {}, + urgency: "high" + end + end +end diff --git a/test/models/push/subscription_test.rb b/test/models/push/subscription_test.rb new file mode 100644 index 0000000..de5bb51 --- /dev/null +++ b/test/models/push/subscription_test.rb @@ -0,0 +1,145 @@ +require "test_helper" + +class Push::SubscriptionTest < ActiveSupport::TestCase + setup do + stub_web_push_dns_resolution + end + + test "valid subscription with permitted endpoint" do + assert build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123").valid? + end + + test "rejects endpoint with non-https scheme" do + subscription = build_subscription(endpoint: "http://fcm.googleapis.com/fcm/send/abc123") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "must use HTTPS" + end + + test "rejects endpoint with non-permitted host" do + subscription = build_subscription(endpoint: "https://attacker.example.com/webhook") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "is not a permitted push service" + end + + test "rejects endpoint whose host only suffix-matches a permitted host" do + subscription = build_subscription(endpoint: "https://evilfcm.googleapis.com.attacker.example/webhook") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "is not a permitted push service" + end + + test "rejects blank endpoint" do + subscription = build_subscription(endpoint: "") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "can't be blank" + end + + test "rejects endpoint on a non-default port" do + subscription = build_subscription(endpoint: "https://fcm.googleapis.com:8443/fcm/send/abc123") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "must use the default HTTPS port" + end + + test "rejects endpoint that resolves to private IP" do + stub_dns_resolution("192.168.1.1") + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address" + end + + test "rejects endpoint that resolves to loopback IP" do + stub_dns_resolution("127.0.0.1") + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address" + end + + test "rejects endpoint that resolves to link-local IP (AWS IMDS)" do + stub_dns_resolution("169.254.169.254") + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address" + end + + test "rejects endpoint whose host resolves to nothing without raising" do + stub_dns_failure + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + assert_nil subscription.resolved_endpoint_ip + assert_not subscription.valid? + assert_includes subscription.errors[:endpoint], "resolves to a private or invalid IP address" + end + + test "resolved_endpoint_ip returns the pinned public IP" do + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + assert_equal DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP, subscription.resolved_endpoint_ip + end + + test "notification carries the resolved endpoint IP for delivery pinning" do + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + notification = subscription.notification(title: "t", body: "b", path: "/") + + assert_equal DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP, notification.instance_variable_get(:@endpoint_ip) + end + + test "delivery is skipped when the endpoint no longer resolves to a public IP" do + stub_dns_resolution("10.0.0.5") # host now answers with a private address + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + assert_nil subscription.resolved_endpoint_ip + WebPush.expects(:payload_send).never + subscription.notification(title: "t", body: "b", path: "/").deliver + end + + test "delivery is skipped for a non-permitted host even when it resolves publicly" do + # A row that predates endpoint validation: its host resolves to a public IP, + # but it is not a permitted push service, so delivery must not proceed. + subscription = build_subscription(endpoint: "https://attacker.example.com/collect") + + assert_nil subscription.resolved_endpoint_ip + WebPush.expects(:payload_send).never + subscription.notification(title: "t", body: "b", path: "/").deliver + end + + test "delivery is skipped for a permitted host on a non-default port" do + # A legacy/bypassed row: permitted host, resolves publicly, but port 22. + subscription = build_subscription(endpoint: "https://fcm.googleapis.com:22/fcm/send/abc123") + + assert_nil subscription.resolved_endpoint_ip + WebPush.expects(:payload_send).never + subscription.notification(title: "t", body: "b", path: "/").deliver + end + + test "delivery sends with the pinned endpoint_ip" do + subscription = build_subscription(endpoint: "https://fcm.googleapis.com/fcm/send/abc123") + + WebPush.expects(:payload_send).with(has_entry(endpoint_ip: DnsTestHelper::WEB_PUSH_PUBLIC_TEST_IP)) + subscription.notification(title: "t", body: "b", path: "/").deliver + end + + test "accepts all permitted push service domains" do + [ + "https://fcm.googleapis.com/fcm/send/token123", + "https://jmt17.google.com/fcm/send/token123", + "https://updates.push.services.mozilla.com/wpush/v2/token123", + "https://web.push.apple.com/QaBC123", + "https://wns2-db5p.notify.windows.com/w/?token=abc123" + ].each do |endpoint| + subscription = build_subscription(endpoint: endpoint) + assert subscription.valid?, "Expected #{endpoint} to be valid, got: #{subscription.errors.full_messages}" + end + end + + private + def build_subscription(endpoint:) + Push::Subscription.new(user: users(:david), endpoint: endpoint, p256dh_key: "test_key", auth_key: "test_auth") + end +end diff --git a/test/models/room/push_test.rb b/test/models/room/push_test.rb index 7c5c590..8ecfdc6 100644 --- a/test/models/room/push_test.rb +++ b/test/models/room/push_test.rb @@ -3,6 +3,10 @@ require "test_helper" class Room::PushTest < ActiveSupport::TestCase include ActiveJob::TestHelper + setup do + stub_web_push_dns_resolution + end + test "deliver new message to other room users with push subscriptions" do task_count = Push::Subscription.count - users(:david).push_subscriptions.count perform_enqueued_jobs only: Room::PushMessageJob do diff --git a/test/test_helper.rb b/test/test_helper.rb index 9c1310c..5c4ac31 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -17,7 +17,7 @@ class ActiveSupport::TestCase # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order. fixtures :all - include SessionTestHelper, MentionTestHelper, TurboTestHelper + include SessionTestHelper, MentionTestHelper, TurboTestHelper, DnsTestHelper setup do ActionCable.server.pubsub.clear diff --git a/test/test_helpers/dns_test_helper.rb b/test/test_helpers/dns_test_helper.rb new file mode 100644 index 0000000..86aa578 --- /dev/null +++ b/test/test_helpers/dns_test_helper.rb @@ -0,0 +1,21 @@ +module DnsTestHelper + WEB_PUSH_PUBLIC_TEST_IP = "142.250.185.206" # stable public IP for web push DNS stubs in tests + + private + # Surfguard resolves through Resolv.getaddresses, which honours /etc/hosts and + # search domains and returns every address a host answers with. + def stub_dns_resolution(*ips) + Resolv.stubs(:getaddresses).returns(ips.map(&:to_s)) + end + + # A host that resolves to nothing: the resolver errors (timeout/NXDOMAIN), + # which Surfguard catches and reports as Unresolvable, distinct from a host + # that resolves only to blocked addresses. + def stub_dns_failure(error = Resolv::ResolvError) + Resolv.stubs(:getaddresses).raises(error) + end + + def stub_web_push_dns_resolution + stub_dns_resolution(WEB_PUSH_PUBLIC_TEST_IP) + end +end