From f2ccd7d3942a3369a8f77e00d6c783b6d026ccd7 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Mon, 7 Nov 2022 23:14:21 -0800 Subject: [PATCH] Several more RSA scenarios --- lib/brakeman/checks/check_weak_rsa_key.rb | 120 +++++++++++++--------- test/apps/rails7/lib/some_lib.rb | 11 ++ 2 files changed, 85 insertions(+), 46 deletions(-) diff --git a/lib/brakeman/checks/check_weak_rsa_key.rb b/lib/brakeman/checks/check_weak_rsa_key.rb index ef54e562df..bbeb38a564 100644 --- a/lib/brakeman/checks/check_weak_rsa_key.rb +++ b/lib/brakeman/checks/check_weak_rsa_key.rb @@ -7,71 +7,99 @@ class Brakeman::CheckWeakRSAKey < Brakeman::BaseCheck def run_check tracker.find_call(targets: [:'OpenSSL::PKey::RSA'], method: [:new, :generate], nested: true).each do |result| - check_key_size(result) + key_size_arg = result[:call].first_arg + check_key_size(result, key_size_arg) end - tracker.find_call(targets: [:'OpenSSL::PKey::RSA.new'], method: [:public_encrypt, :public_decrypt, :private_encrypt, :private_decrypt], nested: true).each do |result| - check_padding(result) - end - end + tracker.find_call(targets: [:'OpenSSL::PKey'], method: [:generate_key], nested: true).each do |result| + call = result[:call] + key_type = call.first_arg + options_arg = call.second_arg - def check_key_size result - return unless original? result + next unless options_arg and hash? options_arg + + if string? key_type and key_type.value.upcase == 'RSA' + key_size_arg = (hash_access(options_arg, :rsa_keygen_bits) || hash_access(options_arg, s(:str, 'rsa_key_gen_bits'))) + check_key_size(result, key_size_arg) + end + end - first_arg = result[:call].first_arg + tracker.find_call(targets: [:'OpenSSL::PKey::RSA.new'], methods: [:public_encrypt, :public_decrypt, :private_encrypt, :private_decrypt], nested: true).each do |result| + padding_arg = result[:call].second_arg + check_padding(result, padding_arg) + end - if number? first_arg - key_size = first_arg.value + tracker.find_call(targets: [:'OpenSSL::PKey.generate_key'], methods: [:encrypt, :decrypt, :sign, :verify, :sign_raw, :verify_raw], nested: true).each do |result| + call = result[:call] + options_arg = call.last_arg - if key_size < 1024 - confidence = :high - message = msg("RSA key with size ", msg_code(key_size.to_s), " is considered very weak. Use at least 2048 bit key size") - elsif key_size < 2048 - confidence = :medium - message = msg("RSA key with size ", msg_code(key_size.to_s), " is considered weak. Use at least 2048 bit key size") + if options_arg and hash? options_arg + padding_arg = (hash_access(options_arg, :rsa_padding_mode) || hash_access(options_arg, s(:str, 'rsa_padding_mode'))) else - return + padding_arg = nil end - warn result: result, - warning_type: "Weak Cryptography", - warning_code: :small_rsa_key_size, - message: message, - confidence: confidence, - user_input: first_arg, - cwe_id: [326] + check_padding(result, padding_arg) + end + end + + def check_key_size result, key_size_arg + return unless number? key_size_arg + return unless original? result + + key_size = key_size_arg.value + + if key_size < 1024 + confidence = :high + message = msg("RSA key with size ", msg_code(key_size.to_s), " is considered very weak. Use at least 2048 bit key size") + elsif key_size < 2048 + confidence = :medium + message = msg("RSA key with size ", msg_code(key_size.to_s), " is considered weak. Use at least 2048 bit key size") + else + return end + + warn result: result, + warning_type: "Weak Cryptography", + warning_code: :small_rsa_key_size, + message: message, + confidence: confidence, + user_input: key_size_arg, + cwe_id: [326] end PKCS1_PADDING = s(:colon2, s(:colon2, s(:colon2, s(:const, :OpenSSL), :PKey), :RSA), :PKCS1_PADDING).freeze + PKCS1_PADDING_STR = s(:str, 'pkcs1').freeze + SSLV23_PADDING = s(:colon2, s(:colon2, s(:colon2, s(:const, :OpenSSL), :PKey), :RSA), :SSLV23_PADDING).freeze + SSLV23_PADDING_STR = s(:str, 'sslv23').freeze NO_PADDING = s(:colon2, s(:colon2, s(:colon2, s(:const, :OpenSSL), :PKey), :RSA), :NO_PADDING).freeze + NO_PADDING_STR = s(:str, 'none').freeze - def check_padding result + def check_padding result, padding_arg return unless original? result - padding_arg = result[:call].second_arg + if string? padding_arg + padding_arg = padding_arg.deep_clone(padding_arg.line) + padding_arg.value.downcase! + end case padding_arg - when PKCS1_PADDING, nil - message = "Use of padding mode PKCS1 (default if not specified), which is known to be insecure" - - warn result: result, - warning_type: "Weak Cryptography", - warning_code: :insecure_rsa_padding_mode, - message: message, - confidence: :high, - user_input: padding_arg, - cwe_id: [780] - when NO_PADDING - message = "No padding mode used for RSA key. A safe padding mode should be specified for RSA keys" - - warn result: result, - warning_type: "Weak Cryptography", - warning_code: :missing_rsa_padding_mode, - message: message, - confidence: :high, - user_input: padding_arg, - cwe_id: [780] + when PKCS1_PADDING, PKCS1_PADDING_STR, nil + message = "Use of padding mode PKCS1 (default if not specified), which is known to be insecure. Use OAEP instead" + when SSLV23_PADDING, SSLV23_PADDING_STR + message = "Use of padding mode SSLV23 for RSA key, which is only useful for outdated versions of SSL. Use OAEP instead" + when NO_PADDING, NO_PADDING_STR + message = "No padding mode used for RSA key. A safe padding mode (OAEP) should be specified for RSA keys" + else + return end + + warn result: result, + warning_type: "Weak Cryptography", + warning_code: :insecure_rsa_padding_mode, + message: message, + confidence: :high, + user_input: padding_arg, + cwe_id: [780] end end diff --git a/test/apps/rails7/lib/some_lib.rb b/test/apps/rails7/lib/some_lib.rb index b9c98d7e75..08733ed866 100644 --- a/test/apps/rails7/lib/some_lib.rb +++ b/test/apps/rails7/lib/some_lib.rb @@ -17,4 +17,15 @@ def small_rsa_keys OpenSSL::PKey::RSA.new(1024) # Weak OpenSSL::PKey::RSA.new(2048) # Okay end + + def pky_api + weak_rsa = OpenSSL::PKey.generate_key("rsa", rsa_keygen_bits: 1024) # Medium warning about key size + weak_encrypted = weak_rsa.encrypt("data", "rsa_padding_mode" => "pkcs1") + weak_encrypted = weak_rsa.decrypt("data", "rsa_padding_mode" => "oaep") + weak_signature_digest = weak_rsa.sign("SHA256", "data", rsa_padding_mode: "PKCS1") + weak_rsa.verify("SHA256", "data", rsa_padding_mode: "none") + weak_rsa.sign_raw(nil, "data", rsa_padding_mode: "none") + weak_rsa.verify_raw(nil, "data", rsa_padding_mode: "none") + weak_rsa.encrypt("data") # default is also pkcs1 + end end