Skip to content

Commit d0d7733

Browse files
const-cloudinaryconst-cloudinary
authored
Fix API parameters signature
1 parent 7ffc9d1 commit d0d7733

File tree

2 files changed

+118
-9
lines changed

2 files changed

+118
-9
lines changed

‎lib/cloudinary/utils.rb‎

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -469,12 +469,49 @@ def self.text_style(layer)
469469
end
470470
end
471471

472-
def self.api_string_to_sign(params_to_sign)
473-
params_to_sign.map{|k,v| [k.to_s, v.is_a?(Array) ? v.join(",") : v]}.reject{|k,v| v.nil? || v == ""}.sort_by(&:first).map{|k,v| "#{k}=#{v}"}.join("&")
472+
# Encodes a parameter for safe inclusion in URL query strings.
473+
#
474+
# Specifically replaces "&" characters with their percent-encoded equivalent "%26"
475+
# to prevent them from being interpreted as parameter separators in URL query strings.
476+
#
477+
# @param [Object] value The parameter to encode
478+
# @return [String] Encoded parameter
479+
# @private
480+
def self.encode_param(value)
481+
value.to_s.gsub("&", "%26")
474482
end
475483

476-
def self.api_sign_request(params_to_sign, api_secret, signature_algorithm = nil)
477-
to_sign = api_string_to_sign(params_to_sign)
484+
# Generates a string to be signed for API requests
485+
#
486+
# @param [Hash] params_to_sign Parameters to include in the signature
487+
# @param [Integer] signature_version Version of signature algorithm to use:
488+
# - Version 1: Original behavior without parameter encoding
489+
# - Version 2+ (default): Includes parameter encoding to prevent parameter smuggling
490+
# @return [String] String to be signed
491+
# @private
492+
def self.api_string_to_sign(params_to_sign, signature_version = 2)
493+
params_to_sign.map { |k, v| [k.to_s, v.is_a?(Array) ? v.join(",") : v] }
494+
.reject { |k, v| v.nil? || v == "" }
495+
.sort_by(&:first)
496+
.map { |k, v|
497+
param_string = "#{k}=#{v}"
498+
signature_version >= 2 ? encode_param(param_string) : param_string
499+
}
500+
.join("&")
501+
end
502+
503+
# Signs API request parameters
504+
#
505+
# @param [Hash] params_to_sign Parameters to include in the signature
506+
# @param [String] api_secret API secret for signing
507+
# @param [Symbol] signature_algorithm Hash algorithm to use (:sha1 or :sha256)
508+
# @param [Integer] signature_version Version of signature algorithm to use:
509+
# - Version 1: Original behavior without parameter encoding
510+
# - Version 2+ (default): Includes parameter encoding to prevent parameter smuggling
511+
# @return [String] Hexadecimal signature
512+
# @private
513+
def self.api_sign_request(params_to_sign, api_secret, signature_algorithm = nil, signature_version = 2)
514+
to_sign = api_string_to_sign(params_to_sign, signature_version)
478515
hash("#{to_sign}#{api_secret}", signature_algorithm, :hexdigest)
479516
end
480517

@@ -1351,7 +1388,7 @@ def self.verify_api_response_signature(public_id, version, signature, signature_
13511388
:version => version
13521389
}
13531390

1354-
signature == api_sign_request(parameters_to_sign, api_secret, signature_algorithm)
1391+
signature == api_sign_request(parameters_to_sign, api_secret, signature_algorithm, 1)
13551392
end
13561393

13571394
# Verifies the authenticity of a notification signature.

‎spec/utils_spec.rb‎

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33

44
describe Cloudinary::Utils do
55
SIGNATURE_VERIFICATION_API_SECRET = "X7qLTrsES31MzxxkxPPA-pAGGfU"
6+
API_SIGN_REQUEST_TEST_SECRET = "hdcixPpR2iKERPwqvH6sHdK9cyac"
7+
API_SIGN_REQUEST_CLOUD_NAME = "dn6ot3ged"
68

79
before :each do
810
Cloudinary.reset_config
@@ -971,28 +973,61 @@
971973
end
972974

973975
it "should sign an API request using SHA1 by default" do
974-
signature = Cloudinary::Utils.api_sign_request({ :cloud_name => "dn6ot3ged", :timestamp => 1568810420, :username => "[email protected]" }, "hdcixPpR2iKERPwqvH6sHdK9cyac")
976+
signature = Cloudinary::Utils.api_sign_request({ :cloud_name => API_SIGN_REQUEST_CLOUD_NAME, :timestamp => 1568810420, :username => "[email protected]" }, API_SIGN_REQUEST_TEST_SECRET)
975977
expect(signature).to eq("14c00ba6d0dfdedbc86b316847d95b9e6cd46d94")
976978
end
977979

978980
it "should sign an API request using SHA256" do
979981
Cloudinary.config.signature_algorithm = Cloudinary::Utils::ALGO_SHA256
980-
signature = Cloudinary::Utils.api_sign_request({ :cloud_name => "dn6ot3ged", :timestamp => 1568810420, :username => "[email protected]" }, "hdcixPpR2iKERPwqvH6sHdK9cyac")
982+
signature = Cloudinary::Utils.api_sign_request({ :cloud_name => API_SIGN_REQUEST_CLOUD_NAME, :timestamp => 1568810420, :username => "[email protected]" }, API_SIGN_REQUEST_TEST_SECRET)
981983
expect(signature).to eq("45ddaa4fa01f0c2826f32f669d2e4514faf275fe6df053f1a150e7beae58a3bd")
982984
end
983985

