View Issue Details

IDProjectCategoryView StatusLast Update
0003717Far ManagerEditorpublic2019-08-08 21:42
ReporterpepakAssigned ToDrKnS 
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Platformx64OSWindowsOS Version7
Product Version3.0 
Target VersionFixed in Version 
Summary0003717: #5411 should be optional
DescriptionUpdate #5411 changed the way an edited file is saved to the disk: Instead of using in-place save, a new file is created and then renamed. Unfortunately, this prevents editing of files unless the user has a DELETE privilege on the original file. FAR resorts to elevation in this case, but that is not a good solution as it leaves FAR in a privileged state afterwards. The original approach didn't need either a DELETE privilege or elevation, it was sufficient if the user had a WRITE privilege.

Also, this change causes the original file data to remain on the disk. It is now next to impossible to change the content of a file in such a way the original data is completely destroyed.
Steps To Reproduce1) Create a directory where the user has READ, WRITE and EXECUTE but not DELETE.
2) Create a new text file there.
3) Edit the file in 5411.
4) Save the file.
5) The saving will fail and a new temp file with the saved contents appears.
TagsNo tags attached.
Build5448

Activities

pepak

2019-06-20 07:03

reporter   bugnote:0016525

Here is a patch which achieves just that. Copy to FAR source tree and run 'git apply patch.diff'.

patch.diff (3,871 bytes)
From 62a01366c9f00fc54639800a62f6a94042384762 Mon Sep 17 00:00:00 2001
From: pepak <pepak@pepak.net>
Date: Thu, 20 Jun 2019 06:01:05 +0200
Subject: [PATCH] Safe saving of editor files (build 5411) now optional

---
 far/config.cpp   | 1 +
 far/config.hpp   | 1 +
 far/dizlist.cpp  | 2 +-
 far/fileedit.cpp | 2 +-
 far/usermenu.cpp | 2 +-
 5 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/far/config.cpp b/far/config.cpp
index 15e41c313..27d6a4ad4 100644
--- a/far/config.cpp
+++ b/far/config.cpp
@@ -1776,6 +1776,7 @@ void Options::InitConfigsData()
 		{FSSF_PRIVATE,           NKeyEditor,                 L"FileSizeLimit"sv,                 EdOpt.FileSizeLimit, 0},
 		{FSSF_PRIVATE,           NKeyEditor,                 L"KeepEditorEOL"sv,                 EdOpt.KeepEOL, true},
 		{FSSF_PRIVATE,           NKeyEditor,                 L"NewFileUnixEOL"sv,                EdOpt.NewFileUnixEOL, false},
+		{FSSF_PRIVATE,           NKeyEditor,                 L"SafeSaving"sv,                    EdOpt.SafeSaving, true},
 		{FSSF_PRIVATE,           NKeyEditor,                 L"CreateBackups"sv,                 EdOpt.CreateBackups, false},
 		{FSSF_PRIVATE,           NKeyEditor,                 L"PersistentBlocks"sv,              EdOpt.PersistentBlocks, false},
 		{FSSF_PRIVATE,           NKeyEditor,                 L"ReadOnlyLock"sv,                  EdOpt.ReadOnlyLock, 0},
diff --git a/far/config.hpp b/far/config.hpp
index efff690e0..78be39c59 100644
--- a/far/config.hpp
+++ b/far/config.hpp
@@ -522,6 +522,7 @@ public:
 		BoolOption KeepEOL;
 		BoolOption AddUnicodeBOM;
 		BoolOption NewFileUnixEOL;
+		BoolOption SafeSaving;
 		BoolOption CreateBackups;
 	};
 
diff --git a/far/dizlist.cpp b/far/dizlist.cpp
index d0b4a2b4d..79a3fa4df 100644
--- a/far/dizlist.cpp
+++ b/far/dizlist.cpp
@@ -342,7 +342,7 @@ bool DizList::Flush(const string& Path,const string* DizName)
 		const auto IsFileExists = FileAttr != INVALID_FILE_ATTRIBUTES;
 		const auto IsHardLink = IsFileExists && GetNumberOfLinks(m_DizFileName) > 1;
 		const auto IsSymLink = IsFileExists && (FileAttr & FILE_ATTRIBUTE_REPARSE_POINT);
