Closed git-sgmoore closed 4 weeks ago
Hello git-sgmoore, your code review is greatly appreciated.
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.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.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.mktemp
and delete themselves after the script exitsFor 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?
@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!
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 https://github.com/BenWestgate/Bails/pull/129/commits/f006701b11e31eb31e6c03d62c47d77fa2f6d46d
- 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.
- 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.
- 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.
- 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.
- 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.
- 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.
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.
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.
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.
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.
Unsecured temporary files: The script creates temporary files without using any measures to secure them. This could potentially expose sensitive data.
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.