From 27b37ea8380627cb5c6e0f3bcbe1b03892e5e54a Mon Sep 17 00:00:00 2001 From: sophia Date: Tue, 18 Aug 2020 11:38:04 -0500 Subject: [PATCH 1/3] Scrub credentials as whole words, don't capture matching substrings --- lib/vagrant/util/credential_scrubber.rb | 2 +- test/unit/vagrant/util/credential_scrubber_test.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/vagrant/util/credential_scrubber.rb b/lib/vagrant/util/credential_scrubber.rb index 6acb3f37a..8446b975d 100644 --- a/lib/vagrant/util/credential_scrubber.rb +++ b/lib/vagrant/util/credential_scrubber.rb @@ -32,7 +32,7 @@ module Vagrant def self.desensitize(string) string = string.to_s.dup sensitive_strings.each do |remove| - string.gsub!(remove, REPLACEMENT_TEXT) + string.gsub!(/(\W|^)#{remove}(\W|$)/, " #{REPLACEMENT_TEXT} ") end string end diff --git a/test/unit/vagrant/util/credential_scrubber_test.rb b/test/unit/vagrant/util/credential_scrubber_test.rb index e206fd763..1bbcefbe9 100644 --- a/test/unit/vagrant/util/credential_scrubber_test.rb +++ b/test/unit/vagrant/util/credential_scrubber_test.rb @@ -94,5 +94,18 @@ describe Vagrant::Util::CredentialScrubber do end end end + + context "with sensitive words that are part of non-sensitive words" do + let(:to_scrub){ ["a"] } + + it "should not remove parts of words" do + result = subject.desensitize(string) + to_scrub.each do |registered_value| + expect(result).not_to match(/(\W|^)#{registered_value}(\W|$)/) + end + expect(result).to include("my-birthday") + expect(result).to include("my-cats-birthday") + end + end end end From bb5d0e9c288a4170a61384a9afbdb3f7abb6c653 Mon Sep 17 00:00:00 2001 From: sophia Date: Wed, 19 Aug 2020 17:43:22 -0500 Subject: [PATCH 2/3] Remove credentials scrubbing from caching synced folders Passwords are (and should) never stored be as part of folder data. I think there is also a case to be made about desensitizes information here can lead to leaking of credentials. For example if an exported folder is named "vagrant" and the users password is "vagrant", the synced_folder cache will show "****" in place of the folder name, indicating that it is also password. --- .../action/builtin/mixin_synced_folders.rb | 4 --- .../builtin/mixin_synced_folders_test.rb | 30 ------------------- 2 files changed, 34 deletions(-) diff --git a/lib/vagrant/action/builtin/mixin_synced_folders.rb b/lib/vagrant/action/builtin/mixin_synced_folders.rb index 1209f5d69..4fa8dc453 100644 --- a/lib/vagrant/action/builtin/mixin_synced_folders.rb +++ b/lib/vagrant/action/builtin/mixin_synced_folders.rb @@ -99,10 +99,6 @@ module Vagrant folder_data = JSON.dump(folders) - # Scrub any register credentials from the synced folders - # configuration data to prevent accidental leakage - folder_data = Util::CredentialScrubber.desensitize(folder_data) - machine.data_dir.join("synced_folders").open("w") do |f| f.write(folder_data) end diff --git a/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb b/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb index e07223753..55ebc5924 100644 --- a/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb +++ b/test/unit/vagrant/action/builtin/mixin_synced_folders_test.rb @@ -273,11 +273,6 @@ describe Vagrant::Action::Builtin::MixinSyncedFolders do subject.save_synced_folders(machine, folders, options) end - it "should call credential scrubber before writing file" do - expect(Vagrant::Util::CredentialScrubber).to receive(:desensitize).and_call_original - subject.save_synced_folders(machine, folders, options) - end - context "when folder data is defined" do let(:folders) { {"root" => { @@ -288,31 +283,6 @@ describe Vagrant::Action::Builtin::MixinSyncedFolders do expect(output_file).to receive(:write).with(JSON.dump(folders)) subject.save_synced_folders(machine, folders, options) end - - context "when folder data configuration includes sensitive data" do - let(:password) { "VAGRANT_TEST_PASSWORD" } - - before do - folders["root"][:folder_password] = password - Vagrant::Util::CredentialScrubber.sensitive(password) - end - - after { Vagrant::Util::CredentialScrubber.unsensitive(password) } - - it "should not include password when writing file" do - expect(output_file).to receive(:write) do |content| - expect(content).not_to include(password) - end - subject.save_synced_folders(machine, folders, options) - end - - it "should mask password content when writing file" do - expect(output_file).to receive(:write) do |content| - expect(content).to include(Vagrant::Util::CredentialScrubber::REPLACEMENT_TEXT) - end - subject.save_synced_folders(machine, folders, options) - end - end end end From e2f012ff58fabcb5d4abab38b02e62405cc5b684 Mon Sep 17 00:00:00 2001 From: sophia Date: Thu, 20 Aug 2020 18:03:01 -0500 Subject: [PATCH 3/3] Escape value being scrubbed --- lib/vagrant/util/credential_scrubber.rb | 2 +- test/unit/vagrant/util/credential_scrubber_test.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/vagrant/util/credential_scrubber.rb b/lib/vagrant/util/credential_scrubber.rb index 8446b975d..dfc9b818e 100644 --- a/lib/vagrant/util/credential_scrubber.rb +++ b/lib/vagrant/util/credential_scrubber.rb @@ -32,7 +32,7 @@ module Vagrant def self.desensitize(string) string = string.to_s.dup sensitive_strings.each do |remove| - string.gsub!(/(\W|^)#{remove}(\W|$)/, " #{REPLACEMENT_TEXT} ") + string.gsub!(/(\W|^)#{Regexp.escape(remove)}(\W|$)/, "\\1#{REPLACEMENT_TEXT}\\2") end string end diff --git a/test/unit/vagrant/util/credential_scrubber_test.rb b/test/unit/vagrant/util/credential_scrubber_test.rb index 1bbcefbe9..04ba5ff41 100644 --- a/test/unit/vagrant/util/credential_scrubber_test.rb +++ b/test/unit/vagrant/util/credential_scrubber_test.rb @@ -107,5 +107,17 @@ describe Vagrant::Util::CredentialScrubber do expect(result).to include("my-cats-birthday") end end + + context "with sensitive words that are part of non-sensitive words" do + let(:to_scrub){ ["avery@strange/string^indeed!"] } + let(:string){ "a line of text with avery@strange/string^indeed! my-birthday and my-cats-birthday embedded" } + + it "should work for strings with escape characters" do + result = subject.desensitize(string) + to_scrub.each do |registered_value| + expect(result).not_to include(registered_value) + end + end + end end end