Secure File Removal Improvements by jentfoo · Pull Request #32260 · gravitational/teleport

jentfoo · 2023-09-20T20:08:18Z

This PR improves the reliability of secure file removal (particularly under conditions of unexpected errors), and leverages this functionality when removing certificates on disk. This will have little value for SSD's but can be an effective measure when magnetic hard drives are used.

This PR fixes https://github.com/gravitational/teleport-private/issues/670

zmb3

Haven't tried to run this yet, just did a pass of the code.

zmb3

Looks better, one last thought.

espadolini

Can we unlink the file first, and then overwrite its contents? As it is, anything opening the file while it still exists on disk but is in the process of being overwritten will just get some random garbage.

jentfoo · 2023-09-21T14:51:13Z

Can we unlink the file first, and then overwrite its contents? As it is, anything opening the file while it still exists on disk but is in the process of being overwritten will just get some random garbage.

We can, but this would be an OS conditional logic. I don't think in Windows you can write to files which have been removed from disk. It would be only Linux and MacOS which would allow us to open the handle, remove, and then overwrite.

Your recommendation is superior when it can be supported, but since we would have to support both methods is it worth doubling the code?

AntonAM · 2023-09-21T15:12:36Z

@jentfoo I don't think we have to double the code, there are already fs_unix.go and fs_windows.go, which can be used to provide platform-specific implementations. For example, you could add something like unlinkFile function in those, that we call before overwriting. And on mac/linux it would unlink the file, but on windows it would be no-op. Or something like that.

jentfoo · 2023-09-21T15:18:01Z

@AntonAM Perhaps you see something I don't. I don't believe you can simply no-op that single action unless you have other actions which are no-op in the unix case. There is a need for action reordering with regards to the unlink / remove.

Unix:

Open
Unlink
Overwrite

Windows:

Overwrite
Remove

If we try to combine these we have a total steps of:

Open
Unlink (optional)
Overwrite
Remove (if not already unlinked)

While we could build the combined operation above, and that would technically be less lines. But I feel like it's more confusing than a single conditional which chooses the flow that makes sense for the OS.

espadolini · 2023-09-21T15:30:02Z

Maybe on windows we can rename the file before overwriting it? Especially around key management, I'm wholly convinced that we have code that expects that if a file exists then its contents must be well-formed.

jentfoo · 2023-09-21T15:39:13Z

I would be concerned about renaming, that would introduce a new place where the chain of operations may fail and result in a sensitive file being retained on disk. Instead I would lean towards identify and improve any logic that could be impacted by this small overwrite window.

codingllama

Thanks for yet another security improvement, Jent!

codingllama · 2023-09-21T17:08:17Z

+		return &os.PathError{Op: "RemoveAllSecure", Path: path, Err: syscall.EINVAL} // error type matches os.RemoveAll
+	}
+
+	if info, err := os.Lstat(path); err != nil {


FYI in case you want to try it, but there may be an elegant traversal solution using filepath.WalkDir. No need to change anything though.

https://pkg.go.dev/path/filepath#WalkDir

AntonAM · 2023-09-21T17:37:33Z

@jentfoo no, just wanted to highlight this option, I don't insist on the change :)

jentfoo · 2023-09-21T20:24:30Z

@AntonAM and @espadolini I updated it with the OS conditional logic, it ended up being cleaner than I had imagined

codingllama · 2023-09-21T21:14:56Z

Suggestion: drop separate branches. Builds on the previous suggestions by assuming f can't be nil in this block.

Suggested change

if runtime.GOOS == "windows" {

// windows can't unlink the file before overwriting

if f != nil {

for i := 0; i < 3; i++ {

_ = overwriteFile(f)

}

}

}()

return trace.ConvertSystemError(os.Remove(filePath))

} else {

removeErr := os.Remove(filePath)

if f != nil {

for i := 0; i < 3; i++ {

_ = overwriteFile(f)

}

}

return trace.ConvertSystemError(removeErr)

}

}

isWindows := runtime.GOOS == "windows"

var removeErr error

if !isWindows {

// Unlink first on unix systems.

removeErr = os.Remove(filePath)

}

for i := 0; i < 3; i++ {

_ = overwriteFile(f)

}

if isWindows {

removeErr = os.Remove(filePath)

}

return trace.ConvertSystemError(removeErr)

}

Mixing the conditional multiple times reduces readability to me, but I don't feel strongly about it

Both look fine to me.

codingllama · 2023-09-21T21:20:25Z

Looks good, mostly stylistic comments left.

zmb3

Approved, but I do think we need to check the sync error (see comment)

zmb3 · 2023-09-22T15:09:54Z

Both look fine to me.

zmb3 · 2023-09-22T15:10:52Z

Should we be ignoring an error here?

Writes are buffered in many cases, meaning the error that we are checking (from CopyN) can be nil and the real error would surface here, but we're ignoring it.

I updated is to that any error (from the copy or sync) will break the overwrite loop, but otherwise the errors are still ignored.

As extra caution, even if an error occurs during the overwrite process, we still want to attempt a removal of sensitive files.

This commit ensures that deleted keyfiles have been overwritten. This has little value on SSD's but can improve the security when the disk is magnetic.

…ible This adds an OS conditional so that if possible the file will be removed and then overwritten using the previous file handle. This will reduces the chance that the file will be witnessed with unexpected contents.

