From 777b6713bb473d2e09c8340ab9a96373fdbaae50 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 22 Nov 2018 15:35:49 +0100 Subject: [PATCH] Ensure that db encryption keys have proper bytesize --- config/settings.rb | 8 +++ lib/gitlab/crypto_helper.rb | 4 +- lib/gitlab/utils.rb | 14 +++++ spec/config/settings_spec.rb | 98 +++++++++++++++++++++++++++++++++++ spec/lib/gitlab/utils_spec.rb | 38 ++++++++++++++ 5 files changed, 160 insertions(+), 2 deletions(-) diff --git a/config/settings.rb b/config/settings.rb index 3f3481bb65d..1b94df785a7 100644 --- a/config/settings.rb +++ b/config/settings.rb @@ -95,6 +95,14 @@ class Settings < Settingslogic Gitlab::Application.secrets.db_key_base[0..31] end + def attr_encrypted_db_key_base_32 + Gitlab::Utils.ensure_utf8_size(attr_encrypted_db_key_base, bytes: 32.bytes) + end + + def attr_encrypted_db_key_base_12 + Gitlab::Utils.ensure_utf8_size(attr_encrypted_db_key_base, bytes: 12.bytes) + end + # This should be used for :per_attribute_salt_and_iv mode. There is no # need to truncate the key because the encryptor will use the salt to # generate a hash of the password: diff --git a/lib/gitlab/crypto_helper.rb b/lib/gitlab/crypto_helper.rb index e85753c3a12..87a03d9c58f 100644 --- a/lib/gitlab/crypto_helper.rb +++ b/lib/gitlab/crypto_helper.rb @@ -6,8 +6,8 @@ module Gitlab AES256_GCM_OPTIONS = { algorithm: 'aes-256-gcm', - key: Settings.attr_encrypted_db_key_base_truncated, - iv: Settings.attr_encrypted_db_key_base_truncated[0..11] + key: Settings.attr_encrypted_db_key_base_32, + iv: Settings.attr_encrypted_db_key_base_12 }.freeze def sha256(value) diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index 9e59137a2c0..ad2efa6b4e1 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -16,6 +16,20 @@ module Gitlab str.force_encoding(Encoding::UTF_8) end + def ensure_utf8_size(str, bytes:) + raise ArgumentError if str.empty? || bytes.negative? + + truncated = str.each_char.each_with_object(String.new) do |char, object| + if object.bytesize + char.bytesize > bytes + break object + else + object.concat(char) + end + end + + truncated + ("\0" * (bytes - truncated.bytesize)) + end + # Append path to host, making sure there's one single / in between def append_path(host, path) "#{host.to_s.sub(%r{\/+$}, '')}/#{path.to_s.sub(%r{^\/+}, '')}" diff --git a/spec/config/settings_spec.rb b/spec/config/settings_spec.rb index 83b2de47741..fdbd0cb8990 100644 --- a/spec/config/settings_spec.rb +++ b/spec/config/settings_spec.rb @@ -6,4 +6,102 @@ describe Settings do expect(described_class.omniauth.enabled).to be true end end + + describe '.attr_encrypted_db_key_base_truncated' do + it 'is a string with maximum 32 bytes size' do + expect(described_class.attr_encrypted_db_key_base_truncated.bytesize) + .to be <= 32 + end + end + + describe '.attr_encrypted_db_key_base_12' do + context 'when db key base secret is less than 12 bytes' do + before do + allow(described_class) + .to receive(:attr_encrypted_db_key_base) + .and_return('a' * 10) + end + + it 'expands db key base secret to 12 bytes' do + expect(described_class.attr_encrypted_db_key_base_12) + .to eq ('a' * 10) + ("\0" * 2) + end + end + + context 'when key has multiple multi-byte UTF chars exceeding 12 bytes' do + before do + allow(described_class) + .to receive(:attr_encrypted_db_key_base) + .and_return('❤' * 18) + end + + it 'does not use more than 32 bytes' do + db_key_base = described_class.attr_encrypted_db_key_base_12 + + expect(db_key_base).to eq ('❤' * 4) + expect(db_key_base.bytesize).to eq 12 + end + end + end + + describe '.attr_encrypted_db_key_base_32' do + context 'when db key base secret is less than 32 bytes' do + before do + allow(described_class) + .to receive(:attr_encrypted_db_key_base) + .and_return('a' * 10) + end + + it 'expands db key base secret to 32 bytes' do + expanded_key_base = ('a' * 10) + ("\0" * 22) + + expect(expanded_key_base.bytesize).to eq 32 + expect(described_class.attr_encrypted_db_key_base_32) + .to eq expanded_key_base + end + end + + context 'when db key base secret is 32 bytes' do + before do + allow(described_class) + .to receive(:attr_encrypted_db_key_base) + .and_return('a' * 32) + end + + it 'returns original value' do + expect(described_class.attr_encrypted_db_key_base_32) + .to eq 'a' * 32 + end + end + + context 'when db key base contains multi-byte UTF character' do + before do + allow(described_class) + .to receive(:attr_encrypted_db_key_base) + .and_return('❤' * 6) + end + + it 'does not use more than 32 bytes' do + db_key_base = described_class.attr_encrypted_db_key_base_32 + + expect(db_key_base).to eq '❤❤❤❤❤❤' + ("\0" * 14) + expect(db_key_base.bytesize).to eq 32 + end + end + + context 'when db key base multi-byte UTF chars exceeding 32 bytes' do + before do + allow(described_class) + .to receive(:attr_encrypted_db_key_base) + .and_return('❤' * 18) + end + + it 'does not use more than 32 bytes' do + db_key_base = described_class.attr_encrypted_db_key_base_32 + + expect(db_key_base).to eq ('❤' * 10) + ("\0" * 2) + expect(db_key_base.bytesize).to eq 32 + end + end + end end diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index ad2c9d7f2af..9927ad41108 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -127,4 +127,42 @@ describe Gitlab::Utils do end end end + + describe '.ensure_utf8_size' do + context 'string is has less bytes than expected' do + it 'backfills string with null characters' do + transformed = described_class.ensure_utf8_size('a' * 10, bytes: 32) + + expect(transformed.bytesize).to eq 32 + expect(transformed).to eq ('a' * 10) + ("\0" * 22) + end + end + + context 'string size is exactly the one that is expected' do + it 'returns original value' do + transformed = described_class.ensure_utf8_size('a' * 32, bytes: 32) + + expect(transformed).to eq 'a' * 32 + expect(transformed.bytesize).to eq 32 + end + end + + context 'when string contains a few multi-byte UTF characters' do + it 'backfills string with null characters' do + transformed = described_class.ensure_utf8_size('❤' * 6, bytes: 32) + + expect(transformed).to eq '❤❤❤❤❤❤' + ("\0" * 14) + expect(transformed.bytesize).to eq 32 + end + end + + context 'when string has multiple multi-byte UTF chars exceeding 32 bytes' do + it 'truncates string to 32 characters and backfills it if needed' do + transformed = described_class.ensure_utf8_size('❤' * 18, bytes: 32) + + expect(transformed).to eq ('❤' * 10) + ("\0" * 2) + expect(transformed.bytesize).to eq 32 + end + end + end end