Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config file stores console history, this can be a privacy or security risk #5563

Closed
Coding-Enthusiast opened this issue Aug 15, 2019 · 13 comments
Labels
security 🔐 technical issue that affects security of funds

Comments

@Coding-Enthusiast
Copy link

Coding-Enthusiast commented Aug 15, 2019

The config file is storing the last 50 "strings" that were written in console. This can be both privacy concern (for those who use commands involving transactions or addresses and don't want them lying around unencrypted) or a security risk (for those who used commands involving secrets such as convert_xkey, ...).
In my opinion this feature is not needed at all and should be removed entirely. Consoles/terminals are usually storing the last couple of commands in memory as long as the console is running not forever (on disk) to make it easy to repeat the command in that session only.

If there is any other reason for this feature that I'm not aware of, then at least the following 2 needs be added:

  1. The warning (when console tab is opened) should inform user that their commands will be stored on disk.
  2. Add a new command called DeleteConsoleHistoryFromDisk (or something similar) so that user can choose to delete this through console without having to go through config file for manual deletion.
@gits7r
Copy link

gits7r commented Aug 15, 2019

Nice catch. I see absolutely no usefulness in keeping this "feature". I am wondering if it was intentional or just didn't raise in nobody's eyes until now. I don't think this is needed to anything, what is the sense to keep such stuff in the config file on disk? It makes sense to keep some last lines in RAM only in that session.

I think the % of users using console to send btc / build transactions when they have a GUI wallet tool is very small, but still this should be addressed.

@SomberNight
Copy link
Member

Consoles/terminals are usually storing the last couple of commands in memory as long as the console is running not forever (on disk) to make it easy to repeat the command in that session only.

This is not the case with bash history at least.

In my opinion this feature is not needed at all and should be removed entirely
I see absolutely no usefulness in keeping this "feature".

There are at least two usecases for the console: using it for handling real money, and debugging stuff.
I personally use it to debug stuff all the time, and there it is useful to be able to replay commands across restarts.

I think the % of users using console to send btc / build transactions when they have a GUI wallet tool is very small, but still this should be addressed.

Indeed the only people affected are power users who use the console with secret data and don't know about the history being saved. Still, I'm not arguing that they should know about the history being saved.

  1. The warning (when console tab is opened) should inform user that their commands will be stored on disk.
  2. Add a new command called DeleteConsoleHistoryFromDisk (or something similar) so that user can choose to delete this through console without having to go through config file for manual deletion.

Or there could be an option to toggle this. Although there are already so many options :)

@SomberNight SomberNight added the security 🔐 technical issue that affects security of funds label Aug 15, 2019
@ecdsa
Copy link
Member

ecdsa commented Aug 15, 2019

I dont want to add another option.
we could store the history in the wallet file.

@Coding-Enthusiast
Copy link
Author

we could store the history in the wallet file.

This could work, but I think there are more complications.
For starters it makes commands wallet specific, and if console is used for debugging and to "replay commands across restarts" then working on another wallet won't bring up the same command history and that may defeat the purpose of storing them in first place.
Also console_history would need to be always encrypted (similar to seed field inside wallet file) otherwise the problem would only be solved for those who do entire wallet encryption and not partial.

@ecdsa
Copy link
Member

ecdsa commented Aug 19, 2019

For starters it makes commands wallet specific, and if console is used for debugging and to "replay commands across restarts" then working on another wallet won't bring up the same command history and that may defeat the purpose of storing them in first place.

it's fine to make commands wallet specific

Also console_history would need to be always encrypted (similar to seed field inside wallet file) otherwise the problem would only be solved for those who do entire wallet encryption and not partial.

no it would not. you are not supposed to enter your seed or password in the console.

@Coding-Enthusiast
Copy link
Author

Also console_history would need to be always encrypted (similar to seed field inside wallet file) otherwise the problem would only be solved for those who do entire wallet encryption and not partial.

Let me rephrase this part. When we partially encrypt an Electrum wallet file, inside that JSON, fields such as addr_history, transactions,... remain unencrypted and can be read if the wallet file is opened in a text editor. However fields such as seed, xprv,... are encrypted with that password. If console_history is added to wallet file, then it has to be encrypted just like how seed,... are being encrypted.

@ldz1
Copy link
Contributor

ldz1 commented Aug 19, 2019

no it would not. you are not supposed to enter your seed or password in the console.

How about people who do not use a GUI for various reasons? If I interpret correctly, Electrum is not recommended in such a case?

In my opinion, it would be nice to have solution to this problem. Configurability sounds good. The daily user does not need to know anything about it. But if someone uses the console then he should be able to disable such disk storing.

@ecdsa
Copy link
Member

ecdsa commented Aug 19, 2019

no it would not. you are not supposed to enter your seed or password in the console.

How about people who do not use a GUI for various reasons? If I interpret correctly, Electrum is not recommended in such a case?

In my opinion, it would be nice to have solution to this problem. Configurability sounds good. The daily user does not need to know anything about it. But if someone uses the console then he should be able to disable such disk storing.

You do not understand what this issue is about.
This issue is about the console tab of the Qt GUI.
If you use a terminal and the CLI, then your history is stored elsewhere, for example your bash_history

@ldz1
Copy link
Contributor

ldz1 commented Aug 19, 2019

If you use a terminal and the CLI, then your history is stored elsewhere, for example your bash_history

This is something I have to deal with. However, I would not expect Electrum to write sensitive data to disk unless it is necessary. For me It doesn't matter if we're talking about GUI, console tab in the GUI or terminal input. I think controlling all system dependencies (like bash) is pointless. It looks different when it comes to your own files, I mean files that Electrum creates and saves its own data in them (like the mentioned config file).

Argument like the following does not quite convince me - The runtime environment stores sensitive data explicitly on the disk, which is why we will do it too.

Maybe I'm a little oversensitive :). I do not know...

@ecdsa
Copy link
Member

ecdsa commented Aug 19, 2019

Whatever is in your console history is as private as the rest of your wallet file (addresses, history, etc). There is no reason to add an extra encryption layer. Note that if you use password protected commands from the console, a password dialog pops up so that you do not need to enter your password directly in the console.

@ecdsa ecdsa closed this as completed in 2b52ee2 Aug 25, 2019
@attritionorg
Copy link

@Coding-Enthusiast Speaking to the potential security aspect, what are the default permissions of the 'config' file?

@SomberNight
Copy link
Member

@attritionorg on UNIX-based systems, I believe it is 600:

path = os.path.join(self.path, "config")
s = json.dumps(self.user_config, indent=4, sort_keys=True)
try:
with open(path, "w", encoding='utf-8') as f:
f.write(s)
os.chmod(path, stat.S_IREAD | stat.S_IWRITE)
except FileNotFoundError:

@attritionorg
Copy link

@SomberNight Thanks! That means that by itself, it isn't a vulnerability I would say. It would require someone compromising an account in some fashion (e.g. another vuln, social engineering, leaked creds) to gain access to any sensitive information in that file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security 🔐 technical issue that affects security of funds
Projects
None yet
Development

No branches or pull requests

6 participants