From d589aa9f81003ddeb07ff2e82231caa37173265e Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 26 Nov 2018 15:48:46 -0800 Subject: [PATCH] Update default_nic_type implementation within VirtualBox provider In some cases the E1000 NIC type is the only acceptable value. Since defaulting causes breakages to existing boxes, leave the default value as `nil` but check the VirtualBox version in use and print warning to user if VirtualBox version is vulnerable and E1000 NIC types are configured for use within defined network adapters. --- plugins/providers/virtualbox/action.rb | 2 + .../providers/virtualbox/action/network.rb | 7 - .../virtualbox/action/set_default_nic_type.rb | 69 ++++++++ plugins/providers/virtualbox/config.rb | 16 +- templates/locales/en.yml | 11 ++ .../virtualbox/action/network_test.rb | 38 +---- .../action/set_default_nic_type_test.rb | 148 ++++++++++++++++++ .../providers/virtualbox/config_test.rb | 20 +-- .../docs/virtualbox/configuration.html.md | 16 +- 9 files changed, 240 insertions(+), 87 deletions(-) create mode 100644 plugins/providers/virtualbox/action/set_default_nic_type.rb create mode 100644 test/unit/plugins/providers/virtualbox/action/set_default_nic_type_test.rb diff --git a/plugins/providers/virtualbox/action.rb b/plugins/providers/virtualbox/action.rb index 39c4ac61b..113be2b03 100644 --- a/plugins/providers/virtualbox/action.rb +++ b/plugins/providers/virtualbox/action.rb @@ -42,6 +42,7 @@ module VagrantPlugins autoload :PrepareForwardedPortCollisionParams, File.expand_path("../action/prepare_forwarded_port_collision_params", __FILE__) autoload :Resume, File.expand_path("../action/resume", __FILE__) autoload :SaneDefaults, File.expand_path("../action/sane_defaults", __FILE__) + autoload :SetDefaultNICType, File.expand_path("../action/set_default_nic_type", __FILE__) autoload :SetName, File.expand_path("../action/set_name", __FILE__) autoload :SnapshotDelete, File.expand_path("../action/snapshot_delete", __FILE__) autoload :SnapshotRestore, File.expand_path("../action/snapshot_restore", __FILE__) @@ -71,6 +72,7 @@ module VagrantPlugins b.use SyncedFolderCleanup b.use SyncedFolders b.use PrepareNFSSettings + b.use SetDefaultNICType b.use ClearNetworkInterfaces b.use Network b.use NetworkFixIPv6 diff --git a/plugins/providers/virtualbox/action/network.rb b/plugins/providers/virtualbox/action/network.rb index 8f8e163c1..07f1ae665 100644 --- a/plugins/providers/virtualbox/action/network.rb +++ b/plugins/providers/virtualbox/action/network.rb @@ -37,8 +37,6 @@ module VagrantPlugins available_slots.delete(slot) end - default_nic_type = env[:machine].provider_config.default_nic_type - @logger.debug("Available slots for high-level adapters: #{available_slots.inspect}") @logger.info("Determining network adapters required for high-level configuration...") available_slots = available_slots.to_a.sort @@ -46,11 +44,6 @@ module VagrantPlugins # We only handle private and public networks next if type != :private_network && type != :public_network - if default_nic_type && !options.key?(:nic_type) && !options.key?(:virtualbox__nic_type) - @logger.info("Setting default nic type (`#{default_nic_type}`) for `#{type}` - `#{options}`") - options[:virtualbox__nic_type] = default_nic_type - end - options = scoped_hash_override(options, :virtualbox) # Figure out the slot that this adapter will go into diff --git a/plugins/providers/virtualbox/action/set_default_nic_type.rb b/plugins/providers/virtualbox/action/set_default_nic_type.rb new file mode 100644 index 000000000..e3b337445 --- /dev/null +++ b/plugins/providers/virtualbox/action/set_default_nic_type.rb @@ -0,0 +1,69 @@ +require "log4r" + +module VagrantPlugins + module ProviderVirtualBox + module Action + # This sets the default NIC type used for network adapters created + # on the guest. Also includes a check of NIC types in use and VirtualBox + # version to determine if E1000 NIC types are vulnerable. + # + # NOTE: Vulnerability was fixed here: https://www.virtualbox.org/changeset/75330/vbox + class SetDefaultNICType + # Defines versions of VirtualBox with susceptible implementation + # of the E1000 devices. + E1000_SUSCEPTIBLE = Gem::Requirement.new("<= 5.2.22").freeze + + def initialize(app, env) + @logger = Log4r::Logger.new("vagrant::plugins::virtualbox::set_default_nic_type") + @app = app + end + + def call(env) + default_nic_type = env[:machine].provider_config.default_nic_type + + e1000_in_use = [ + # simple check on default_nic_type + ->{ default_nic_type.nil? || default_nic_type.to_s.start_with?("8254") }, + # check provider defined adapters + ->{ env[:machine].provider_config.network_adapters.values.detect{ |_, opts| + opts[:nic_type].to_s.start_with?("8254") } }, + # finish with inspecting configured networks + ->{ env[:machine].config.vm.networks.detect{ |_, opts| + opts.fetch(:virtualbox__nic_type, opts[:nic_type]).to_s.start_with?("8254") } } + ] + + # Check if VirtualBox E1000 implementation is vulnerable + if E1000_SUSCEPTIBLE.satisfied_by?(Gem::Version.new(env[:machine].provider.driver.version)) + @logger.info("Detected VirtualBox version with susceptible E1000 implementation (`#{E1000_SUSCEPTIBLE}`)") + if e1000_in_use.any?(&:call) + env[:ui].warn I18n.t("vagrant.actions.vm.set_default_nic_type.e1000_warning") + end + end + + if default_nic_type + @logger.info("Default NIC type for VirtualBox interfaces `#{default_nic_type}`") + # Update network adapters defined in provider configuration + env[:machine].provider_config.network_adapters.each do |slot, args| + _, opts = args + if opts && !opts.key?(:nic_type) + @logger.info("Setting default NIC type (`#{default_nic_type}`) adapter `#{slot}` - `#{args}`") + opts[:nic_type] = default_nic_type + end + end + + # Update generally defined networks + env[:machine].config.vm.networks.each do |type, options| + next if !type.to_s.end_with?("_network") + if !options.key?(:nic_type) && !options.key?(:virtualbox__nic_type) + @logger.info("Setting default NIC type (`#{default_nic_type}`) for `#{type}` - `#{options}`") + options[:virtualbox__nic_type] = default_nic_type + end + end + end + + @app.call(env) + end + end + end + end +end diff --git a/plugins/providers/virtualbox/config.rb b/plugins/providers/virtualbox/config.rb index f64b208d6..e785880ce 100644 --- a/plugins/providers/virtualbox/config.rb +++ b/plugins/providers/virtualbox/config.rb @@ -20,8 +20,8 @@ module VagrantPlugins attr_reader :customizations # Set the default type of NIC hardware to be used for network - # devices. By default this is "virtio". If it is set to `nil` - # no type will be set and VirtualBox's default will be used. + # devices. By default this is `nil` and VirtualBox's default + # will be used. # # @return [String] attr_accessor :default_nic_type @@ -167,8 +167,7 @@ module VagrantPlugins # The default name is just nothing, and we default it @name = nil if @name == UNSET_VALUE - @default_nic_type = "virtio" if @default_nic_type == UNSET_VALUE - set_default_nic_type! if @default_nic_type + @default_nic_type = nil if @default_nic_type == UNSET_VALUE end def validate(machine) @@ -201,15 +200,6 @@ module VagrantPlugins { "VirtualBox Provider" => errors } end - def set_default_nic_type! - network_adapters.each do |_, args| - _, opts = args - if opts && !opts.key?(:nic_type) - opts[:nic_type] = @default_nic_type - end - end - end - def to_s "VirtualBox" end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 1a6498ce9..d9408f585 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -2300,6 +2300,17 @@ en: mounting: Mounting shared folders... mounting_entry: "%{guestpath} => %{hostpath}" nomount_entry: "Automounting disabled: %{hostpath}" + set_default_nic_type: + e1000_warning: |- + Vagrant has detected a configuration issue which exposes a + vulnerability with the installed version of VirtualBox. The + current guest is configured to use an E1000 NIC type for a + network adapter which is vulnerable in this version of VirtualBox. + Ensure the guest is trusted to use this configuration or update + the NIC type using one of the methods below: + + https://www.vagrantup.com/docs/virtualbox/configuration.html#default-nic-type + https://www.vagrantup.com/docs/virtualbox/networking.html#virtualbox-nic-type set_name: setting_name: |- Setting the name of the VM: %{name} diff --git a/test/unit/plugins/providers/virtualbox/action/network_test.rb b/test/unit/plugins/providers/virtualbox/action/network_test.rb index 1e6bc7c88..34ed17d94 100644 --- a/test/unit/plugins/providers/virtualbox/action/network_test.rb +++ b/test/unit/plugins/providers/virtualbox/action/network_test.rb @@ -84,42 +84,6 @@ describe VagrantPlugins::ProviderVirtualBox::Action::Network do expect{ subject.call(env) }.not_to raise_error(Vagrant::Errors::NetworkAddressInvalid) end - describe "setting nic type" do - before do - guest = double("guest") - allow(driver).to receive(:read_bridged_interfaces) { [] } - allow(driver).to receive(:read_host_only_interfaces) { [] } - allow(driver).to receive(:create_host_only_network) { {} } - allow(driver).to receive(:read_dhcp_servers) { [] } - allow(driver).to receive(:create_dhcp_server) - allow(machine).to receive(:guest) { guest } - allow(guest).to receive(:capability) - end - - it "sets default nic type when unset" do - machine.config.vm.network 'private_network', { type: 'dhcp' } - subject.call(env) - _, net_config = machine.config.vm.networks.detect { |type, _| type == :private_network } - expect(net_config[:virtualbox__nic_type]).to eq("virtio") - end - - it "does not set nic type when already set" do - machine.config.vm.network 'private_network', { type: 'dhcp', nic_type: "custom" } - subject.call(env) - _, net_config = machine.config.vm.networks.detect { |type, _| type == :private_network } - expect(net_config[:nic_type]).to eq("custom") - expect(net_config[:virtualbox__nic_type]).to be_nil - end - - it "does not set nic type when namespaced option is set" do - machine.config.vm.network 'private_network', { type: 'dhcp', virtualbox__nic_type: "custom" } - subject.call(env) - _, net_config = machine.config.vm.networks.detect { |type, _| type == :private_network } - expect(net_config[:nic_type]).to be_nil - expect(net_config[:virtualbox__nic_type]).to eq("custom") - end - end - context "with a dhcp private network" do let(:bridgedifs) { [] } let(:hostonlyifs) { [] } @@ -154,7 +118,7 @@ describe VagrantPlugins::ProviderVirtualBox::Action::Network do mac: nil, name: nil, netmask: "255.255.255.0", - nic_type: "virtio", + nic_type: nil, type: :dhcp, dhcp_ip: "172.28.128.2", dhcp_lower: "172.28.128.3", diff --git a/test/unit/plugins/providers/virtualbox/action/set_default_nic_type_test.rb b/test/unit/plugins/providers/virtualbox/action/set_default_nic_type_test.rb new file mode 100644 index 000000000..f621d152c --- /dev/null +++ b/test/unit/plugins/providers/virtualbox/action/set_default_nic_type_test.rb @@ -0,0 +1,148 @@ +require_relative "../base" + +describe VagrantPlugins::ProviderVirtualBox::Action::SetDefaultNICType do + include_context "unit" + include_context "virtualbox" + + let(:iso_env) do + # We have to create a Vagrantfile so there is a root path + env = isolated_environment + env.vagrantfile("") + env.create_vagrant_env + end + + let(:machine) do + iso_env.machine(iso_env.machine_names[0], :virtualbox).tap do |m| + allow(m.provider).to receive(:driver).and_return(driver) + end + end + + let(:env) {{ machine: machine, ui: machine.ui }} + let(:app) { lambda { |*args| }} + let(:driver) { double("driver") } + + subject { described_class.new(app, env) } + + describe "#call" do + let(:provider_config) { + double("provider_config", + default_nic_type: default_nic_type, + network_adapters: network_adapters) + } + let(:default_nic_type) { nil } + let(:network_adapters) { {} } + let(:virtualbox_version) { "5.2.23" } + + before do + allow(driver).to receive(:version).and_return(virtualbox_version) + allow(machine).to receive(:provider_config).and_return(provider_config) + end + + it "should call the next action" do + expect(app).to receive(:call) + subject.call(env) + end + + context "when default_nic_type is set" do + let(:default_nic_type) { "CUSTOM_NIC_TYPE" } + + context "when network adapters are defined" do + let(:network_adapters) { {"1" => [:nat, {}], "2" => [:intnet, {nic_type: nil}]} } + + it "should set nic type if not defined" do + subject.call(env) + expect(network_adapters["1"].last[:nic_type]).to eq(default_nic_type) + end + + it "should not set nic type if already defined" do + subject.call(env) + expect(network_adapters["2"].last[:nic_type]).to be_nil + end + end + + context "when vm networks are defined" do + before do + machine.config.vm.network :private_network + machine.config.vm.network :public_network, nic_type: nil + machine.config.vm.network :private_network, virtualbox__nic_type: "STANDARD" + end + + it "should add namespaced nic type when not defined" do + subject.call(env) + networks = machine.config.vm.networks.map { |type, opts| + opts if type.to_s.end_with?("_network") }.compact + expect(networks.first[:virtualbox__nic_type]).to eq(default_nic_type) + end + + it "should not add namespaced nic type when nic type defined" do + subject.call(env) + networks = machine.config.vm.networks.map { |type, opts| + opts if type.to_s.end_with?("_network") }.compact + expect(networks[1][:virtualbox__nic_type]).to be_nil + end + + it "should not modify existing namespaced nic type" do + subject.call(env) + networks = machine.config.vm.networks.map { |type, opts| + opts if type.to_s.end_with?("_network") }.compact + expect(networks.last[:virtualbox__nic_type]).to eq("STANDARD") + end + end + end + + context "when virtualbox version is has susceptible E1000" do + let(:virtualbox_version) { "5.2.22" } + + it "should output a warning" do + expect(machine.ui).to receive(:warn) + subject.call(env) + end + + context "when default_nic_type is set to E1000 type" do + let(:default_nic_type) { "82540EM" } + + it "should output a warning" do + expect(machine.ui).to receive(:warn) + subject.call(env) + end + end + + context "when default_nic_type is set to non-E1000 type" do + let(:default_nic_type) { "virtio" } + + it "should not output a warning" do + expect(machine.ui).not_to receive(:warn) + subject.call(env) + end + + context "when network adapter is configured with E1000 type" do + let(:network_adapters) { {"1" => [:nat, {nic_type: "82540EM" }]} } + + it "should output a warning" do + expect(machine.ui).to receive(:warn) + subject.call(env) + end + end + + context "when vm network is configured with E1000 type" do + before { machine.config.vm.network :private_network, nic_type: "82540EM" } + + it "should output a warning" do + expect(machine.ui).to receive(:warn) + subject.call(env) + end + end + + context "when vm network is configured with E1000 type in namespaced argument" do + before { machine.config.vm.network :private_network, virtualbox__nic_type: "82540EM" } + + it "should output a warning" do + expect(machine.ui).to receive(:warn) + subject.call(env) + end + end + end + end + + end +end diff --git a/test/unit/plugins/providers/virtualbox/config_test.rb b/test/unit/plugins/providers/virtualbox/config_test.rb index 1355c4e9b..a2ca1c4b8 100644 --- a/test/unit/plugins/providers/virtualbox/config_test.rb +++ b/test/unit/plugins/providers/virtualbox/config_test.rb @@ -43,11 +43,11 @@ describe VagrantPlugins::ProviderVirtualBox::Config do it { expect(subject.gui).to be(false) } it { expect(subject.name).to be_nil } it { expect(subject.functional_vboxsf).to be(true) } - it { expect(subject.default_nic_type).to eq("virtio") } + it { expect(subject.default_nic_type).to be_nil } it "should have one NAT adapter" do expect(subject.network_adapters).to eql({ - 1 => [:nat, {nic_type: "virtio"}], + 1 => [:nat, {}], }) end end @@ -61,22 +61,6 @@ describe VagrantPlugins::ProviderVirtualBox::Config do end it { expect(subject.default_nic_type).to eq(nic_type) } - - it "should set NAT adapter nic type" do - expect(subject.network_adapters.values.first.last[:nic_type]). - to eq(nic_type) - end - - context "when set to nil" do - let(:nic_type) { nil } - - it { expect(subject.default_nic_type).to be_nil } - - it "should not set NAT adapter nic type" do - expect(subject.network_adapters.values.first.last[:nic_type]). - to be_nil - end - end end describe "#merge" do diff --git a/website/source/docs/virtualbox/configuration.html.md b/website/source/docs/virtualbox/configuration.html.md index a4aaf9a3b..1c9900652 100644 --- a/website/source/docs/virtualbox/configuration.html.md +++ b/website/source/docs/virtualbox/configuration.html.md @@ -43,9 +43,10 @@ end ## Default NIC Type -By default Vagrant will set the NIC type for all network interfaces to -`"virtio"`. If you would rather a different NIC type be used as the -default for guests: +By default Vagrant will not set the NIC type for network interfaces. This +allows VirtualBox to apply the default NIC type for the guest. If you would +like to use a specific NIC type by default for guests, set the `default_nic_type` +option: ```ruby config.vm.provider "virtualbox" do |v| @@ -53,15 +54,6 @@ config.vm.provider "virtualbox" do |v| end ``` -or if you would like to disable the default type so VirtualBox's default -is applied you can set the value to `nil`: - -```ruby -config.vm.provider "virtualbox" do |v| - v.default_nic_type = nil -end -``` - ## Linked Clones By default new machines are created by importing the base box. For large