install-core issues

[git-sgmoore]

1. Hardcoded URLs and file paths: The script uses hardcoded URLs for downloading Bitcoin Core and its checksums. If the URLs change or the files are moved, the script will break. Similarly, the script assumes a certain directory structure. If this structure changes, the script could behave unpredictably.

2. No verification of the GPG keys: Although the script checks the GPG signatures of the downloaded files, it doesn't check the authenticity of the GPG keys used to sign the files. An attacker who can intercept the download could replace the files and the GPG keys with their own, and the script wouldn't be able to tell the difference.

3. No error handling: The script doesn't handle errors in many places. If a command fails, the script may continue running with potentially inconsistent or incorrect data.

4. Use of HTTP instead of HTTPS: The script tries to download files over HTTP before falling back to HTTPS. Downloading files over HTTP is insecure because the data is not encrypted in transit.

5. Unsecured temporary files: The script creates temporary files without using any measures to secure them. This could potentially expose sensitive data.

6. Kill command: At the end of the script, it uses a pkill command to kill the terminal, which can be seen as a risky command.

[BenWestgate]

Hello git-sgmoore, your code review is greatly appreciated.

1. I will use variables to store the URLs for downloading Bitcoin Core and its checksums. This way, if the URLs change in the future, we can easily modify the script without breaking it.
2. I actually do check the authenticity of the GPG keys from two sources: openpgp.org onion service and guix.sigs/builder-keys repository on bitcoin's github, if it cannot find the signer's public key in both these locations it will NOT import that gpg key nor count it as a valid signature towards the threshold 2 good_sigs. Were the signature file faked, they'd also need to fake guix.sigs/builder-keys and upload fraudulent keys to the key server. The last line of defense in that case is the user manually verifying the fingerprint displayed matches the name of the person they are choosing to trust. Let me know if there's anything more I should do or if you want me to explain the check_sigs() function, it probably needs better documentation if it indeed does what I said and that was not clear.
3. I can experiment with using set -e to make the script exit immediately if any command exits with a non-zero status. I've run it hundreds of times and it's generally pretty stable, can you give me some commands in particular that are likely to fail, it handles the downloads and checkums failing.
4. http://6hasakffvppilxgehrswmffqurlcjjjhd76jgvaqmsg6ul25s7t3rzyd.onion/en/download/ this is an .onion hidden service url for bitcoincore.org. It's also commented out and not used because the onion service has been down this month. I'm unsure if https was ever available with this service but I'll make sure to use it if they ever fix it.
5. There is no unencrypted sensitive data stored in temporary files, they're created with mktemp and delete themselves after the script exits
6. You'd prefer the terminal stay open? I agree it could be risky if someone was working in another terminal simultaneously and lost their work. The only purpose of that command was to clean up the user's desktop after installation. Let me know if you see any other risks besides what I've mentioned. My target user does not even know how to use the terminal so I find this unlikely, but expert users are finding all kinds of edge cases.

[bitcoin-tools]

For Question 1, I agree the wget commands shouldn't have the URLs directly in the command. The URLs should be variables at the top of the script.

@git-sgmoore does that address your concern here? If not, how do you suggest Bails would handle a change in the bitcoincore [dot] org directory structure?

[BenWestgate]

@epiccurious you've seemed to have satisfied 1 and 3.
I'm fixing 2 with #17.
4 appears to be invalid and irrelevant since the onion service is still down.

Is 5 invalid? In my bash scripts install-core and setup-persistence what temporary files are leaving sensitive data exposed to other processes on the machine? If none, we can ignore and just remove them after use or process killed.

This solves 6 by only closing the calling terminal

PARENT_PID=$(ps -o ppid= -p $$)
echo "Parent PID: $PARENT_PID"
sleep 2
kill -9 $PARENT_PID

End of install-core will say "Closing terminal window in 10 seconds, press any key to abort", count down then issue the kill command.

We should have this important issue closed early this week!

[BenWestgate]

Part 6 resolved by 69d5039.

[BenWestgate]

Part 5

$XDG_RUNTIME_DIR defines the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.

This is where we should be making our temporary files. That way other processes on the machine cannot read our files.

Done in f006701

[BenWestgate]

1. Hardcoded URLs and file paths: The script uses hardcoded URLs for downloading Bitcoin Core and its checksums. If the URLs change or the files are moved, the script will break. Similarly, the script assumes a certain directory structure. If this structure changes, the script could behave unpredictably.

@epiccurious: has a branch that resolved this. PR it again at your leisure.

2. No verification of the GPG keys: Although the script checks the GPG signatures of the downloaded files, it doesn't check the authenticity of the GPG keys used to sign the files. An attacker who can intercept the download could replace the files and the GPG keys with their own, and the script wouldn't be able to tell the difference.

I have assigned myself to this. He is right that I trust the download source to get the GPG keys and my only verification is the user and key server searching the fingerprints are correct. The key server could be faked and 99% of users will not verify fingerprints from another source.

Instead we should download the keys from github.com/bitcoin/bitcoin since we're already trusting GitHub in the install instructions, and use those keys plus some I hardcore into this project repo. Verify by default with a few of these. Then change the option to "Additional Verification" where the user can trust and untrust the current signatures in the file until they reach the threshold of 2.

3. No error handling: The script doesn't handle errors in many places. If a command fails, the script may continue running with potentially inconsistent or incorrect data.

@epiccurious handled this as well in his branch, at least where it mattered.

4. Use of HTTP instead of HTTPS: The script tries to download files over HTTP before falling back to HTTPS. Downloading files over HTTP is insecure because the data is not encrypted in transit.

This is currently irrelevant, bitcoincore dot org doesn't seem to have intentions of fixing their onion service any time soon. So we start with https clearnet.

5. Unsecured temporary files: The script creates temporary files without using any measures to secure them. This could potentially expose sensitive data.

The $TMP_DIR was changed to $XDG_RUNTIME_DIR which is restricted only to the current user, preventing other users from reading the data. Although nothing sensitive was stored in install-core.

6. Kill command: At the end of the script, it uses a pkill command to kill the terminal, which can be seen as a risky command.

This was changed to query the actual PID of the executing terminal and kill only that terminal and only after a 30 second delay. We no longer kill every terminal.

@epiccurious: we can close this issue now when you PR your 1 & 3. Good to see our first security review addressed completely.