Skip to content

Commit

Permalink
safer os.chmod for wallet files and config: set perms before write
Browse files Browse the repository at this point in the history
Set unix file permissions first, before writing data.
  • Loading branch information
SomberNight committed Apr 8, 2024
1 parent 6d37e46 commit f495511
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 7 deletions.
2 changes: 1 addition & 1 deletion electrum/gui/qt/main_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -2332,7 +2332,7 @@ def on_dialog_closed(*args):

def do_export_privkeys(self, fileName, pklist, is_csv):
with open(fileName, "w+") as f:
os_chmod(fileName, 0o600)
os_chmod(fileName, 0o600) # set restrictive perms *before* we write data
if is_csv:
transaction = csv.writer(f)
transaction.writerow(["address", "private_key"])
Expand Down
2 changes: 1 addition & 1 deletion electrum/simple_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -421,8 +421,8 @@ def save_user_config(self):
s = json.dumps(self.user_config, indent=4, sort_keys=True)
try:
with open(path, "w", encoding='utf-8') as f:
os_chmod(path, stat.S_IREAD | stat.S_IWRITE) # set restrictive perms *before* we write data
f.write(s)
os_chmod(path, stat.S_IREAD | stat.S_IWRITE)
except OSError:
# datadir probably deleted while running... e.g. portable exe running on ejected USB drive
# (in which case it is typically either FileNotFoundError or PermissionError,
Expand Down
10 changes: 5 additions & 5 deletions electrum/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,22 @@ def read(self):
return self.decrypted if self.is_encrypted() else self.raw

def write(self, data: str) -> None:
try:
mode = os.stat(self.path).st_mode
except FileNotFoundError:
mode = stat.S_IREAD | stat.S_IWRITE
s = self.encrypt_before_writing(data)
temp_path = "%s.tmp.%s" % (self.path, os.getpid())
with open(temp_path, "wb") as f:
os_chmod(temp_path, mode) # set restrictive perms *before* we write data
f.write(s.encode("utf-8"))
self.pos = f.seek(0, os.SEEK_END)
f.flush()
os.fsync(f.fileno())
try:
mode = os.stat(self.path).st_mode
except FileNotFoundError:
mode = stat.S_IREAD | stat.S_IWRITE
# assert that wallet file does not exist, to prevent wallet corruption (see issue #5082)
if not self.file_exists():
assert not os.path.exists(self.path)
os.replace(temp_path, self.path)
os_chmod(self.path, mode)
self._file_exists = True
self.logger.info(f"saved {self.path}")

Expand Down

5 comments on commit f495511

@accumulator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit will make issues like #8409 fail harder. This will not write anything to config/wallet if chmod fails for any reason.

@SomberNight
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but I think that is desired.

It was a bug before that we wrote data first and set permissions after.
If we want to loosen this, and accept chmod failing, that might be ok, but that is a design change that can be done separately.
Even if we silence exceptions raised by chmod, chmod should run first: so that on systems where it works, we don't leak sensitive data.

Re the config file, it should have reasonable unix permission as its contents are not authenticated in any way. If we cannot set 0600, we should hard fail. Note that even being able to read the config file is security sensitive, as it contains the rpcpassword.

For wallet files re storage.write(), I guess we could tolerate chmod failing (as in #8997).

@accumulator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but I think that is desired.

Ok, however I think dumping an exception on the user is not the solution.
This should become a warning/error message dialog.

It was a bug before that we wrote data first and set permissions after. If we want to loosen this, and accept chmod failing, that might be ok, but that is a design change that can be done separately. Even if we silence exceptions raised by chmod, chmod should run first: so that on systems where it works, we don't leak sensitive data.

Re the config file, it should have reasonable unix permission as its contents are not authenticated in any way. If we cannot set 0600, we should hard fail. Note that even being able to read the config file is security sensitive, as it contains the rpcpassword.

For wallet files re storage.write(), I guess we could tolerate chmod failing (as in #8997).

What we can do is stat the file after (failed) chmod , or even stat the path towards to the file. If the permissions on the file or the path towards it are acceptable, we could accept a failing chmod operation (as that would imply the user has taken sufficent precautions)

@ecdsa
Copy link
Member

@ecdsa ecdsa commented on f495511 Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@accumulator contrary to what I said this morning, I now thing this commit is fine.
The exception is rare (two cases reported), any every time users seem to be doing weird unexpected things.

@SomberNight
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, however I think dumping an exception on the user is not the solution.
This should become a warning/error message dialog.

We can look into this more if we start getting more reports. note that this code has not changed in a long time, so if chmod was regularly failing we would have more reports or users complaining.

The two reports we have are for wallet files outside the datadir, with potentially weird filesystem settings. In case of these niche failures, I think it is an okay to just log something the user might see (so as a warning+) and leave it at that. It would be overkill to implement some GUI dialogs and the like - these cases are so rare that that code would never get tested and probably just break over time.

Anyway, the diff suggested in #8997 (comment) would cover the two existing reports we know of. In particular, I suggest we only ignore the PermissionErrors, and propagate other errors. I think we could just do that and revisit if we have more examples.

Please sign in to comment.