- I noticed we were creating the RPC server unix domain socket with 0o775.
Instead of hunting down each individual line we create files/dirs, we should
just set a restrictive umask by default. We can still use chmod to relax this
for individual files. -- but we should try to be secure by default
- note: bitcoin core does the same
https://github.com/bitcoin/bitcoin/blame/2fe76ed8324af44c985b96455a05c3e8bec0a03e/src/common/system.cpp#L92
- the umask is set in run_electrum as opposed to __init__.py so that we don't side-effect use-cases where electrum is imported as a library
Adds a CI step to the Cirrus CI which will run claude code on the diff
of a Pull Request and fail if it finds critical security vulnerabilities
or serious code issues. Optinally it can be given a GitHub api key to
create a comment in the pull request.
Followup #10541.
Fixes tests.regtest.TestLightningSwapserver.test_swapserver_forceclose.
In the regtest bob would now signal trampoline support due to #10541 and
include Alice into the invoice trampoline as he is connected to Alice.
Alice would then try to add herself onto the trampoline route, causing
the payment to fail.
Check if self.network before trying to access it. This would trigger an
exception when toggling the trampoline checkbox in offline mode:
```
29.13 | E | gui.qt.exception_window.Exception_Hook | exception caught by crash reporter
Traceback (most recent call last):
File "/home/user/Documents/electrum/electrum/gui/qt/settings_dialog.py", line 133, in on_trampoline_checked
self.network.run_from_another_thread(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'run_from_another_thread'
31.00 | E | gui.qt.exception_window.Exception_Hook | exception caught by crash reporter
Traceback (most recent call last):
File "/home/user/Documents/electrum/electrum/gui/qt/settings_dialog.py", line 131, in on_trampoline_checked
self.network.start_gossip()
^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'start_gossip'
```
Even though the NIP-47 specification kind of defines that requests should
always pass a params dict in their request i witnessed way too often
that clients don't include it in some requests where it is technically
not neccessary and we fail on it.
Handling this gracefully improves compatibility without obvious
downsides.
Add unittest that verifies we only include r_tags for trampoline nodes
if we signal trampoline support in the invoice_features and only signal
trampoline support if we use trampoline or have only open trampoline
channels.
Make the trampoline signaling in bolt11 invoices dependent upon all
unfrozen channels being with trampoline peers instead of the trampoline
config.
Stops automatically freezing non-trampoline channels for receiving if
trampoline is enabled.
One effect of this change is that now we don't signal trampoline support
anymore in the invoice even if trampoline is enabled, if one of the
channels is with a non trampoline peer.
To limit attack surface.
Context:
- both in daemon mode and in GUI mode, we start an RPC server
- the RPC server uses HTTP basic auth, with a random password that is saved in the config file
- read access to the config file implies access to the RPC server
- the traffic is unencrypted
- by default the server listens
- on Windows, on localhost TCP
- all other platform, via unix domain sockets
- if an attacker can listen to localhost TCP traffic, and there was traffic
- they could see the plaintext RPC password and issue their own commands
- e.g. if wireshark was already installed on the system, this might not require root access
- the "ping" and "gui" commands are used by everyday operations that affect most users:
- "ping" is used when trying to launch a second instance of electrum, to contact the first instance and enforce "singleton" behaviour
- "gui" is used for URI handling (`$ xdg-open bitcoin:asdasd`)
- many other sensitive commands, that operate on wallets, require *also* the wallet password
- but note that wallet.unlock can be used by the user to bypass this and store the wallet password in memory (exposed in GUI)
I propose locking down the RPC server when running in GUI mode:
- we still start it, as it is used for "ping" and "gui" RPCs, however we disable all other RPCs
- we could opt-in enable it, using a config var, except that ofc would not help against an attacker that has filesystem write access to the config file
- so I think it's even safer to just "hardcode" disable it: however the functionality is useful for development
- I propose we branch based on `constants.net.TESTNET`
- an alternative we could branch on that is hard to fake is `is_git_clone` in run_electrum
rm the `assert fee >= 0, f"{fee=!r} must be non-negative satoshis"`
from `Abstract_Wallet.get_tx_fee_warning()` to prevent an exception when
users load a psbt with negative tx fee.
It is much easier to reason about the rpcserver if we don't allow changing its basic settings while it is already running. What does it mean to change the TCP port it is listening on ("rpcport") if it's already running? It is even problematic to change the rpcpassword: care needs to be taken to already update it for the current server.
(ref https://github.com/spesmilo/electrum/issues/6762)
This commit disallows changing all of the "rpc*" config variables if the daemon is already running.
---
Simultaneously, it also ensures rpc_password is always set and auth cannot be disabled.
Previously if there was a daemon running, and the user ran
`$ electrum setconfig rpcpassword ""` that would leave the RPC unauthenticated
for the current session. However next time the daemon restarted, get_rpc_credentials would see
the unset password and generate one.
I think this was the worst of both worlds:
- we did not really allow removing the rpc password, except for the current session, and
- perhaps unexpectedly, we would generate a new password on daemon restart
Instead now we explicitly make sure the RPC server can never get into a state where it does not have a password set.
Based on a report by `Zuzana Kotásková <36777@mail.vsfs.cz>`
The 2fa secret is not selectable or copyable, this is very inconveniant
when setting up a new 2fa wallet as the user has to somehow manually
write the secret e.g. on a paper to then enter it again in their 2fa
app. This makes the secret string copyable by clicking on it.
Catch NetLegacySinglesigScriptType and convert it to a
UserFacingException if the user tries to import a private key for which
it is not possible to get a singlesig descriptor (e.g. p2wsh).
Fixes#10536
- could not find a single project that still actually cares about bip70 [0]
- well except maybe BitPay.
- but I cannot test with BitPay:
- they have a testnet3 staging environment on test.bitpay.com
- but the SSL cert they use for bip70 has expired in 2021
- the webUI probably also has not been updated since then...
- they claim to have added LN support in 2022 in a blog post,
but it's not there on test.bitpay.com
- on mainnet, they require KYC before payment
- < ... angry noises >
- their loss then, I don't care.
- this is code that no one wants to maintain
- this does not yet delete the signed bip70 payment data for historical txs
- but it is no longer possible to export it from the GUI
[0]: https://bitcoinops.org/en/topics/bip70-payment-protocol/
As it's failing due to relative imports, this might have been broken since py2->py3 migration.
```
$ python3 ./electrum/interface.py
Traceback (most recent call last):
File "/home/user/wspace/electrum/./electrum/interface.py", line 31, in <module>
import asyncio
File "/usr/lib/python3.13/asyncio/__init__.py", line 8, in <module>
from .base_events import *
File "/usr/lib/python3.13/asyncio/base_events.py", line 18, in <module>
import concurrent.futures
File "/usr/lib/python3.13/concurrent/futures/__init__.py", line 8, in <module>
from concurrent.futures._base import (FIRST_COMPLETED,
...<9 lines>...
as_completed)
File "/usr/lib/python3.13/concurrent/futures/_base.py", line 7, in <module>
import logging
File "/home/user/wspace/electrum/electrum/logging.py", line 6, in <module>
import logging.handlers
ModuleNotFoundError: No module named 'logging.handlers'; 'logging' is not a package
```
Sander van Grieken