diff --git a/.gitignore b/.gitignore index b89b11b4f..96a88d934 100644 --- a/.gitignore +++ b/.gitignore @@ -34,3 +34,6 @@ BRANCH .rubocop-https* .env* +node_modules +yarn.lock + diff --git a/.release-please-manifest.json b/.release-please-manifest.json index 931d27504..8e34c5c9b 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,3 +1,3 @@ { - ".": "3.3.4" + ".": "3.3.7" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b78a9257..da81f33bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,67 @@ This file contains all the latest changes and updates to Postal. +## [3.3.7](https://github.com/postalserver/postal/compare/3.3.6...3.3.7) (2026-06-03) + + +### Bug Fixes + +* **http:** prevent SSRF in outbound webhook and HTTP endpoint requests ([11c9814](https://github.com/postalserver/postal/commit/11c9814474f956619da35e8385ef7fab9f304de0)) +* **message-db:** prevent SQL injection via condition keys (GHSA-x2hq-rfpg-3xr5) ([4314a6e](https://github.com/postalserver/postal/commit/4314a6ec1e2812daa67dd20effd1db1769c1f8e8)) +* **specs:** stub IPv6 support in address guard IPv6 literal spec ([029bfe0](https://github.com/postalserver/postal/commit/029bfe098d9b8c0b5cafc49eac33e767f5748cd3)) + + +### Miscellaneous Chores + +* **deps:** upgrade rack & rails ([0445e5c](https://github.com/postalserver/postal/commit/0445e5c509870dfe9c16366c53dee3fc02ad3904)) +* update security vulnerability reporting instructions ([8be1e27](https://github.com/postalserver/postal/commit/8be1e27fec489ab659ef5e909f705932028b1694)) + +## [3.3.6](https://github.com/postalserver/postal/compare/3.3.5...3.3.6) (2026-04-28) + + +### Bug Fixes + +* **messages:** sandbox rendered email HTML as extra XSS defence ([cad2aa6](https://github.com/postalserver/postal/commit/cad2aa6808519a3ff25215f09f4966d9fa3bb372)) + + +### Miscellaneous Chores + +* ignore node modules and yarn.lock ([b611d57](https://github.com/postalserver/postal/commit/b611d577af79b8e1e75b6d47fa04d1ba03e34eec)) + + +### Code Refactoring + +* **auth:** tighten return_to validation ([84f4e20](https://github.com/postalserver/postal/commit/84f4e20f05db2d11b0144f95960c956f8221e657)) +* **helpers:** escape interpolated values in select options ([9243524](https://github.com/postalserver/postal/commit/924352403553dcfcc569876ca76c219493fac9d6)) +* **tracking:** remove unused src image proxy ([dca7f90](https://github.com/postalserver/postal/commit/dca7f90b9046247c0d953567be35921167e79d87)) + +## [3.3.5](https://github.com/postalserver/postal/compare/3.3.4...3.3.5) (2026-02-01) + + +### Bug Fixes + +* **deliveries:** escape delivery details to prevent HTML injection ([11419f9](https://github.com/postalserver/postal/commit/11419f99140e13688a9613cab3ee03f8d3cbae45)) +* **health_server:** use rackup handler instead of rack handler ([7c47422](https://github.com/postalserver/postal/commit/7c47422c865e738c4d6af0fed1cca4405288341f)) +* oidc scopes are invalid when concatenated ([#3332](https://github.com/postalserver/postal/issues/3332)) ([9c5f96a](https://github.com/postalserver/postal/commit/9c5f96ae90cf06dcd5db776806865752f667bd95)) +* typo in process logging ([#3212](https://github.com/postalserver/postal/issues/3212)) ([b7e5232](https://github.com/postalserver/postal/commit/b7e5232e077b3c9b7a999dcb6676fba0ec61458e)) +* typo in the credentials page ([fd3c7cc](https://github.com/postalserver/postal/commit/fd3c7ccdf6dc4ee0a76c9523cbd735159e4b8000)) +* update url for v2 config ([#3225](https://github.com/postalserver/postal/issues/3225)) ([e00098b](https://github.com/postalserver/postal/commit/e00098b8003cf37f2708f536871b3ade377aed2d)) + + +### Documentation + +* **process.rb:** add help about time unit used by metric ([#3339](https://github.com/postalserver/postal/issues/3339)) ([f5325c4](https://github.com/postalserver/postal/commit/f5325c49ff1152ad53eaaec98717ad3412d379ae)) + + +### Miscellaneous Chores + +* **deps:** upgrade puma, net-imap and other deps ([c03c44b](https://github.com/postalserver/postal/commit/c03c44b442a29aa9881c1e1aae60bead9776a6b6)) +* **dockerfile:** reduce container size ([86de372](https://github.com/postalserver/postal/commit/86de372382bd62bdd5d1372254f8817b0360bd56)) +* remove version from docker-compose.yml ([c78000c](https://github.com/postalserver/postal/commit/c78000ca8f2998aa04648f465060768db6467de6)) +* upgrade resolv to 0.6.2 ([d00d978](https://github.com/postalserver/postal/commit/d00d978872a96369544303d08f6a9d11cdf56b62)) +* upgrade to rails 7.1 and ruby 3.4 ([#3457](https://github.com/postalserver/postal/issues/3457)) ([ab6d443](https://github.com/postalserver/postal/commit/ab6d4430baa33a05f1aa66e776cc2a5bcaa0ede8)) +* upgrade uri gem to 1.0.3 ([f193b8e](https://github.com/postalserver/postal/commit/f193b8e77fc096382ab7aaa6a2c29641b4cb12df)) + ## [3.3.4](https://github.com/postalserver/postal/compare/3.3.3...3.3.4) (2024-06-20) diff --git a/Gemfile b/Gemfile index 48d8bf75c..4d3c0deac 100644 --- a/Gemfile +++ b/Gemfile @@ -29,7 +29,7 @@ gem "ostruct" gem "prometheus-client" gem "puma" gem "rackup" -gem "rails", "= 7.1.5.2" +gem "rails", "= 7.1.6" gem "resolv" gem "secure_headers" gem "sentry-rails" diff --git a/Gemfile.lock b/Gemfile.lock index a4ad73ca5..16adf02bf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -2,35 +2,36 @@ GEM remote: https://rubygems.org/ specs: abbrev (0.1.2) - actioncable (7.1.5.2) - actionpack (= 7.1.5.2) - activesupport (= 7.1.5.2) + actioncable (7.1.6) + actionpack (= 7.1.6) + activesupport (= 7.1.6) nio4r (~> 2.0) websocket-driver (>= 0.6.1) zeitwerk (~> 2.6) - actionmailbox (7.1.5.2) - actionpack (= 7.1.5.2) - activejob (= 7.1.5.2) - activerecord (= 7.1.5.2) - activestorage (= 7.1.5.2) - activesupport (= 7.1.5.2) + actionmailbox (7.1.6) + actionpack (= 7.1.6) + activejob (= 7.1.6) + activerecord (= 7.1.6) + activestorage (= 7.1.6) + activesupport (= 7.1.6) mail (>= 2.7.1) net-imap net-pop net-smtp - actionmailer (7.1.5.2) - actionpack (= 7.1.5.2) - actionview (= 7.1.5.2) - activejob (= 7.1.5.2) - activesupport (= 7.1.5.2) + actionmailer (7.1.6) + actionpack (= 7.1.6) + actionview (= 7.1.6) + activejob (= 7.1.6) + activesupport (= 7.1.6) mail (~> 2.5, >= 2.5.4) net-imap net-pop net-smtp rails-dom-testing (~> 2.2) - actionpack (7.1.5.2) - actionview (= 7.1.5.2) - activesupport (= 7.1.5.2) + actionpack (7.1.6) + actionview (= 7.1.6) + activesupport (= 7.1.6) + cgi nokogiri (>= 1.8.5) racc rack (>= 2.2.4) @@ -38,35 +39,36 @@ GEM rack-test (>= 0.6.3) rails-dom-testing (~> 2.2) rails-html-sanitizer (~> 1.6) - actiontext (7.1.5.2) - actionpack (= 7.1.5.2) - activerecord (= 7.1.5.2) - activestorage (= 7.1.5.2) - activesupport (= 7.1.5.2) + actiontext (7.1.6) + actionpack (= 7.1.6) + activerecord (= 7.1.6) + activestorage (= 7.1.6) + activesupport (= 7.1.6) globalid (>= 0.6.0) nokogiri (>= 1.8.5) - actionview (7.1.5.2) - activesupport (= 7.1.5.2) + actionview (7.1.6) + activesupport (= 7.1.6) builder (~> 3.1) + cgi erubi (~> 1.11) rails-dom-testing (~> 2.2) rails-html-sanitizer (~> 1.6) - activejob (7.1.5.2) - activesupport (= 7.1.5.2) + activejob (7.1.6) + activesupport (= 7.1.6) globalid (>= 0.3.6) - activemodel (7.1.5.2) - activesupport (= 7.1.5.2) - activerecord (7.1.5.2) - activemodel (= 7.1.5.2) - activesupport (= 7.1.5.2) + activemodel (7.1.6) + activesupport (= 7.1.6) + activerecord (7.1.6) + activemodel (= 7.1.6) + activesupport (= 7.1.6) timeout (>= 0.4.0) - activestorage (7.1.5.2) - actionpack (= 7.1.5.2) - activejob (= 7.1.5.2) - activerecord (= 7.1.5.2) - activesupport (= 7.1.5.2) + activestorage (7.1.6) + actionpack (= 7.1.6) + activejob (= 7.1.6) + activerecord (= 7.1.6) + activesupport (= 7.1.6) marcel (~> 1.0) - activesupport (7.1.5.2) + activesupport (7.1.6) base64 benchmark (>= 0.3) bigdecimal @@ -93,10 +95,11 @@ GEM execjs (~> 2) base64 (0.3.0) bcrypt (3.1.20) - benchmark (0.4.1) - bigdecimal (3.2.3) + benchmark (0.5.0) + bigdecimal (4.1.2) bindata (2.5.0) - builder (3.2.4) + builder (3.3.0) + cgi (0.5.1) chronic (0.10.2) coffee-rails (5.0.0) coffee-script (>= 2.2.0) @@ -105,8 +108,8 @@ GEM coffee-script-source execjs coffee-script-source (1.12.2) - concurrent-ruby (1.3.5) - connection_pool (2.5.4) + concurrent-ruby (1.3.6) + connection_pool (3.0.2) crack (1.0.0) bigdecimal rexml @@ -115,7 +118,7 @@ GEM activerecord (>= 5.a) database_cleaner-core (~> 2.0) database_cleaner-core (2.0.1) - date (3.4.1) + date (3.5.1) diff-lcs (1.6.2) domain_name (0.6.20240107) dotenv (3.0.2) @@ -123,8 +126,8 @@ GEM dynamic_form (1.2.0) email_validator (2.2.4) activemodel - erb (5.0.2) - erubi (1.12.0) + erb (6.0.4) + erubi (1.13.1) execjs (2.7.0) factory_bot (6.4.6) activesupport (>= 5.0.0) @@ -149,11 +152,12 @@ GEM hashdiff (1.1.0) hashie (5.0.0) highline (2.1.0) - i18n (1.14.7) + i18n (1.14.8) concurrent-ruby (~> 1.0) - io-console (0.8.1) - irb (1.15.2) + io-console (0.8.2) + irb (1.18.0) pp (>= 0.6.0) + prism (>= 1.3.0) rdoc (>= 4.0.0) reline (>= 0.4.2) jquery-rails (4.5.1) @@ -189,42 +193,45 @@ GEM konfig-config (3.0.0) hashie logger (1.7.0) - loofah (2.24.1) + loofah (2.25.1) crass (~> 1.0.2) nokogiri (>= 1.12.0) - mail (2.8.1) + mail (2.9.0) + logger mini_mime (>= 0.1.1) net-imap net-pop net-smtp - marcel (1.1.0) + marcel (1.2.1) mini_mime (1.1.5) - minitest (5.25.5) + minitest (6.0.6) + drb (~> 2.0) + prism (~> 1.5) mutex_m (0.3.0) mysql2 (0.5.6) net-http (0.4.1) uri - net-imap (0.5.11) + net-imap (0.6.4) date net-protocol net-pop (0.1.2) net-protocol net-protocol (0.2.2) timeout - net-smtp (0.4.0.1) + net-smtp (0.5.1) net-protocol nifty-utils (1.1.7) nilify_blanks (1.4.0) activerecord (>= 4.0.0) activesupport (>= 4.0.0) - nio4r (2.7.4) - nokogiri (1.18.10-aarch64-linux-gnu) + nio4r (2.7.5) + nokogiri (1.19.3-aarch64-linux-gnu) racc (~> 1.4) - nokogiri (1.18.10-arm64-darwin) + nokogiri (1.19.3-arm64-darwin) racc (~> 1.4) - nokogiri (1.18.10-x86_64-darwin) + nokogiri (1.19.3-x86_64-darwin) racc (~> 1.4) - nokogiri (1.18.10-x86_64-linux-gnu) + nokogiri (1.19.3-x86_64-linux-gnu) racc (~> 1.4) omniauth (2.1.2) hashie (>= 3.4.6) @@ -253,18 +260,19 @@ GEM parallel (1.22.1) parser (3.2.1.1) ast (~> 2.4.1) - pp (0.6.2) + pp (0.6.3) prettyprint prettyprint (0.2.0) + prism (1.9.0) prometheus-client (4.2.2) - psych (5.2.6) + psych (5.4.0) date stringio public_suffix (5.0.4) puma (7.0.4) nio4r (~> 2.0) racc (1.8.1) - rack (3.2.1) + rack (3.2.6) rack-oauth2 (2.2.1) activesupport attr_required @@ -276,49 +284,52 @@ GEM base64 (>= 0.1.0) logger (>= 1.6.0) rack (>= 3.0.0, < 4) - rack-session (2.1.1) + rack-session (2.1.2) base64 (>= 0.1.0) rack (>= 3.0.0) - rack-test (2.1.0) + rack-test (2.2.0) rack (>= 1.3) - rackup (2.2.1) + rackup (2.3.1) rack (>= 3) - rails (7.1.5.2) - actioncable (= 7.1.5.2) - actionmailbox (= 7.1.5.2) - actionmailer (= 7.1.5.2) - actionpack (= 7.1.5.2) - actiontext (= 7.1.5.2) - actionview (= 7.1.5.2) - activejob (= 7.1.5.2) - activemodel (= 7.1.5.2) - activerecord (= 7.1.5.2) - activestorage (= 7.1.5.2) - activesupport (= 7.1.5.2) + rails (7.1.6) + actioncable (= 7.1.6) + actionmailbox (= 7.1.6) + actionmailer (= 7.1.6) + actionpack (= 7.1.6) + actiontext (= 7.1.6) + actionview (= 7.1.6) + activejob (= 7.1.6) + activemodel (= 7.1.6) + activerecord (= 7.1.6) + activestorage (= 7.1.6) + activesupport (= 7.1.6) bundler (>= 1.15.0) - railties (= 7.1.5.2) - rails-dom-testing (2.2.0) + railties (= 7.1.6) + rails-dom-testing (2.3.0) activesupport (>= 5.0.0) minitest nokogiri (>= 1.6) - rails-html-sanitizer (1.6.2) - loofah (~> 2.21) + rails-html-sanitizer (1.7.0) + loofah (~> 2.25) nokogiri (>= 1.15.7, != 1.16.7, != 1.16.6, != 1.16.5, != 1.16.4, != 1.16.3, != 1.16.2, != 1.16.1, != 1.16.0.rc1, != 1.16.0) - railties (7.1.5.2) - actionpack (= 7.1.5.2) - activesupport (= 7.1.5.2) + railties (7.1.6) + actionpack (= 7.1.6) + activesupport (= 7.1.6) + cgi irb rackup (>= 1.0.0) rake (>= 12.2) thor (~> 1.0, >= 1.2.2) + tsort (>= 0.2) zeitwerk (~> 2.6) rainbow (3.1.1) - rake (13.1.0) - rdoc (6.14.2) + rake (13.4.2) + rdoc (7.2.0) erb psych (>= 4.0.0) + tsort regexp_parser (2.7.0) - reline (0.6.2) + reline (0.6.3) io-console (~> 0.5) resolv (0.6.2) rexml (3.4.4) @@ -387,17 +398,18 @@ GEM actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) - stringio (3.1.7) + stringio (3.2.0) swd (2.0.3) activesupport (>= 3) attr_required (>= 0.0.5) faraday (~> 2.0) faraday-follow_redirects temple (0.10.3) - thor (1.3.0) + thor (1.5.0) tilt (2.3.0) timecop (0.9.8) - timeout (0.4.3) + timeout (0.6.1) + tsort (0.2.0) turbolinks (5.2.1) turbolinks-source (~> 5.2) turbolinks-source (5.2.0) @@ -423,7 +435,7 @@ GEM base64 websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) - zeitwerk (2.6.13) + zeitwerk (2.8.2) PLATFORMS aarch64-linux @@ -467,7 +479,7 @@ DEPENDENCIES prometheus-client puma rackup - rails (= 7.1.5.2) + rails (= 7.1.6) resolv rspec rspec-rails diff --git a/SECURITY.md b/SECURITY.md index 7936c0808..23e945be0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -11,5 +11,5 @@ We only support updates to the 3.x versions of Postal. ## Reporting a Vulnerability -If you discover a vulnerability in Postal, please do not post an issue on GitHub. Instead you should send an -e-mail to security@postalserver.io with details. We will get back to you directly. +If you discover a vulnerability in Postal, please do not post an issue on GitHub. Please, +instead, [create a new security advisory through GitHub](https://github.com/postalserver/postal/security/advisories/new). diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d2c36cda5..24498f7ed 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -62,10 +62,13 @@ def append_info_to_payload(payload) end def url_with_return_to(url) - if params[:return_to].blank? || !params[:return_to].starts_with?("/") + return_to = params[:return_to] + if return_to.blank? || + !return_to.start_with?("/") || + return_to.start_with?("//", "/\\") url_for(url) else - params[:return_to] + return_to end end diff --git a/app/controllers/legacy_api/messages_controller.rb b/app/controllers/legacy_api/messages_controller.rb index 6d78c1618..239eee3cd 100644 --- a/app/controllers/legacy_api/messages_controller.rb +++ b/app/controllers/legacy_api/messages_controller.rb @@ -15,12 +15,9 @@ class MessagesController < BaseController # OR an error if the message does not exist. # def message - if api_params["id"].blank? - render_parameter_error "`id` parameter is required but is missing" - return - end + message = find_message + return if performed? - message = @current_credential.server.message(api_params["id"]) message_hash = { id: message.id, token: message.token } expansions = api_params["_expansions"] @@ -111,12 +108,9 @@ def message # OR an error if the message does not exist. # def deliveries - if api_params["id"].blank? - render_parameter_error "`id` parameter is required but is missing" - return - end + message = find_message + return if performed? - message = @current_credential.server.message(api_params["id"]) deliveries = message.deliveries.map do |d| { id: d.id, @@ -136,5 +130,37 @@ def deliveries id: api_params["id"] end + private + + # Look up the message referenced by the request's `id` parameter. + # + # The legacy API only ever identifies a message by its integer ID. The + # request body is parsed as JSON, so without validation a JSON object or + # array supplied for `id` would arrive as a Ruby Hash/Array and be passed + # straight through to the message database as a raw set of SQL conditions. + # We therefore reject anything that is not a simple scalar before it can + # reach the database and coerce the value to an integer ID. + # + # Renders an error and returns nil when the parameter is missing or is not + # a scalar; otherwise returns the matched message (raising NotFound when no + # message matches, which the actions rescue). + # + # @return [Postal::MessageDB::Message, nil] + def find_message + id = api_params["id"] + + if id.blank? + render_parameter_error "`id` parameter is required but is missing" + return + end + + unless id.is_a?(String) || id.is_a?(Integer) + render_parameter_error "`id` parameter must be a string or integer" + return + end + + @current_credential.server.message(id.to_i) + end + end end diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb index ea0e940ce..9d59f08e4 100644 --- a/app/controllers/messages_controller.rb +++ b/app/controllers/messages_controller.rb @@ -89,6 +89,18 @@ def deliveries end def html_raw + override_content_security_policy_directives( + default_src: %w('none'), + script_src: %w('none'), + style_src: %w('unsafe-inline'), + img_src: %w(* data:), + font_src: %w(*), + frame_ancestors: %w('self'), + form_action: %w('none'), + base_uri: %w('none') + ) + response.headers["X-Content-Type-Options"] = "nosniff" + response.headers["Referrer-Policy"] = "no-referrer" render html: @message.html_body_without_tracking_image.html_safe end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index c6acf4563..17d124217 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -3,6 +3,7 @@ module ApplicationHelper def format_delivery_details(server, text) + text = h(text) text.gsub!(//) do id = ::Regexp.last_match(1).to_i link_to("message ##{id}", organization_server_message_path(server.organization, server, id), class: "u-link") @@ -32,7 +33,7 @@ def domain_options_for_select(server, selected_domain = nil, options = {}) s << "" server_domains.each do |domain| selected = domain == selected_domain ? "selected='selected'" : "" - s << "" + s << "" end s << "" end @@ -42,7 +43,7 @@ def domain_options_for_select(server, selected_domain = nil, options = {}) s << "" organization_domains.each do |domain| selected = domain == selected_domain ? "selected='selected'" : "" - s << "" + s << "" end s << "" end @@ -59,7 +60,7 @@ def endpoint_options_for_select(server, selected_value = nil, options = {}) http_endpoints.each do |endpoint| value = "#{endpoint.class}##{endpoint.uuid}" selected = value == selected_value ? "selected='selected'" : "" - s << "" + s << "" end s << "" end @@ -70,7 +71,7 @@ def endpoint_options_for_select(server, selected_value = nil, options = {}) smtp_endpoints.each do |endpoint| value = "#{endpoint.class}##{endpoint.uuid}" selected = value == selected_value ? "selected='selected'" : "" - s << "" + s << "" end s << "" end @@ -81,7 +82,7 @@ def endpoint_options_for_select(server, selected_value = nil, options = {}) address_endpoints.each do |endpoint| value = "#{endpoint.class}##{endpoint.uuid}" selected = value == selected_value ? "selected='selected'" : "" - s << "" + s << "" end s << "" end @@ -93,7 +94,7 @@ def endpoint_options_for_select(server, selected_value = nil, options = {}) selected = (selected_value == mode ? "selected='selected'" : "") text = t("route_modes.#{mode.underscore}") - s << "" + s << "" end s << "" end diff --git a/app/lib/smtp_server/server.rb b/app/lib/smtp_server/server.rb index 90902b5a8..fe5a63d4e 100644 --- a/app/lib/smtp_server/server.rb +++ b/app/lib/smtp_server/server.rb @@ -129,7 +129,12 @@ def run_event_loop end # We know who the client is, welcome them. client.logger&.debug "Client identified as #{client_ip_address}" - new_io.print("220 #{Postal::Config.postal.smtp_hostname} ESMTP Postal/#{client.trace_id}") + # Write the greeting line and its terminating CRLF in a single write. Using + # #print emits the string and the "$\" output record separator ("\r\n") as two + # separate writes, which can be sent as two TCP segments. Some clients read the + # greeting text before the CRLF arrives, issue EHLO early, and then mis-handle the + # stray CRLF prepended to the EHLO reply. Sending the line atomically avoids this. + new_io.write("220 #{Postal::Config.postal.smtp_hostname} ESMTP Postal/#{client.trace_id}\r\n") end # Register the client and its socket with nio4r monitor = @io_selector.register(new_io, :r) diff --git a/app/models/http_endpoint.rb b/app/models/http_endpoint.rb index 8cd9044f3..393b78536 100644 --- a/app/models/http_endpoint.rb +++ b/app/models/http_endpoint.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "uri" + # == Schema Information # # Table name: http_endpoints @@ -38,6 +40,7 @@ class HTTPEndpoint < ApplicationRecord validates :name, presence: true validates :url, presence: true + validate :url_must_be_http_or_https validates :encoding, inclusion: { in: ENCODINGS } validates :format, inclusion: { in: FORMATS } validates :timeout, numericality: { greater_than_or_equal_to: 5, less_than_or_equal_to: 60 } @@ -56,4 +59,17 @@ def update_routes routes.each { |r| r.update(endpoint: nil, mode: "Reject") } end + private + + def url_must_be_http_or_https + return if url.blank? + + uri = URI.parse(url) + return if uri.is_a?(URI::HTTP) && uri.host.present? + + errors.add(:url, "must be an HTTP or HTTPS URL") + rescue URI::InvalidURIError + errors.add(:url, "must be a valid HTTP or HTTPS URL") + end + end diff --git a/app/views/messages/html.html.haml b/app/views/messages/html.html.haml index 357f2cb50..826f7e176 100644 --- a/app/views/messages/html.html.haml +++ b/app/views/messages/html.html.haml @@ -14,4 +14,4 @@ This means that we no longer store the raw data for this e-mail or the e-mail didn't include a HTML part. - else - %iframe{:width => "100%", :height => "100%", :src => html_raw_organization_server_message_path(organization, @server, @message.id)} + %iframe{:width => "100%", :height => "100%", :sandbox => "allow-popups allow-popups-to-escape-sandbox", :referrerpolicy => "no-referrer", :src => html_raw_organization_server_message_path(organization, @server, @message.id)} diff --git a/doc/config/environment-variables.md b/doc/config/environment-variables.md index 940424e04..56fc70a5e 100644 --- a/doc/config/environment-variables.md +++ b/doc/config/environment-variables.md @@ -18,6 +18,7 @@ This document contains all the environment variables which are available for thi | `POSTAL_SIGNING_KEY_PATH` | String | Path to the private key used for signing | $config-file-root/signing.key | | `POSTAL_SMTP_RELAYS` | Array of strings | An array of SMTP relays in the format of smtp://host:port | [] | | `POSTAL_TRUSTED_PROXIES` | Array of strings | An array of IP addresses to trust for proxying requests to Postal (in addition to localhost addresses) | [] | +| `POSTAL_ALLOWED_REQUEST_DESTINATIONS` | Array of strings | Hostnames or IP/CIDR ranges that outbound webhook and HTTP endpoint requests are permitted to reach even when they resolve to a private, loopback, link-local or otherwise reserved address. All other such destinations are blocked to prevent SSRF. | [] | | `POSTAL_QUEUED_MESSAGE_LOCK_STALE_DAYS` | Integer | The number of days after which to consider a lock as stale. Messages with stale locks will be removed and not retried. | 1 | | `POSTAL_BATCH_QUEUED_MESSAGES` | Boolean | When enabled queued messages will be de-queued in batches based on their destination | true | | `WEB_SERVER_DEFAULT_PORT` | Integer | The default port the web server should listen on unless overriden by the PORT environment variable | 5000 | diff --git a/doc/config/yaml.yml b/doc/config/yaml.yml index f3a735a9f..4e0406d38 100644 --- a/doc/config/yaml.yml +++ b/doc/config/yaml.yml @@ -29,6 +29,8 @@ postal: smtp_relays: [] # An array of IP addresses to trust for proxying requests to Postal (in addition to localhost addresses) trusted_proxies: [] + # Hostnames or IP/CIDR ranges that outbound webhook and HTTP endpoint requests are permitted to reach even when they resolve to a private, loopback, link-local or otherwise reserved address. All other such destinations are blocked to prevent SSRF. + allowed_request_destinations: [] # The number of days after which to consider a lock as stale. Messages with stale locks will be removed and not retried. queued_message_lock_stale_days: 1 # When enabled queued messages will be de-queued in batches based on their destination diff --git a/lib/postal/config_schema.rb b/lib/postal/config_schema.rb index e3c6415d5..871d094a7 100644 --- a/lib/postal/config_schema.rb +++ b/lib/postal/config_schema.rb @@ -92,6 +92,14 @@ module Postal transform { |ip| IPAddr.new(ip) } end + string :allowed_request_destinations do + array + description "Hostnames or IP/CIDR ranges that outbound webhook and HTTP " \ + "endpoint requests are permitted to reach even when they resolve " \ + "to a private, loopback, link-local or otherwise reserved address. " \ + "All other such destinations are blocked to prevent SSRF." + end + integer :queued_message_lock_stale_days do description "The number of days after which to consider a lock as stale. Messages with stale locks will be removed and not retried." default 1 diff --git a/lib/postal/http.rb b/lib/postal/http.rb index 8a32d9b58..4f17febd6 100644 --- a/lib/postal/http.rb +++ b/lib/postal/http.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "net/https" +require "resolv" require "uri" module Postal @@ -47,19 +48,24 @@ def self.request(method, url, options = {}) request["User-Agent"] = options[:user_agent] || "Postal/#{Postal.version}" - connection = Net::HTTP.new(uri.host, uri.port) - - if uri.scheme == "https" - connection.use_ssl = true - connection.verify_mode = OpenSSL::SSL::VERIFY_PEER - ssl = true - else - ssl = false - end + timeout = options[:timeout] || 60 + ssl = uri.scheme == "https" begin - timeout = options[:timeout] || 60 Timeout.timeout(timeout) do + connect_address = AddressGuard.safe_connect_address(uri.host) + + connection = Net::HTTP.new(uri.host, uri.port) + # Pin the connection to the address we validated above so that the socket + # cannot be redirected to a different (e.g. internal) address via a DNS + # rebinding race between the check and the connection. + connection.ipaddr = connect_address + + if uri.scheme == "https" + connection.use_ssl = true + connection.verify_mode = OpenSSL::SSL::VERIFY_PEER + end + result = connection.request(request) { code: result.code.to_i, @@ -68,6 +74,13 @@ def self.request(method, url, options = {}) secure: ssl } end + rescue BlockedDestinationError => e + { + code: -4, + body: e.message, + headers: {}, + secure: ssl + } rescue OpenSSL::SSL::SSLError { code: -3, @@ -75,7 +88,7 @@ def self.request(method, url, options = {}) headers: {}, secure: ssl } - rescue SocketError, Errno::ECONNRESET, EOFError, Errno::EINVAL, Errno::ENETUNREACH, Errno::EHOSTUNREACH, Errno::ECONNREFUSED => e + rescue Resolv::ResolvError, SocketError, SystemCallError, EOFError => e { code: -2, body: e.message, diff --git a/lib/postal/http/address_guard.rb b/lib/postal/http/address_guard.rb new file mode 100644 index 000000000..ed863d2ab --- /dev/null +++ b/lib/postal/http/address_guard.rb @@ -0,0 +1,202 @@ +# frozen_string_literal: true + +require "ipaddr" +require "resolv" +require "socket" + +module Postal + module HTTP + # Guards outbound HTTP requests against SSRF by resolving the destination + # host and refusing to connect to private, loopback, link-local, multicast + # or otherwise reserved addresses (for example cloud metadata endpoints). + # + # Administrators can permit specific destinations by adding hostnames or + # IP/CIDR ranges to the `postal.allowed_request_destinations` config option. + class AddressGuard + + # IP ranges that outbound requests are never allowed to reach unless the + # destination has been explicitly allowlisted. + BLOCKED_RANGES = [ + # IPv4 + "0.0.0.0/8", # "this host on this network" + "10.0.0.0/8", # RFC1918 private + "100.64.0.0/10", # RFC6598 carrier-grade NAT + "127.0.0.0/8", # loopback + "169.254.0.0/16", # link-local (incl. 169.254.169.254 metadata) + "172.16.0.0/12", # RFC1918 private + "192.0.0.0/24", # IETF protocol assignments + "192.168.0.0/16", # RFC1918 private + "198.18.0.0/15", # benchmarking + "224.0.0.0/4", # multicast + "240.0.0.0/4", # reserved + # IPv6 + "::/128", # unspecified + "::1/128", # loopback + "::ffff:0:0/96", # IPv4-mapped (also re-checked against the v4 list) + "fc00::/7", # unique-local + "fe80::/10", # link-local + "ff00::/8", # multicast + ].map { |range| IPAddr.new(range) }.freeze + + class << self + + # Resolve and validate the given host, returning the IP address the + # connection should be pinned to (as a string). Pinning the connection + # to the validated address prevents a DNS-rebinding race between the + # check here and the actual connection. + # + # @param [String] host the hostname or IP literal from the request URL + # @raise [Postal::HTTP::BlockedDestinationError] if the host cannot be + # resolved or any resolved address is not permitted + # @raise [SocketError] if the host only resolves to addresses whose + # family this server cannot reach (e.g. IPv6 with no IPv6 support) + # @return [String] the validated IP address to connect to + def safe_connect_address(host) + new(host).safe_connect_address + end + + # Whether this server has IPv6 connectivity (a global IPv6 address on + # one of its interfaces). Memoized as it does not change at runtime. + def ipv6_supported? + return @ipv6_supported unless @ipv6_supported.nil? + + @ipv6_supported = local_families.include?(:ipv6) + end + + # Whether this server has IPv4 connectivity. Defaults to true unless the + # host clearly only has IPv6, so that a host reporting no global + # addresses at all (e.g. inside a minimal container) still attempts IPv4 + # as it did before this guard existed. + def ipv4_supported? + return @ipv4_supported unless @ipv4_supported.nil? + + families = local_families + @ipv4_supported = families.include?(:ipv4) || !families.include?(:ipv6) + end + + private + + def local_families + families = [] + Socket.ip_address_list.each do |address| + families << :ipv4 if address.ipv4? && !address.ipv4_loopback? + families << :ipv6 if address.ipv6? && !address.ipv6_loopback? && !address.ipv6_linklocal? + end + families.uniq + end + + end + + # @param [String] host + def initialize(host) + @host = host.to_s + end + + def safe_connect_address + if @host.empty? + raise BlockedDestinationError, "No host was given for the request" + end + + addresses = resolve + if addresses.empty? + raise BlockedDestinationError, "Could not resolve '#{@host}' to any IP address" + end + + # Reject the whole request if *any* resolved address is blocked. This is + # checked before the reachability filtering below so that a blocked + # destination is always reported as such, regardless of which address + # families this particular server can reach. It also defeats DNS + # responses that mix a public and a private address to slip past. + addresses.each do |address| + next unless blocked?(address) + + raise BlockedDestinationError, + "Destination '#{@host}' (#{address}) is not permitted" + end + + # Only connect to an address whose family this server can actually + # reach. Otherwise we might pin the connection to an IPv6 address on a + # host without IPv6 connectivity and fail to connect even when a usable + # IPv4 address was available. + usable = addresses.select { |address| family_reachable?(address) } + if usable.empty? + raise SocketError, + "'#{@host}' only resolves to addresses this server cannot reach " \ + "(#{addresses.join(', ')})" + end + + # Prefer IPv4 for predictability; only use IPv6 when it is the only + # reachable option. + (usable.find(&:ipv4?) || usable.first).to_s + end + + private + + # @return [Array] + def resolve + return [IPAddr.new(@host)] if ip_literal? + + Resolv.getaddresses(@host).filter_map do |address| + IPAddr.new(address) + rescue IPAddr::InvalidAddressError + nil + end + end + + def ip_literal? + IPAddr.new(@host) + true + rescue IPAddr::InvalidAddressError + false + end + + # @param [IPAddr] address + def family_reachable?(address) + if address.ipv6? && !address.ipv4_mapped? + self.class.ipv6_supported? + else + self.class.ipv4_supported? + end + end + + # @param [IPAddr] address + def blocked?(address) + return false if allowlisted?(address) + + # IPv4-mapped IPv6 addresses (::ffff:a.b.c.d) must be checked against the + # IPv4 rules using the embedded address, otherwise they bypass the list. + if address.ipv6? && address.ipv4_mapped? + mapped = address.native + return true if mapped.ipv4? && BLOCKED_RANGES.any? { |range| range.include?(mapped) } + end + + BLOCKED_RANGES.any? { |range| range.include?(address) } + end + + # @param [IPAddr] address + def allowlisted?(address) + allowlist.any? do |entry| + if entry.is_a?(IPAddr) + entry.include?(address) + else + entry.casecmp?(@host) + end + end + end + + # Allowlist entries are kept as strings in config. An entry that parses as + # an IP/CIDR is matched against the resolved address; anything else is + # matched against the request hostname (case-insensitively). + # + # @return [Array] + def allowlist + @allowlist ||= Array(Postal::Config.postal.allowed_request_destinations).map do |entry| + IPAddr.new(entry.to_s) + rescue IPAddr::InvalidAddressError + entry.to_s + end + end + + end + end +end diff --git a/lib/postal/http/blocked_destination_error.rb b/lib/postal/http/blocked_destination_error.rb new file mode 100644 index 000000000..c08a4446e --- /dev/null +++ b/lib/postal/http/blocked_destination_error.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Postal + module HTTP + # Raised when an outbound request would be sent to an address that is not + # permitted (a private, loopback, link-local or otherwise reserved address + # that has not been explicitly allowlisted). Used as an SSRF guard. + class BlockedDestinationError < StandardError + end + end +end diff --git a/lib/postal/message_db/database.rb b/lib/postal/message_db/database.rb index 52f456986..415a88deb 100644 --- a/lib/postal/message_db/database.rb +++ b/lib/postal/message_db/database.rb @@ -70,7 +70,7 @@ def new_message(attributes = {}) # Return the total size of all stored messages # def total_size - query("SELECT SUM(size) AS size FROM `#{database_name}`.`raw_message_sizes`").first["size"] || 0 + query("SELECT SUM(size) AS size FROM #{escape_identifier(database_name)}.`raw_message_sizes`").first["size"] || 0 end # @@ -151,11 +151,11 @@ def select(table, options = {}) if options[:count] sql_query << " COUNT(id) AS count" elsif options[:fields] - sql_query << (" " + options[:fields].map { |f| "`#{f}`" }.join(", ")) + sql_query << (" " + options[:fields].map { |f| escape_identifier(f) }.join(", ")) else sql_query << " *" end - sql_query << " FROM `#{database_name}`.`#{table}`" + sql_query << " FROM #{escape_identifier(database_name)}.#{escape_identifier(table)}" if options[:where].present? sql_query << (" " + build_where_string(options[:where], " AND ")) end @@ -163,7 +163,7 @@ def select(table, options = {}) direction = (options[:direction] || "ASC").upcase raise Postal::Error, "Invalid direction #{options[:direction]}" unless %w[ASC DESC].include?(direction) - sql_query << " ORDER BY `#{options[:order]}` #{direction}" + sql_query << " ORDER BY #{escape_identifier(options[:order])} #{direction}" end if options[:limit] @@ -211,7 +211,7 @@ def select_with_pagination(table, page, options = {}) # Will return the total number of affected rows. # def update(table, attributes, options = {}) - sql_query = "UPDATE `#{database_name}`.`#{table}` SET" + sql_query = "UPDATE #{escape_identifier(database_name)}.#{escape_identifier(table)} SET" sql_query << " #{hash_to_sql(attributes)}" if options[:where] sql_query << (" " + build_where_string(options[:where])) @@ -227,8 +227,8 @@ def update(table, attributes, options = {}) # Will return the ID of the new item. # def insert(table, attributes) - sql_query = "INSERT INTO `#{database_name}`.`#{table}`" - sql_query << (" (" + attributes.keys.map { |k| "`#{k}`" }.join(", ") + ")") + sql_query = "INSERT INTO #{escape_identifier(database_name)}.#{escape_identifier(table)}" + sql_query << (" (" + attributes.keys.map { |k| escape_identifier(k) }.join(", ") + ")") sql_query << (" VALUES (" + attributes.values.map { |v| escape(v) }.join(", ") + ")") with_mysql do |mysql| query_on_connection(mysql, sql_query) @@ -243,8 +243,8 @@ def insert_multi(table, keys, values) if values.empty? nil else - sql_query = "INSERT INTO `#{database_name}`.`#{table}`" - sql_query << (" (" + keys.map { |k| "`#{k}`" }.join(", ") + ")") + sql_query = "INSERT INTO #{escape_identifier(database_name)}.#{escape_identifier(table)}" + sql_query << (" (" + keys.map { |k| escape_identifier(k) }.join(", ") + ")") sql_query << " VALUES " sql_query << values.map { |v| "(" + v.map { |r| escape(r) }.join(", ") + ")" }.join(", ") query(sql_query) @@ -260,7 +260,7 @@ def insert_multi(table, keys, values) # Will return the total number of affected rows. # def delete(table, options = {}) - sql_query = "DELETE FROM `#{database_name}`.`#{table}`" + sql_query = "DELETE FROM #{escape_identifier(database_name)}.#{escape_identifier(table)}" sql_query << (" " + build_where_string(options[:where], " AND ")) with_mysql do |mysql| query_on_connection(mysql, sql_query) @@ -351,32 +351,41 @@ def build_where_string(attributes, joiner = ", ") def hash_to_sql(hash, joiner = ", ") hash.map do |key, value| + column = escape_identifier(key) if value.is_a?(Array) && value.all? { |v| v.is_a?(Integer) } - "`#{key}` IN (#{value.join(', ')})" + "#{column} IN (#{value.join(', ')})" elsif value.is_a?(Array) escaped_values = value.map { |v| escape(v) }.join(", ") - "`#{key}` IN (#{escaped_values})" + "#{column} IN (#{escaped_values})" elsif value.is_a?(Hash) sql = [] value.each do |operator, inner_value| case operator when :less_than - sql << "`#{key}` < #{escape(inner_value)}" + sql << "#{column} < #{escape(inner_value)}" when :greater_than - sql << "`#{key}` > #{escape(inner_value)}" + sql << "#{column} > #{escape(inner_value)}" when :less_than_or_equal_to - sql << "`#{key}` <= #{escape(inner_value)}" + sql << "#{column} <= #{escape(inner_value)}" when :greater_than_or_equal_to - sql << "`#{key}` >= #{escape(inner_value)}" + sql << "#{column} >= #{escape(inner_value)}" end end sql.empty? ? "1=1" : sql.join(joiner) else - "`#{key}` = #{escape(value)}" + "#{column} = #{escape(value)}" end end.join(joiner) end + # Escape a value for safe use as a MySQL identifier (e.g. a column or + # table name). Identifiers are wrapped in backticks and any backtick + # within the identifier is doubled so it cannot break out of the quoting + # and inject arbitrary SQL. + def escape_identifier(identifier) + "`" + identifier.to_s.gsub("`", "``") + "`" + end + end end end diff --git a/lib/tracking_middleware.rb b/lib/tracking_middleware.rb index bee3a6378..d66473637 100644 --- a/lib/tracking_middleware.rb +++ b/lib/tracking_middleware.rb @@ -48,25 +48,11 @@ def dispatch_image_request(request, server_token, message_token) Sentry.capture_exception(e) if defined?(Sentry) end - source_image = request.params["src"] - case source_image - when nil + if request.params["src"].nil? headers = {} headers["Content-Type"] = "image/png" headers["Content-Length"] = TRACKING_PIXEL.bytesize.to_s [200, headers, [TRACKING_PIXEL]] - when /\Ahttps?:\/\// - response = Postal::HTTP.get(source_image, timeout: 3) - return [404, {}, ["Not found"]] unless response[:code] == 200 - - headers = {} - headers["Content-Type"] = response[:headers]["content-type"]&.first - headers["Last-Modified"] = response[:headers]["last-modified"]&.first - headers["Cache-Control"] = response[:headers]["cache-control"]&.first - headers["Etag"] = response[:headers]["etag"]&.first - headers["Content-Length"] = response[:body].bytesize.to_s - [200, headers, [response[:body]]] - else [400, {}, ["Invalid/missing source image"]] end diff --git a/spec/apis/legacy_api/messages/deliveries_spec.rb b/spec/apis/legacy_api/messages/deliveries_spec.rb index 2daf95f6b..cfc69cc29 100644 --- a/spec/apis/legacy_api/messages/deliveries_spec.rb +++ b/spec/apis/legacy_api/messages/deliveries_spec.rb @@ -64,6 +64,23 @@ end end + # Regression test for GHSA-x2hq-rfpg-3xr5 (see message_spec.rb). A JSON + # object supplied for `id` must be rejected before reaching the database + # rather than being interpreted as a raw set of SQL conditions. + context "when the message ID is a JSON object (SQL injection attempt)" do + it "rejects it with a parameter error and never reaches the database" do + expect_any_instance_of(Server).not_to receive(:message) + post "/api/v1/messages/deliveries", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: { "id`=0 OR SLEEP(5)#" => "x" } }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "parameter-error" + expect(parsed_body["data"]["message"]).to match(/must be a string or integer/) + end + end + context "when the message ID exists" do let(:server) { create(:server) } let(:credential) { create(:credential, server: server) } diff --git a/spec/apis/legacy_api/messages/message_spec.rb b/spec/apis/legacy_api/messages/message_spec.rb index 202b66b50..c40d756dc 100644 --- a/spec/apis/legacy_api/messages/message_spec.rb +++ b/spec/apis/legacy_api/messages/message_spec.rb @@ -63,6 +63,56 @@ end end + # Regression tests for GHSA-x2hq-rfpg-3xr5. The request body is parsed as + # JSON, so a JSON object/array supplied for `id` would otherwise arrive as + # a Ruby Hash/Array and be passed straight through to the message database + # as a raw set of SQL conditions (blind SQL injection). These must be + # rejected before reaching the database. + context "when the message ID is a JSON object (SQL injection attempt)" do + it "rejects it with a parameter error and never reaches the database" do + expect_any_instance_of(Server).not_to receive(:message) + post "/api/v1/messages/message", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: { "id`=0 OR SLEEP(5)#" => "x" } }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "parameter-error" + expect(parsed_body["data"]["message"]).to match(/must be a string or integer/) + end + end + + context "when the message ID is a JSON array" do + it "rejects it with a parameter error" do + post "/api/v1/messages/message", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: [1, 2, 3] }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "parameter-error" + expect(parsed_body["data"]["message"]).to match(/must be a string or integer/) + end + end + + context "when the message ID is provided as a numeric string" do + let(:message) { MessageFactory.outgoing(server) } + + it "is coerced to an integer and looks the message up" do + post "/api/v1/messages/message", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: message.id.to_s }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "success" + expect(parsed_body["data"]).to match({ + "id" => message.id, + "token" => message.token + }) + end + end + context "when the message ID exists" do let(:server) { create(:server) } let(:credential) { create(:credential, server: server) } diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb new file mode 100644 index 000000000..4beb3de98 --- /dev/null +++ b/spec/helpers/application_helper_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe ApplicationHelper, type: :helper do + describe "#endpoint_options_for_select" do + let(:server) { create(:server) } + + context "when an endpoint has HTML characters in its description" do + let(:payload) { %q(x'">) } + + before do + create(:http_endpoint, server: server, name: payload) + end + + it "HTML-escapes the endpoint description in the option text" do + html = helper.endpoint_options_for_select(server) + + # The raw payload must not appear verbatim — if it does, the browser + # will execute the ") + + # Escaped form should appear instead. + expect(html).to include("<script>alert(1)</script>") + end + + it "does not allow the payload to break out of the option tag" do + html = helper.endpoint_options_for_select(server) + + # The ' and > characters in the payload must be escaped so they + # cannot close the opening