From d4bfc8c30c87f736438bb7c592e75c0dc0eeefa3 Mon Sep 17 00:00:00 2001 From: Nelson Wittwer Date: Mon, 2 Feb 2026 21:25:38 -0500 Subject: [PATCH] demonstrate hacker one problem --- Gemfile.lock | 14 +++---- .../app_proxy_verification.rb | 9 +++- .../app_proxy_verification_test.rb | 42 +++++++++++++++++++ 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 95eaf8f8e..d10e11729 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -145,6 +145,7 @@ GEM marcel (1.1.0) method_source (1.0.0) mini_mime (1.1.5) + mini_portile2 (2.8.9) minitest (5.26.2) mocha (2.8.0) ruby2_keywords (>= 0.0.5) @@ -161,11 +162,8 @@ GEM net-smtp (0.5.1) net-protocol nio4r (2.7.5) - nokogiri (1.18.10-arm64-darwin) - racc (~> 1.4) - nokogiri (1.18.10-x86_64-darwin) - racc (~> 1.4) - nokogiri (1.18.10-x86_64-linux-gnu) + nokogiri (1.18.10) + mini_portile2 (~> 2.8.2) racc (~> 1.4) oj (3.16.13) bigdecimal (>= 3.0) @@ -291,9 +289,8 @@ GEM actionpack (>= 6.1) activesupport (>= 6.1) sprockets (>= 3.0.0) - sqlite3 (2.7.4-arm64-darwin) - sqlite3 (2.7.4-x86_64-darwin) - sqlite3 (2.7.4-x86_64-linux-gnu) + sqlite3 (2.7.4) + mini_portile2 (~> 2.8.0) stringio (3.1.8) syntax_tree (6.1.1) prettier_print (>= 1.2.0) @@ -317,6 +314,7 @@ PLATFORMS arm64-darwin-21 arm64-darwin-22 arm64-darwin-23 + arm64-darwin-24 x86_64-darwin-19 x86_64-darwin-20 x86_64-darwin-21 diff --git a/lib/shopify_app/controller_concerns/app_proxy_verification.rb b/lib/shopify_app/controller_concerns/app_proxy_verification.rb index d9b02bfc8..c093a0050 100644 --- a/lib/shopify_app/controller_concerns/app_proxy_verification.rb +++ b/lib/shopify_app/controller_concerns/app_proxy_verification.rb @@ -20,6 +20,13 @@ def query_string_valid?(query_string) signature = query_hash.delete("signature") return false if signature.nil? + # Reject requests with array parameters to prevent HMAC signature bypass. + # Shopify's App Proxy never sends duplicate query parameters, so any array + # values indicate a potential canonicalization attack where an attacker + # could swap "ids=1,2" (string) with "ids=1&ids=2" (array) while maintaining + # the same signature. + return false if query_hash.values.any? { |v| v.is_a?(Array) } + ActiveSupport::SecurityUtils.secure_compare( calculated_signature(query_hash), signature, @@ -27,7 +34,7 @@ def query_string_valid?(query_string) end def calculated_signature(query_hash_without_signature) - sorted_params = query_hash_without_signature.collect { |k, v| "#{k}=#{Array(v).join(",")}" }.sort.join + sorted_params = query_hash_without_signature.collect { |k, v| "#{k}=#{v}" }.sort.join OpenSSL::HMAC.hexdigest( OpenSSL::Digest.new("sha256"), diff --git a/test/shopify_app/controller_concerns/app_proxy_verification_test.rb b/test/shopify_app/controller_concerns/app_proxy_verification_test.rb index ff0f6f230..f93e1a1ec 100644 --- a/test/shopify_app/controller_concerns/app_proxy_verification_test.rb +++ b/test/shopify_app/controller_concerns/app_proxy_verification_test.rb @@ -71,6 +71,48 @@ class AppProxyVerificationTest < ActionController::TestCase end end + test "HMAC signature bypass via canonicalization collision is prevented" do + with_test_routes do + # VULNERABILITY: The calculated_signature method uses Array(v).join(",") + # which creates a collision where "1,2" (string) and ["1","2"] (array) + # produce the same canonical form, allowing signature replay attacks. + + timestamp = "1466106083" + secret = ShopifyApp.configuration.secret + + # Step 1: Attacker captures/knows a valid signature for STRING parameter "ids=1,2" + params_for_signing = { + "ids" => "1,2", + "path_prefix" => "/apps/my-app", + "shop" => "some-random-store.myshopify.com", + "timestamp" => timestamp, + } + + # This replicates the vulnerable canonicalization: Array("1,2").join(",") => "1,2" + sorted_params = params_for_signing.collect { |k, v| "#{k}=#{Array(v).join(",")}" }.sort.join + valid_signature = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), secret, sorted_params) + + # Step 2: Verify the original string-based request works + string_query = "ids=1,2&path_prefix=/apps/my-app&shop=some-random-store.myshopify.com&" \ + "timestamp=#{timestamp}&signature=#{valid_signature}" + @request.env["QUERY_STRING"] = string_query + get :basic + assert_response :ok, "Legitimate string parameter request should succeed" + + # Step 3: ATTACK - Send ARRAY ["1", "2"] using the STRING's signature + # Rack parses duplicate params (ids=1&ids=2) as an array ["1", "2"] + # The vulnerable code: Array(["1","2"]).join(",") => "1,2" (same as string!) + array_query = "ids=1&ids=2&path_prefix=/apps/my-app&shop=some-random-store.myshopify.com&" \ + "timestamp=#{timestamp}&signature=#{valid_signature}" + @request.env["QUERY_STRING"] = array_query + get :basic + + # Step 4: This MUST return 403 Forbidden to prevent the attack + # If this returns 200 OK, the vulnerability exists! + assert_response :forbidden, "Array parameter smuggling attack should be rejected" + end + end + private def query_string_valid?(query_string)