984986
it "should sign an API request using SHA256 via parameter" do
985-
signature = Cloudinary::Utils.api_sign_request({ :cloud_name => "dn6ot3ged", :timestamp => 1568810420, :username => "[email protected]" }, "hdcixPpR2iKERPwqvH6sHdK9cyac", :sha256)
987+
signature = Cloudinary::Utils.api_sign_request({ :cloud_name => API_SIGN_REQUEST_CLOUD_NAME, :timestamp => 1568810420, :username => "[email protected]" }, API_SIGN_REQUEST_TEST_SECRET, :sha256)
986988
expect(signature).to eq("45ddaa4fa01f0c2826f32f669d2e4514faf275fe6df053f1a150e7beae58a3bd")
987989
end
988990

989991
it "should raise when unsupported algorithm is passed" do
990992
signature_algorithm = "unsupported_algorithm"
991993

992-
expect { Cloudinary::Utils.api_sign_request({ :cloud_name => "dn6ot3ged", :timestamp => 1568810420, :username => "[email protected]" }, "hdcixPpR2iKERPwqvH6sHdK9cyac", signature_algorithm) }
994+
expect { Cloudinary::Utils.api_sign_request({ :cloud_name => API_SIGN_REQUEST_CLOUD_NAME, :timestamp => 1568810420, :username => "[email protected]" }, API_SIGN_REQUEST_TEST_SECRET, signature_algorithm) }
993995
.to raise_error("Unsupported algorithm 'unsupported_algorithm'")
994996
end
995997

998+
it "should prevent parameter smuggling via & characters in parameter values with signature version 2" do
999+
params_with_ampersand = {
1000+
:cloud_name => API_SIGN_REQUEST_CLOUD_NAME,
1001+
:timestamp => 1568810420,
1002+
:notification_url => "https://fake.com/callback?a=1&tags=hello,world"
1003+
}
1004+
1005+
signature_v1_with_ampersand = Cloudinary::Utils.api_sign_request(params_with_ampersand, API_SIGN_REQUEST_TEST_SECRET, nil, 1)
1006+
signature_v2_with_ampersand = Cloudinary::Utils.api_sign_request(params_with_ampersand, API_SIGN_REQUEST_TEST_SECRET, nil, 2)
1007+
1008+
params_smuggled = {
1009+
:cloud_name => API_SIGN_REQUEST_CLOUD_NAME,
1010+
:timestamp => 1568810420,
1011+
:notification_url => "https://fake.com/callback?a=1",
1012+
:tags => "hello,world"
1013+
}
1014+
1015+
signature_v1_smuggled = Cloudinary::Utils.api_sign_request(params_smuggled, API_SIGN_REQUEST_TEST_SECRET, nil, 1)
1016+
signature_v2_smuggled = Cloudinary::Utils.api_sign_request(params_smuggled, API_SIGN_REQUEST_TEST_SECRET, nil, 2)
1017+
1018+
# Version 1 is vulnerable to parameter smuggling
1019+
expect(signature_v1_with_ampersand).to eq(signature_v1_smuggled)
1020+
1021+
# Version 2 prevents parameter smuggling
1022+
expect(signature_v2_with_ampersand).not_to eq(signature_v2_smuggled)
1023+
1024+
expected_v2_signature = "4fdf465dd89451cc1ed8ec5b3e314e8a51695704"
1025+
expect(signature_v2_with_ampersand).to eq(expected_v2_signature)
1026+
1027+
expected_v2_smuggled_signature = "7b4e3a539ff1fa6e6700c41b3a2ee77586a025f9"
1028+
expect(signature_v2_smuggled).to eq(expected_v2_smuggled_signature)
1029+
end
1030+
9961031
describe ":if" do
9971032
describe 'with literal condition string' do
9981033
it "should include the if parameter as the first component in the transformation string" do
@@ -1284,6 +1319,43 @@
12841319
Cloudinary::Utils::ALGO_SHA256)
12851320
).to be true
12861321
end
1322+
1323+
it "should use signature version 1 (without parameter encoding) for backward compatibility" do
1324+
public_id_with_ampersand = 'tests/logo&version=2'
1325+
1326+
expected_signature_v1 = Cloudinary::Utils.api_sign_request(
1327+
{ :public_id => public_id_with_ampersand, :version => test_version },
1328+
SIGNATURE_VERIFICATION_API_SECRET,
1329+
nil,
1330+
1
1331+
)
1332+
1333+
expected_signature_v2 = Cloudinary::Utils.api_sign_request(
1334+
{ :public_id => public_id_with_ampersand, :version => test_version },
1335+
SIGNATURE_VERIFICATION_API_SECRET,
1336+
nil,
1337+
2
1338+
)
1339+
1340+
expect(expected_signature_v1).not_to eq(expected_signature_v2)
1341+
1342+
# verify_api_response_signature should use version 1 for backward compatibility
1343+
expect(
1344+
Cloudinary::Utils.verify_api_response_signature(
1345+
public_id_with_ampersand,
1346+
test_version,
1347+
expected_signature_v1
1348+
)
1349+
).to be true
1350+
1351+
expect(
1352+
Cloudinary::Utils.verify_api_response_signature(
1353+
public_id_with_ampersand,
1354+
test_version,
1355+
expected_signature_v2
1356+
)
1357+
).to be false
1358+
end
12871359
end
12881360

12891361
describe ".verify_notification_signature" do

0 commit comments

Comments
 (0)