-		const auto SaveSafely = IsFileExists && !IsHardLink && !IsSymLink;
+		const auto SaveSafely = Global->Opt->EdOpt.SafeSaving && IsFileExists && !IsHardLink && !IsSymLink;
 		const auto OutFileName = SaveSafely? MakeTempInSameDir(m_DizFileName) : m_DizFileName;
 
 		{
diff --git a/far/fileedit.cpp b/far/fileedit.cpp
index c4d466243..09201ecf4 100644
--- a/far/fileedit.cpp
+++ b/far/fileedit.cpp
@@ -1924,7 +1924,7 @@ int FileEditor::SaveFile(const string& Name,int Ask, bool bSaveAs, error_state_e
 		const auto IsFileExists = m_FileAttributes != INVALID_FILE_ATTRIBUTES;
 		const auto IsHardLink = IsFileExists && GetNumberOfLinks(Name) > 1;
 		const auto IsSymLink = IsFileExists && (m_FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT);
-		const auto SaveSafely = IsFileExists && !IsHardLink && !IsSymLink && !IsStream;
+		const auto SaveSafely = Global->Opt->EdOpt.SafeSaving && IsFileExists && !IsHardLink && !IsSymLink && !IsStream;
 		const auto OutFileName = SaveSafely? MakeTempInSameDir(Name) : Name;
 
 		{
diff --git a/far/usermenu.cpp b/far/usermenu.cpp
index 2e8f28799..752b8b7c2 100644
--- a/far/usermenu.cpp
+++ b/far/usermenu.cpp
@@ -345,7 +345,7 @@ void UserMenu::SaveMenu(const string& MenuFileName) const
 		const auto IsFileExists = FileAttr != INVALID_FILE_ATTRIBUTES;
 		const auto IsHardLink = IsFileExists && GetNumberOfLinks(MenuFileName) > 1;
 		const auto IsSymLink = IsFileExists && (FileAttr & FILE_ATTRIBUTE_REPARSE_POINT);
-		const auto SaveSafely = IsFileExists && !IsHardLink && !IsSymLink;
+		const auto SaveSafely = Global->Opt->EdOpt.SafeSaving && IsFileExists && !IsHardLink && !IsSymLink;
 		const auto OutFileName = SaveSafely? MakeTempInSameDir(MenuFileName) : MenuFileName;
 
 		{
-- 
2.21.0.windows.1

patch.diff (3,871 bytes)

DrKnS

2019-06-20 21:18

administrator   bugnote:0016527

I'm not (yet) convinced that it's needed.

> this prevents editing of files unless the user has a DELETE privilege on the original file
Is that a common case?

> it leaves FAR in a privileged state afterwards
not really.

> It is now next to impossible to change the content of a file in such a way the original data is completely destroyed.
Was it ever guaranteed that WriteFile writes to the same physical sectors? Is it possible at all with modern hardware (e.g. SSD)?

pepak

2019-06-20 21:32

reporter   bugnote:0016528

Re: Is that a common case?
Does it even matter? As it is a recommended security practice to reduce privileges, FAR should support that scenario. I can understand that someone might consider the threat of a damaged file content to be more serious, and by all means allow for that by making the option for "safe saving" enabled by default, but other's might have different priorities.

Re: Was it ever guaranteed
It was never guaranteed, but it was likely, or at least possible. Now it is almost guaranteed that the original data will *not* get overwritten.

DrKnS

2019-08-08 21:42

administrator   bugnote:0016567

5448: Editor.SaveSafely

Issue History

Date Modified Username Field Change
2019-06-18 09:49 pepak New Issue
2019-06-20 07:03 pepak File Added: patch.diff
2019-06-20 07:03 pepak Note Added: 0016525
2019-06-20 21:18 DrKnS Note Added: 0016527
2019-06-20 21:32 pepak Note Added: 0016528
2019-08-08 21:42 DrKnS Assigned To => DrKnS
2019-08-08 21:42 DrKnS Status new => closed
2019-08-08 21:42 DrKnS Resolution open => fixed
2019-08-08 21:42 DrKnS Build => 5448
2019-08-08 21:42 DrKnS Note Added: 0016567