Prevent overwriting irregular files (cp, save, export commands) #1515

sw-pschmied · 2018-11-13T13:19:25Z

Signed-off-by: Philipp Schmied pschmied@schutzwerk.com

- What I did

This fixes #1514.

- How I did it

The function command.ValidateOutputPath was already present in cli/command/image/save.go. I've moved this to cli/command/utils.go in order to use this function to fix the issue for all three commands.

After that, I've added code to this existing function to use a new helper method called ValidateOutputPathFileMode which also resides in cli/command/utils.go. This is being used to check for irregular files/devices and returns an error in case an invalid target is being passed. This is now being called at the start of the functions implementing the cp, export and save commands. For cp operations to a container, the fix only uses the helper method mentioned above, so it's possible to unify this check for all three operations. The helper method also determines whether the target path points to a device or any other invalid/irregular file to display an error message accordingly.

Also, I've added test cases for all commands.

- How to verify it

Clone the cli master branch and compile a static docker binary
Pull an arbitrary docker image: docker pull ubuntu:latest
Invoke docker save ubuntu:latest -o /dev/random
Check the output of stat /dev/random indicating /dev/random is now a regular file
Perform the steps above using my PR and see that this operation is prevented, yielding failed to save image: invalid output path: "/dev/random" must be a directory or a regular file: Got a device.

- Description for the changelog

cp, save, export: Prevent overwriting irregular files

- A picture of a cute animal (not mandatory but encouraged)

codecov-io · 2018-11-13T13:27:10Z

Codecov Report

Merging #1515 into master will decrease coverage by 1.04%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   56.13%   55.08%   -1.05%     
==========================================
  Files         306      289      -17     
  Lines       20931    19389    -1542     
==========================================
- Hits        11749    10681    -1068     
+ Misses       8334     8015     -319     
+ Partials      848      693     -155

thaJeztah

Thanks! I only had a quick glance over this; left some comments/suggestions

thaJeztah · 2018-11-13T14:01:38Z

cli/command/utils.go

