util: make os_chmod best-effort, unless critical=True is specified

[accumulator]

A successful chmod is not strictly necessary. Changing the default to best-effort, with override option.

[SomberNight]

os.chmod is often used as a security precaution, so it is probably not a good idea to have it fail ~silently. Also, if we add this critical flag, should it not default to true? That would be a smaller change.

Btw, for the storage.py write() use case, it looks to me we might be calling os.chmod too late... The ordering does not look good. I will have a look at that. (EDIT: done in https://github.com/spesmilo/electrum/commit/f495511886aee470a1132dc945dfe449b4b07ccf)

I don't fully understand the traces in the linked issue. Would not doing the chmod even "fix" those cases? Maybe there are permission issues in general there.

[accumulator]

os.chmod is often used as a security precaution, so it is probably not a good idea to have it fail ~silently.

I don't think it adds much in terms of security. Even without chmod, default permissions usually don't grant anyone else write permissions. Read permissions, ok, that's debatable, but we have a wallet password for true security instead of relying on filesystem permissions and the OS enforcing that.

In most cases chmod just succeeds, unless the filesystem has a peculiar configuration (see e.g. my NFS comment below).

Also, if we add this critical flag, should it not default to true? That would be a smaller change.

The places where os_chmod is currently called from all looked like best-effort would be sufficient, so defaulting to critical=True just adds code to all those places.

Btw, for the storage.py write() use case, it looks to me we might be calling os.chmod too late... The ordering does not look good. I will have a look at that. (EDIT: done in f495511)

I don't fully understand the traces in the linked issue. Would not doing the chmod even "fix" those cases? Maybe there are permission issues in general there.

I've ran into similar issues when using NFS mounts, which can have very granular permissions, e.g. permission changes with chmod can be explicity granted/forbidden, independent on read/write access.

Maybe certain cloud filesystem offerings have similar restrictions.

[SomberNight]

Even without chmod, default permissions usually don't grant anyone else write permissions. Read permissions, ok, that's debatable,

I think it depends on the umask. On many systems it defaults to 0022 but on some it is 0002 - anyway, the unix group is typically not sensitive on typical DEs probably. In any case, read permissions are sensitive for wallet files.

but we have a wallet password for true security instead of relying on filesystem permissions and the OS enforcing that.

True.

I've ran into similar issues when using NFS mounts, which can have very granular permissions, e.g. permission changes with chmod can be explicity granted/forbidden, independent on read/write access.

Hmm, ok. I see.

---

Note that both reports in https://github.com/spesmilo/electrum/issues/8409 are for custom wallet directories. How about handling just that specific case? As in, e.g.:

diff --git a/electrum/storage.py b/electrum/storage.py
index d5770424a2..6e1752e5d7 100644
--- a/electrum/storage.py
+++ b/electrum/storage.py
@@ -94,7 +94,10 @@ class WalletStorage(Logger):
         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
+            try:
+                os_chmod(temp_path, mode)  # set restrictive perms *before* we write data
+            except PermissionError as e:  # tolerate NFS or similar weirdness?
+                self.logger.warning(f"cannot chmod temp wallet file: {e!r}")
             f.write(s.encode("utf-8"))
             self.pos = f.seek(0, os.SEEK_END)
             f.flush()

I would prefer logger.warning here, as it is quite unexpected to fail here in general, and probably better to tell the user.