Merge pull request #770 from joseivanlopez/security_issues

Hardening commands execution
yast · Dec 17, 2018 · 640ea5b · 640ea5b
2 parents ab01b41 + 3b715c7
commit 640ea5b
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 8 deletions.
diff --git a/package/yast2-installation.changes b/package/yast2-installation.changes
@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Mon Dec 17 09:03:47 UTC 2018 - jlopez@suse.com
+
+- Hardening commands execution (part of bsc#1118291).
+- Replace backticks by Yast::Execute.
+- 4.1.34
+
 -------------------------------------------------------------------
 Wed Dec 12 15:26:26 UTC 2018 - Josef Reidinger <jreidinger@suse.com>
 

diff --git a/package/yast2-installation.spec b/package/yast2-installation.spec
@@ -16,7 +16,7 @@
 #
 
 Name:           yast2-installation
-Version:        4.1.33
+Version:        4.1.34
 Release:        0
 
 BuildRoot:      %{_tmppath}/%{name}-%{version}-build
@@ -42,9 +42,9 @@ BuildRequires:  yast2-xml
 BuildRequires:  rubygem(rspec)
 BuildRequires:  rubygem(yast-rake)
 
-# Yast2::Systemd::Service
-BuildRequires:  yast2 >= 4.1.3
-Requires:       yast2 >= 4.1.3
+# Yast::Execute.stdout
+BuildRequires:  yast2 >= 4.1.42
+Requires:       yast2 >= 4.1.42
 
 # Y2Packager::SelfUpdateAddonRepo
 BuildRequires:	yast2-packager >= 4.1.5

diff --git a/src/lib/transfer/file_from_url.rb b/src/lib/transfer/file_from_url.rb
@@ -20,7 +20,7 @@
 # ------------------------------------------------------------------------------
 
 require "yast"
-require "shellwords"
+require "yast2/execute"
 
 # rubocop:disable all
 module Yast::Transfer
@@ -468,7 +468,8 @@ def get_file_from_url(scheme:, host:, urlpath:, localfile:,
             # checking if device has already been mounted. Taking new mountpoint
             # if it has already been done.
             # storage-ng: This should be move to storage-ng
-            mp = `/usr/bin/findmnt --first-only --noheadings --output=target /dev/#{Shellwords.escape(_Host2)}`.split.last
+            findmnt = ["/usr/bin/findmnt", "--first-only", "--noheadings", "--output=target", "/dev/#{_Host2}"]
+            mp = Yast::Execute.locally!.stdout(*findmnt).split.last
             already_mounted = !mp.nil?
             mount_point = mp if already_mounted
             Builtins.y2milestone(

diff --git a/test/file_from_url_test.rb b/test/file_from_url_test.rb
@@ -43,8 +43,12 @@ def Get(scheme, host, urlpath, localfile)
       # the local/target mess was last modified in
       # https://github.com/yast/yast-autoinstallation/commit/69f1966dd1456301a69102c6d3bacfe7c9f9dc49
       # for https://bugzilla.suse.com/show_bug.cgi?id=829265
+
+      allow(Yast::Execute).to receive(:new).and_return(execute_object)
     end
 
+    let(:execute_object) { Yast::Execute.new }
+
     it "returns false for unknown scheme" do
       expect(subject.Get("money_transfer_protocol",
         "bank", "account", "pocket")).to eq(false)
@@ -105,8 +109,8 @@ def Get(scheme, host, urlpath, localfile)
           }
 
           mount_points.each do |device, mp|
-            expect(subject).to receive(:`)
-              .with("/usr/bin/findmnt --first-only --noheadings --output=target #{device}")
+            expect(execute_object).to receive(:stdout)
+              .with("/usr/bin/findmnt", "--first-only", "--noheadings", "--output=target", device)
               .and_return(mp.first)
           end