Use secure_compare during signature verification (#553)

Author: hieuk09

CHANGELOG.md

@@ -1,6 +1,7 @@
 ### 2.5.3 (Next)
 
 * [#549](https://github.com/slack-ruby/slack-ruby-client/pull/549): Add group ID formatting support for message mentions - [@n0h0](https://github.com/n0h0).
+* [#553](https://github.com/slack-ruby/slack-ruby-client/pull/553): Use `secure_compare` during signature verification - [@hieuk09](https://github.com/hieuk09).
 * Your contribution here.
 
 ### 2.5.2 (2025/02/19)

lib/slack-ruby-client.rb

@@ -22,7 +22,7 @@
 begin
   require 'openssl'
 rescue LoadError # rubocop:disable Lint/SuppressedException
-  # Used in slack/web/config
+  # Used in slack/web/config and slack/utils/security
 end
 require_relative 'slack/web/config'
 require_relative 'slack/web/api/errors/slack_error'
@@ -57,3 +57,5 @@
 # Events API
 require_relative 'slack/events/config'
 require_relative 'slack/events/request'
+
+require_relative 'slack/utils/security'

lib/slack/events/request.rb

@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 module Slack
   module Events
     class Request
@@ -59,7 +60,7 @@ def valid?
         signature_basestring = [version, timestamp, body].join(':')
         hex_hash = OpenSSL::HMAC.hexdigest(digest, signing_secret, signature_basestring)
         computed_signature = [version, hex_hash].join('=')
-        computed_signature == signature
+        Utils::Security.secure_compare(computed_signature, signature)
       end
 
       # Validates the request signature and its expiration.

lib/slack/utils/security.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+# This module is copied from activesupport
+# https://github.com/rails/rails/blob/3235827585d87661942c91bc81f64f56d710f0b2/activesupport/lib/active_support/security_utils.rb
+module Slack
+  module Utils
+    # rubocop:disable Naming/MethodParameterName
+    module Security
+      # Constant time string comparison, for fixed length strings.
+      #
+      # The values compared should be of fixed length, such as strings
+      # that have already been processed by HMAC. Raises in case of length mismatch.
+
+      if defined?(OpenSSL.fixed_length_secure_compare)
+        def fixed_length_secure_compare(a, b)
+          OpenSSL.fixed_length_secure_compare(a, b)
+        end
+      else
+        def fixed_length_secure_compare(a, b)
+          raise ArgumentError, 'inputs must be of equal length' unless a.bytesize == b.bytesize
+
+          l = a.unpack "C#{a.bytesize}"
+
+          res = 0
+          b.each_byte { |byte| res |= byte ^ l.shift }
+          res.zero?
+        end
+      end
+      module_function :fixed_length_secure_compare
+
+      # Secure string comparison for strings of variable length.
+      #
+      # While a timing attack would not be able to discern the content of
+      # a secret compared via secure_compare, it is possible to determine
+      # the secret length. This should be considered when using secure_compare
+      # to compare weak, short secrets to user input.
+      def secure_compare(a, b)
+        a.bytesize == b.bytesize && fixed_length_secure_compare(a, b)
+      end
+      module_function :secure_compare
+    end
+    # rubocop:enable Naming/MethodParameterName
+  end
+end

spec/slack/utils/security_spec.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'slack/utils/security'
+
+RSpec.describe Slack::Utils::Security do
+  describe '.secure_compare' do
+    it 'performs string comparison correctly' do
+      expect(described_class.secure_compare('a', 'a')).to be true
+      expect(described_class.secure_compare('a', 'b')).to be false
+    end
+
+    it 'returns false on bytesize mismatch' do
+      expect(described_class.secure_compare('a', "\u{ff41}")).to be false
+    end
+  end
+
+  describe '.fixed_length_secure_compare' do
+    it 'performs string comparison correctly' do
+      expect(described_class.fixed_length_secure_compare('a', 'a')).to be true
+      expect(described_class.fixed_length_secure_compare('a', 'b')).to be false
+    end
+
+    it 'raises ArgumentError on length mismatch' do
+      expect do
+        described_class.fixed_length_secure_compare('a', 'ab')
+      end.to raise_error(ArgumentError, 'inputs must be of equal length')
+    end
+  end
+end