+	dir := filepath.Dir(path)
+	if dir != "" && dir != "." {
+		if _, err := os.Stat(dir); os.IsNotExist(err) {
+			return errors.Errorf("unable to validate output path: directory %q does not exist", dir)


perhaps this should be "invalid output path" instead of "unable to validate output path" (because we were able to validate it, but it turned out to be an invalid path)

thaJeztah · 2018-11-13T14:07:30Z

cli/command/utils.go

+	// (if the path exists and doesn't point to a directory)
+	if fileInfo, err := os.Stat(path); !os.IsNotExist(err) {
+		if !fileInfo.Mode().IsDir() && !fileInfo.Mode().IsRegular() {
+			return errors.Errorf("unable to validate output path: %q must be a directory or a regular file", path)


Same here for the "unable to validate.."

Also thinking if we should mention that the target is a "special" file (not sure)

sw-pschmied · 2018-11-13T14:14:00Z

Hi and thanks for your feedback! Applying changes shortly.

sw-pschmied · 2018-11-13T15:57:55Z

I've pushed the changes, made some additional refactoring and updated my original post. The major changes to the post are:

Mention and describe the additional helper method ValidateOutputPathFileMode
The error message now contains information on the type of invalid target being passed (device / misc irregular file)

sw-pschmied · 2018-11-30T11:17:15Z

ping @thaJeztah - is there else to change?

thaJeztah

Sorry for the delay; I've been a bit busy 😅

Left some comments inline, but feedback welcome, as usual 👍 Thanks!

cli/command/container/cp.go

cli/command/container/cp_test.go

cli/command/container/export_test.go

cli/command/utils.go

thaJeztah · 2018-11-30T15:08:29Z

cli/command/utils.go

+// helper to `ValidateOutputPath`
+func ValidateOutputPathFileMode(fileMode os.FileMode) error {
+	if fileMode&os.ModeDevice != 0 {
+		return errors.New("Got a device")


Should probably be lowercase for an error;

Suggested change

return errors.New("Got a device")

return errors.New("got a device")

cli/command/utils.go

sw-pschmied · 2018-12-03T08:53:13Z

Hi @thaJeztah, I've just pushed my changes. Thanks for the feedback!

sw-pschmied · 2019-01-24T08:42:54Z

ping @thaJeztah :)

thaJeztah

LGTM! Sorry for the delay 😅

ping @vdemeester @silvin-lubecki PTAL

silvin-lubecki · 2019-02-06T16:09:09Z

cli/command/utils.go

+func ValidateOutputPathFileMode(fileMode os.FileMode) error {
+	if fileMode&os.ModeDevice != 0 {
+		return errors.New("got a device")
+	} else if fileMode&os.ModeIrregular != 0 {


very small nit: I think the else can be removed. The 2 if can even be transformed to a switch:

switch{ case fileMode&os.ModeDevice != 0: return errors.New("got a device") case fileMode&os.ModeIrregular != 0: return errors.New("got an irregular file") } return nil

silvin-lubecki · 2019-02-06T16:11:27Z

cli/command/utils.go

+		}
+
+		if err := ValidateOutputPathFileMode(fileInfo.Mode()); err != nil {
+			return errors.Wrap(err, fmt.Sprintf("invalid output path: %q must be a directory or a regular file", path))


small nit: use errors.Wrapf instead.

silvin-lubecki

2 small nits but otherwise LGTM, thank you @sw-pschmied !

Signed-off-by: Philipp Schmied <pschmied@schutzwerk.com>

sw-pschmied · 2019-02-07T08:20:37Z

Thanks, I just pushed my changes @silvin-lubecki

vdemeester

LGTM 🐯

silvin-lubecki · 2019-02-07T09:06:12Z

Thanks a lot @sw-pschmied for detecting this issue AND fixing it ! 👍👍

GordonTheTurtle added the status/0-triage label Nov 13, 2018

thaJeztah requested changes Nov 13, 2018

View reviewed changes

sw-pschmied force-pushed the 1514-prevent-replacing-irregular-files branch 2 times, most recently from ee5b322 to 416087a Compare November 13, 2018 15:44

thaJeztah requested changes Nov 30, 2018

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Nov 30, 2018

sw-pschmied force-pushed the 1514-prevent-replacing-irregular-files branch from 416087a to 7f8e0d1 Compare December 3, 2018 08:51

thaJeztah approved these changes Jan 25, 2019

View reviewed changes

thaJeztah added the impact/changelog label Jan 25, 2019

silvin-lubecki reviewed Feb 6, 2019

View reviewed changes

silvin-lubecki approved these changes Feb 6, 2019

View reviewed changes

Prevent overwriting irregular files (cp, save, export commands)

7632776

Signed-off-by: Philipp Schmied <pschmied@schutzwerk.com>

sw-pschmied force-pushed the 1514-prevent-replacing-irregular-files branch from 7f8e0d1 to 7632776 Compare February 7, 2019 08:19

vdemeester approved these changes Feb 7, 2019

View reviewed changes

silvin-lubecki merged commit b1d2709 into docker:master Feb 7, 2019

GordonTheTurtle added this to the 19.03.0 milestone Feb 7, 2019

sw-pschmied deleted the 1514-prevent-replacing-irregular-files branch February 7, 2019 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent overwriting irregular files (cp, save, export commands) #1515

Prevent overwriting irregular files (cp, save, export commands) #1515

sw-pschmied commented Nov 13, 2018 •

edited

Loading

codecov-io commented Nov 13, 2018 •

edited

Loading

thaJeztah left a comment

thaJeztah Nov 13, 2018

thaJeztah Nov 13, 2018

sw-pschmied commented Nov 13, 2018

sw-pschmied commented Nov 13, 2018

sw-pschmied commented Nov 30, 2018

thaJeztah left a comment

thaJeztah Nov 30, 2018

sw-pschmied commented Dec 3, 2018

sw-pschmied commented Jan 24, 2019

thaJeztah left a comment

silvin-lubecki Feb 6, 2019

silvin-lubecki Feb 6, 2019

silvin-lubecki left a comment

sw-pschmied commented Feb 7, 2019

vdemeester left a comment

silvin-lubecki commented Feb 7, 2019

	return errors.New("Got a device")
	return errors.New("got a device")

Prevent overwriting irregular files (cp, save, export commands) #1515

Prevent overwriting irregular files (cp, save, export commands) #1515

Conversation

sw-pschmied commented Nov 13, 2018 • edited Loading

codecov-io commented Nov 13, 2018 • edited Loading

Codecov Report

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Nov 13, 2018

Choose a reason for hiding this comment

thaJeztah Nov 13, 2018

Choose a reason for hiding this comment

sw-pschmied commented Nov 13, 2018

sw-pschmied commented Nov 13, 2018

sw-pschmied commented Nov 30, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Nov 30, 2018

Choose a reason for hiding this comment

sw-pschmied commented Dec 3, 2018

sw-pschmied commented Jan 24, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

silvin-lubecki Feb 6, 2019

Choose a reason for hiding this comment

silvin-lubecki Feb 6, 2019

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

sw-pschmied commented Feb 7, 2019

vdemeester left a comment

Choose a reason for hiding this comment

silvin-lubecki commented Feb 7, 2019

sw-pschmied commented Nov 13, 2018 •

edited

Loading

codecov-io commented Nov 13, 2018 •

edited

Loading