public-teleport-github-review-bot · 2023-09-22T22:12:03Z

@jentfoo See the table below for backport results.

Branch	Result
branch/v12	Failed
branch/v13	Failed
branch/v14	Create PR

* utils.RemoveSecure: Still attempt a removal after error in overwrite As extra caution, even if an error occurs during the overwrite process, we still want to attempt a removal of sensitive files. * keystore.go: More secure removal of keyfiles This commit ensures that deleted keyfiles have been overwritten. This has little value on SSD's but can improve the security when the disk is magnetic. * Apply PR feedback, notably better testing and early unlinking if possible This adds an OS conditional so that if possible the file will be removed and then overwritten using the previous file handle. This will reduces the chance that the file will be witnessed with unexpected contents.

jentfoo added backport/branch/v12 labels Sep 20, 2023

jentfoo requested review from AntonAM, adaadb6, codingllama, lxea, r0mant, rosstimothy, wadells, xacrimon and zmb3 September 20, 2023 20:08

github-actions Bot requested a review from jimbishopp September 20, 2023 20:08

github-actions Bot added the size/sm label Sep 20, 2023

zmb3 reviewed Sep 20, 2023

View reviewed changes

Comment thread lib/utils/fs.go Outdated

Comment thread lib/utils/fs.go Outdated

Comment thread lib/utils/fs.go Outdated

Comment thread lib/utils/fs.go Outdated

Comment thread lib/utils/fs.go Outdated

zmb3 approved these changes Sep 20, 2023

View reviewed changes

Comment thread lib/utils/fs.go Outdated

rosstimothy reviewed Sep 20, 2023

View reviewed changes

Comment thread lib/utils/fs.go Outdated

rosstimothy reviewed Sep 20, 2023

View reviewed changes

Comment thread lib/utils/fs.go Outdated

Comment thread lib/utils/fs.go Outdated

wadells approved these changes Sep 20, 2023

View reviewed changes

xacrimon approved these changes Sep 21, 2023

View reviewed changes

espadolini reviewed Sep 21, 2023

View reviewed changes

Comment thread lib/utils/fs.go Outdated

codingllama reviewed Sep 21, 2023

View reviewed changes

codingllama approved these changes Sep 22, 2023

View reviewed changes

Comment thread lib/utils/fs.go Outdated

public-teleport-github-review-bot Bot removed request for AntonAM, adaadb6, jimbishopp, lxea and r0mant September 22, 2023 14:35

zmb3 approved these changes Sep 22, 2023

View reviewed changes

jentfoo added 3 commits September 22, 2023 15:28

utils.RemoveSecure: Still attempt a removal after error in overwrite

1b93476

As extra caution, even if an error occurs during the overwrite process, we still want to attempt a removal of sensitive files.

keystore.go: More secure removal of keyfiles

605ad4f

This commit ensures that deleted keyfiles have been overwritten. This has little value on SSD's but can improve the security when the disk is magnetic.

jentfoo force-pushed the jent/secure_removal_improvements branch from 70cee2c to d1b5d21 Compare September 22, 2023 21:30

jentfoo added this pull request to the merge queue Sep 22, 2023

Merged via the queue into master with commit ce4ea1c Sep 22, 2023

jentfoo deleted the jent/secure_removal_improvements branch September 22, 2023 22:09

This was referenced Sep 22, 2023

[v12] Secure File Removal Improvements #32433

Merged

[v13] Secure File Removal Improvements #32434

Closed

[v14] Secure File Removal Improvements #32435

Merged

[v13] Secure File Removal Improvements #32437

Merged

gzdunek mentioned this pull request Oct 4, 2023

removeSecure() should close the file before removing it on Windows #32948

Merged

jentfoo mentioned this pull request Oct 24, 2023

Prevent SSO Redirects to other origins #33853

Merged

-	if runtime.GOOS == "windows" {
-		// windows can't unlink the file before overwriting
-		if f != nil {
-			for i := 0; i < 3; i++ {
-				_ = overwriteFile(f)
-			}
-		}
-	}()
-		return trace.ConvertSystemError(os.Remove(filePath))
-	} else {
-		removeErr := os.Remove(filePath)
-		if f != nil {
-			for i := 0; i < 3; i++ {
-				_ = overwriteFile(f)
-			}
-		}
-		return trace.ConvertSystemError(removeErr)
-	}
-}
+	isWindows := runtime.GOOS == "windows"
+	var removeErr error
+	if !isWindows {
+		// Unlink first on unix systems.
+		removeErr = os.Remove(filePath)
+	}
+	for i := 0; i < 3; i++ {
+		_ = overwriteFile(f)
+	}
+	if isWindows {
+		removeErr = os.Remove(filePath)
+	}
+	return trace.ConvertSystemError(removeErr)
+}

Conversation

jentfoo commented Sep 20, 2023

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

espadolini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jentfoo commented Sep 21, 2023

Uh oh!

AntonAM commented Sep 21, 2023

Uh oh!

jentfoo commented Sep 21, 2023

Uh oh!

espadolini commented Sep 21, 2023

Uh oh!

jentfoo commented Sep 21, 2023

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codingllama Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

AntonAM commented Sep 21, 2023

Uh oh!

jentfoo commented Sep 21, 2023

Uh oh!

Uh oh!

codingllama Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

jentfoo Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

zmb3 Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codingllama commented Sep 21, 2023

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmb3 Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

zmb3 Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

jentfoo Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!