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

Password protect the JSONRPC interface #3374

Closed
ghost opened this Issue Nov 24, 2017 · 75 comments

Comments

Projects
None yet
@ghost

ghost commented Nov 24, 2017

The JSONRPC interface is currently completely unprotected, I believe it should be a priority to add at least some form of password protection.

Scans for the JSONRPC interface of Ethereum wallets have already started:
https://www.bleepingcomputer.com/news/security/theres-some-intense-web-scans-going-on-for-bitcoin-and-ethereum-wallets/

@SoundButton

This comment has been minimized.

Show comment
Hide comment
@SoundButton

SoundButton Nov 25, 2017

Even if a hacker got access to your wallet somehow it's still encrypted with strong password i hope.

SoundButton commented Nov 25, 2017

Even if a hacker got access to your wallet somehow it's still encrypted with strong password i hope.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 25, 2017

The JSONRPC interface is not about your personal wallet at home. It is mostly used by web servers for remotely executing commands, like handling a wallet via a web interface or accepting web payments.

While the electrum daemon is running, someone on a different virtual host of the web server could easily access your wallet via the local RPC port.

Currently, there is no security/authentication, giving someone access to the RPC port full access to the wallet.

ghost commented Nov 25, 2017

The JSONRPC interface is not about your personal wallet at home. It is mostly used by web servers for remotely executing commands, like handling a wallet via a web interface or accepting web payments.

While the electrum daemon is running, someone on a different virtual host of the web server could easily access your wallet via the local RPC port.

Currently, there is no security/authentication, giving someone access to the RPC port full access to the wallet.

@taviso

This comment has been minimized.

Show comment
Hide comment
@taviso

taviso Jan 6, 2018

Hello, I'm not a bitcoin user, a colleague pointed me at this bug report because localhost RPC servers drive me crazy 😛.

I installed Electrum to look, and I'm confused why this isn't being treated as a critical and urgent vulnerability? If this bug wasn't already open for months, I would have reported this as a vulnerability, but maybe I misunderstand something.

The JSON RPC server is enabled by default, it does use a random port but a website can simply scan for the right port in seconds.

I made you a demo. It's very basic, but you get the idea.

If you did set a password, some misdirection is required, but it's still game over, no?

Here is how I reproduced:

  • Install Electrum 3.0.3 on Windows.
  • Create a new wallet, all default settings. I left the wallet password blank - the default setting.
  • Visit in Chrome.
  • Wait a few seconds while it guesses the port, then an alert() appears with: seed: {"id": 0.7398595146147573, "result": "pony south strike horror throw acquire able afford pen lunch monster runway", "jsonrpc": "2.0"}

(Note: i dont use bitcoin, you can steal my empty wallet if you like)

taviso commented Jan 6, 2018

Hello, I'm not a bitcoin user, a colleague pointed me at this bug report because localhost RPC servers drive me crazy 😛.

I installed Electrum to look, and I'm confused why this isn't being treated as a critical and urgent vulnerability? If this bug wasn't already open for months, I would have reported this as a vulnerability, but maybe I misunderstand something.

The JSON RPC server is enabled by default, it does use a random port but a website can simply scan for the right port in seconds.

I made you a demo. It's very basic, but you get the idea.

If you did set a password, some misdirection is required, but it's still game over, no?

Here is how I reproduced:

  • Install Electrum 3.0.3 on Windows.
  • Create a new wallet, all default settings. I left the wallet password blank - the default setting.
  • Visit in Chrome.
  • Wait a few seconds while it guesses the port, then an alert() appears with: seed: {"id": 0.7398595146147573, "result": "pony south strike horror throw acquire able afford pen lunch monster runway", "jsonrpc": "2.0"}

(Note: i dont use bitcoin, you can steal my empty wallet if you like)

@devrandom

This comment has been minimized.

Show comment
Hide comment
@devrandom

devrandom Jan 6, 2018

Contributor

Looks like electrum unconditionally returns a positive CORS pre-flight check. Looks like a serious issue.

Contributor

devrandom commented Jan 6, 2018

Looks like electrum unconditionally returns a positive CORS pre-flight check. Looks like a serious issue.

@mithrandi

This comment has been minimized.

Show comment
Hide comment
@mithrandi

mithrandi Jan 6, 2018

Contributor

The code doing this dates back to the original implementation:

electrum/gui/jsonrpc.py

Lines 32 to 42 in 2c67de8

class RequestHandler(SimpleJSONRPCRequestHandler):
def do_OPTIONS(self):
self.send_response(200)
self.end_headers()
def end_headers(self):
self.send_header("Access-Control-Allow-Headers",
"Origin, X-Requested-With, Content-Type, Accept")
self.send_header("Access-Control-Allow-Origin", "*")
SimpleJSONRPCRequestHandler.end_headers(self)

I suspect this should just be removed, but there may be a reason it's there?

Contributor

mithrandi commented Jan 6, 2018

The code doing this dates back to the original implementation:

electrum/gui/jsonrpc.py

Lines 32 to 42 in 2c67de8

class RequestHandler(SimpleJSONRPCRequestHandler):
def do_OPTIONS(self):
self.send_response(200)
self.end_headers()
def end_headers(self):
self.send_header("Access-Control-Allow-Headers",
"Origin, X-Requested-With, Content-Type, Accept")
self.send_header("Access-Control-Allow-Origin", "*")
SimpleJSONRPCRequestHandler.end_headers(self)

I suspect this should just be removed, but there may be a reason it's there?

mithrandi added a commit to mithrandi/electrum that referenced this issue Jan 6, 2018

Stop allowing CORS for the JSON-RPC server
As far as I can tell, there is no need to allow this, and doing so poses severe security risks (see spesmilo#3374).
@ecdsa

This comment has been minimized.

Show comment
Hide comment
@ecdsa

ecdsa Jan 6, 2018

Member

thanks for pointing this out. this was introduced when the jsonrpc interface was merged with the daemon 4682d95

Member

ecdsa commented Jan 6, 2018

thanks for pointing this out. this was introduced when the jsonrpc interface was merged with the daemon 4682d95

@taviso

This comment has been minimized.

Show comment
Hide comment
@taviso

taviso Jan 6, 2018

Thanks for investigating so quickly, in my opinion this merits a new release and an announcement, what do you think?

I think just scanning for people in the background of a website is really easy, and seems likely someone will try that. Even with encrypted wallets, you can still change options, change destination addresses, deanonymize users via listaddresses and so on.

Maybe there is a way to do more, like setting requests_dir to the wallet directory? I noticed if you set {method:"gui" param=[{uri: <bitcoinuri>}] it will silently change any address you've already entered - that seems kinda scary.

taviso commented Jan 6, 2018

Thanks for investigating so quickly, in my opinion this merits a new release and an announcement, what do you think?

I think just scanning for people in the background of a website is really easy, and seems likely someone will try that. Even with encrypted wallets, you can still change options, change destination addresses, deanonymize users via listaddresses and so on.

Maybe there is a way to do more, like setting requests_dir to the wallet directory? I noticed if you set {method:"gui" param=[{uri: <bitcoinuri>}] it will silently change any address you've already entered - that seems kinda scary.

@ecdsa

This comment has been minimized.

Show comment
Hide comment
@ecdsa

ecdsa Jan 6, 2018

Member

indeed we will do a new release

Member

ecdsa commented Jan 6, 2018

indeed we will do a new release

kyuupichan added a commit to Electron-Cash/Electron-Cash that referenced this issue Jan 6, 2018

Stop allowing CORS for the JSON-RPC server
As far as I can tell, there is no need to allow this, and doing so poses severe security risks (see spesmilo#3374).

ecdsa added a commit that referenced this issue Jan 6, 2018

Stop allowing CORS for the JSON-RPC server
As far as I can tell, there is no need to allow this, and doing so poses severe security risks (see #3374).
@ecdsa

This comment has been minimized.

Show comment
Hide comment
@ecdsa

ecdsa Jan 6, 2018

Member

A new release (3.0.4) is available on the website and on Google Play, that disables CORS.
We may enable it again after we add password protection.

Member

ecdsa commented Jan 6, 2018

A new release (3.0.4) is available on the website and on Google Play, that disables CORS.
We may enable it again after we add password protection.

@kirelagin

This comment has been minimized.

Show comment
Hide comment
@kirelagin

kirelagin Jan 7, 2018

TBH simply disabling CORS doesn’t seem to be a proper solution, given that there is plenty of software other than browsers that runs locally.

I’d expect this RPC server to at least be disabled by default and, even better, password protected, given that, I suspect, the vast majority of users never use it and don’t even know that it exists.

kirelagin commented Jan 7, 2018

TBH simply disabling CORS doesn’t seem to be a proper solution, given that there is plenty of software other than browsers that runs locally.

I’d expect this RPC server to at least be disabled by default and, even better, password protected, given that, I suspect, the vast majority of users never use it and don’t even know that it exists.

@SomberNight

This comment has been minimized.

Show comment
Hide comment
@SomberNight

SomberNight Jan 7, 2018

Member

@kirelagin The application itself uses the RPC server to work. Password protection is currently being implemented.

Member

SomberNight commented Jan 7, 2018

@kirelagin The application itself uses the RPC server to work. Password protection is currently being implemented.

@kirelagin

This comment has been minimized.

Show comment
Hide comment
@kirelagin

kirelagin Jan 7, 2018

@SomberNight Oh, I see.

In that case you’ll still need a way to securely agree on a password betwen the daemon and the GUI app. I mean, if you simply write the password to a file for the GUI to read, rouge local apps will also get it from there :(.

kirelagin commented Jan 7, 2018

@SomberNight Oh, I see.

In that case you’ll still need a way to securely agree on a password betwen the daemon and the GUI app. I mean, if you simply write the password to a file for the GUI to read, rouge local apps will also get it from there :(.

@SomberNight

This comment has been minimized.

Show comment
Hide comment
@SomberNight

SomberNight Jan 7, 2018

Member

@kirelagin Yes, I know. Though the same apps might as well go for the wallet file directly, right?

Member

SomberNight commented Jan 7, 2018

@kirelagin Yes, I know. Though the same apps might as well go for the wallet file directly, right?

@kirelagin

This comment has been minimized.

Show comment
Hide comment
@kirelagin

kirelagin Jan 7, 2018

@SomberNight Yep, you are right, there is nothing you can do about that, so that’s the problem of the user who doesn’t set a password.
Though, as @taviso pointed out, signing transactions is not the only issue, being able to modify GUI input fields via this API is also a serious one.

kirelagin commented Jan 7, 2018

@SomberNight Yep, you are right, there is nothing you can do about that, so that’s the problem of the user who doesn’t set a password.
Though, as @taviso pointed out, signing transactions is not the only issue, being able to modify GUI input fields via this API is also a serious one.

erictapen added a commit to erictapen/nixpkgs that referenced this issue Jan 7, 2018

electrum: 3.0.3 -> 3.0.4
From the release notes [1]:

 * Fix a vulnerability caused by Cross-Origin Resource Sharing (CORS)
   in the JSONRPC interface. Previous versions of Electrum are
   vulnerable to port scanning and deanonimization attacks from
   malicious websites. Wallets that are not password-protected are
   vulnerable to theft.

See this [2] for explanation.

[1] https://github.com/spesmilo/electrum/blob/3.0.4/RELEASE-NOTES
[2] spesmilo/electrum#3374

erictapen added a commit to erictapen/nixpkgs that referenced this issue Jan 7, 2018

electrum: 3.0.3 -> 3.0.4
From the release notes [1]:

 * Fix a vulnerability caused by Cross-Origin Resource Sharing (CORS)
   in the JSONRPC interface. Previous versions of Electrum are
   vulnerable to port scanning and deanonimization attacks from
   malicious websites. Wallets that are not password-protected are
   vulnerable to theft.

See this [2] for explanation.

[1] https://github.com/spesmilo/electrum/blob/3.0.4/RELEASE-NOTES
[2] spesmilo/electrum#3374

@erictapen erictapen referenced this issue Jan 7, 2018

Merged

electrum: 3.0.3 -> 3.0.4 #33548

0 of 8 tasks complete

cryptapus added a commit to cryptapus/electrum-myr that referenced this issue Jan 7, 2018

@h43z

This comment has been minimized.

Show comment
Hide comment
@h43z

h43z Jan 7, 2018

Is it correct that without explicitly loading the wallet with electrum daemon load_wallet one cannot run commands against the RPC? So a "normal" desktop user of electrum cannot be attacked, right?

h43z commented Jan 7, 2018

Is it correct that without explicitly loading the wallet with electrum daemon load_wallet one cannot run commands against the RPC? So a "normal" desktop user of electrum cannot be attacked, right?

@fyookball

This comment has been minimized.

Show comment
Hide comment
@fyookball

fyookball Jan 7, 2018

good work guys. We ported the patch to E.C. thanks.

fyookball commented Jan 7, 2018

good work guys. We ported the patch to E.C. thanks.

@kirelagin

This comment has been minimized.

Show comment
Hide comment
@kirelagin

kirelagin Jan 7, 2018

@h43z It is not correct. The RPC daemon is started automatically as soon as you launch the GUI app.

kirelagin commented Jan 7, 2018

@h43z It is not correct. The RPC daemon is started automatically as soon as you launch the GUI app.

@taviso

This comment has been minimized.

Show comment
Hide comment
@taviso

taviso Jan 7, 2018

@h43z No, there is a load_wallet rpc, method=daemon, params=[{subcommand: “load_wallet“}].

taviso commented Jan 7, 2018

@h43z No, there is a load_wallet rpc, method=daemon, params=[{subcommand: “load_wallet“}].

@h43z

This comment has been minimized.

Show comment
Hide comment
@h43z

h43z Jan 7, 2018

got ya guys. One can load the the wallet via rpc... 🤦‍♂️

h43z commented Jan 7, 2018

got ya guys. One can load the the wallet via rpc... 🤦‍♂️

@SomberNight

This comment has been minimized.

Show comment
Hide comment
@SomberNight

SomberNight Jan 7, 2018

Member

To sum up the parts that are probably the most interesting,
on an internet-connected device, when you open a website that exploits this, if Electrum (pre-3.0.4) is running at that time,

  • the xpub of any wallet can be read
  • the seed words/private keys can be read directly IF your wallet has no password set
    • if there is a password, the ciphertext can not be retrieved, only online bruteforce is possible (making a new json-rpc request for every password-retry)
Member

SomberNight commented Jan 7, 2018

To sum up the parts that are probably the most interesting,
on an internet-connected device, when you open a website that exploits this, if Electrum (pre-3.0.4) is running at that time,

  • the xpub of any wallet can be read
  • the seed words/private keys can be read directly IF your wallet has no password set
    • if there is a password, the ciphertext can not be retrieved, only online bruteforce is possible (making a new json-rpc request for every password-retry)
@mautematico

This comment has been minimized.

Show comment
Hide comment
@mautematico

mautematico Jan 8, 2018

@loshchil it is irresponsible itself if a user decides not to encrypt their wallets.
Also, users don't need to be familiar with github or anything else. They just need to keep their software up-to-date.

However, I agree that we need a proper way to let users know there are critical bugs.
Are you able to come up with a solution and propose it in a PR?

mautematico commented Jan 8, 2018

@loshchil it is irresponsible itself if a user decides not to encrypt their wallets.
Also, users don't need to be familiar with github or anything else. They just need to keep their software up-to-date.

However, I agree that we need a proper way to let users know there are critical bugs.
Are you able to come up with a solution and propose it in a PR?

@loshchil

This comment has been minimized.

Show comment
Hide comment
@loshchil

loshchil Jan 8, 2018

@mautematico

They just need to keep their software up-to-date.

The current situation we are in is a good example that the above-mentioned view is hard to defend if user's security is taken into account. More specifically, a few moments after the vulnerability appeared on github, a ton of responsible users who used the most recent version so far have become targets to trivial exploits (i.e., hundreds or thousands of hackers are qualified to implement them) which could allow to hack wallets with or without brute-forcing their passwords.

I appreciate your proposal to come with a solution and a PR. However, I will limit my contribution to this reply here.

My proposal would be to
i) Make Electrum regularly check whether a more recent version is available.
ii) Modify Electrum main window title/caption "Electrum x.y.z" to "Electrum x.y.z TEXT",
where TEXT depends on the situation we are in. For instance, it can be empty, "(is not safe to use)",
"(outdated but does not have known vulnerabilities)", "(outdated and has known vulnerabilities)"
iii) Update Electrum.org to have a section where known vulnerabilities are described version-wise and a set of best practices to deal with them is presented.

loshchil commented Jan 8, 2018

@mautematico

They just need to keep their software up-to-date.

The current situation we are in is a good example that the above-mentioned view is hard to defend if user's security is taken into account. More specifically, a few moments after the vulnerability appeared on github, a ton of responsible users who used the most recent version so far have become targets to trivial exploits (i.e., hundreds or thousands of hackers are qualified to implement them) which could allow to hack wallets with or without brute-forcing their passwords.

I appreciate your proposal to come with a solution and a PR. However, I will limit my contribution to this reply here.

My proposal would be to
i) Make Electrum regularly check whether a more recent version is available.
ii) Modify Electrum main window title/caption "Electrum x.y.z" to "Electrum x.y.z TEXT",
where TEXT depends on the situation we are in. For instance, it can be empty, "(is not safe to use)",
"(outdated but does not have known vulnerabilities)", "(outdated and has known vulnerabilities)"
iii) Update Electrum.org to have a section where known vulnerabilities are described version-wise and a set of best practices to deal with them is presented.

mbakke pushed a commit to guix-mirror/guix that referenced this issue Jan 9, 2018

gnu: Electrum: Update to 3.0.5 [security fixes].
This fixes a critical security issue:

<spesmilo/electrum#3374>

* gnu/packages/finance.scm (electrum): Update to 3.0.5.
@n0bd

This comment has been minimized.

Show comment
Hide comment
@n0bd

n0bd Jan 9, 2018

@attritionorg, invalid reference, because that bug i submitted is not by any means related to this issue, if you had properly read and understood the bug report you would understand that, but youre probably too busy sending people boxes of feces to care

n0bd commented Jan 9, 2018

@attritionorg, invalid reference, because that bug i submitted is not by any means related to this issue, if you had properly read and understood the bug report you would understand that, but youre probably too busy sending people boxes of feces to care

@msadar

This comment has been minimized.

Show comment
Hide comment
@msadar

msadar Jan 9, 2018

@ecdsa

what if I never access to the previous version and deleted the wallet software? which means keeping as cold storage?

msadar commented Jan 9, 2018

@ecdsa

what if I never access to the previous version and deleted the wallet software? which means keeping as cold storage?

@leonklingele

This comment has been minimized.

Show comment
Hide comment
@leonklingele

leonklingele Jan 9, 2018

@msadar you were only susceptible to an attack while Electrum was running and your wallet unlocked.
Nevertheless, as it is unknown whether the bug has been exploited, it might be worth to create a new cold wallet / Electrum wallet, encrypt it, and transfer your funds to it.

leonklingele commented Jan 9, 2018

@msadar you were only susceptible to an attack while Electrum was running and your wallet unlocked.
Nevertheless, as it is unknown whether the bug has been exploited, it might be worth to create a new cold wallet / Electrum wallet, encrypt it, and transfer your funds to it.

@AngelTs

This comment has been minimized.

Show comment
Hide comment
@AngelTs

AngelTs Jan 10, 2018

It is an excellent example why you should use only the ported version:
because the ported version (even with bug(s)) in most cases is used for a very short time (set cd/usb stick, do stuff, then remove it), while installed version is very probably to works all time!

AngelTs commented Jan 10, 2018

It is an excellent example why you should use only the ported version:
because the ported version (even with bug(s)) in most cases is used for a very short time (set cd/usb stick, do stuff, then remove it), while installed version is very probably to works all time!

@zatricky

This comment has been minimized.

Show comment
Hide comment
@zatricky

zatricky Jan 11, 2018

@ecdsa re CVE id, is there a place where we will know it has been issued? Or will you be posting it here when it has been issued?

zatricky commented Jan 11, 2018

@ecdsa re CVE id, is there a place where we will know it has been issued? Or will you be posting it here when it has been issued?

@attritionorg

This comment has been minimized.

Show comment
Hide comment
@attritionorg

attritionorg Jan 11, 2018

That sheet is for assignments requested via DWF, a CNA that handles some open source software. If you request via the form on MITRE's web site, you won't be able to look it up until it is published, or the requesting party receives the assignment and posts it here.

attritionorg commented Jan 11, 2018

That sheet is for assignments requested via DWF, a CNA that handles some open source software. If you request via the form on MITRE's web site, you won't be able to look it up until it is published, or the requesting party receives the assignment and posts it here.

@petterreinholdtsen

This comment has been minimized.

Show comment
Hide comment
@petterreinholdtsen

petterreinholdtsen Jan 18, 2018

Hi. Do you need help getting a CVE assigned, or is the process already in progress? I could ask the Debian security team for one, if it is useful.

petterreinholdtsen commented Jan 18, 2018

Hi. Do you need help getting a CVE assigned, or is the process already in progress? I could ask the Debian security team for one, if it is useful.

@ecdsa

This comment has been minimized.

Show comment
Hide comment
@ecdsa

ecdsa Jan 18, 2018

Member

@petterreinholdtsen I filled the form on MITRE, and my request is listed on the document linked above. Is there something else I need to do? I am not familiar with the process.

Member

ecdsa commented Jan 18, 2018

@petterreinholdtsen I filled the form on MITRE, and my request is listed on the document linked above. Is there something else I need to do? I am not familiar with the process.

Flurbos added a commit to Flurbos/electrum-uno that referenced this issue Jan 19, 2018

@petterreinholdtsen

This comment has been minimized.

Show comment
Hide comment
@petterreinholdtsen

petterreinholdtsen Jan 20, 2018

I'm not sure either, but have no better suggestion. I asked on #debian-security and it seem to be the recommended approach based on the feedback there.

petterreinholdtsen commented Jan 20, 2018

I'm not sure either, but have no better suggestion. I asked on #debian-security and it seem to be the recommended approach based on the feedback there.

@carnil

This comment has been minimized.

Show comment
Hide comment
@carnil

carnil Jan 28, 2018

The assigned CVE for this issue looks to be CVE-2018-1000022

carnil commented Jan 28, 2018

The assigned CVE for this issue looks to be CVE-2018-1000022

@attritionorg

This comment has been minimized.

Show comment
Hide comment
@attritionorg

attritionorg Feb 2, 2018

@carnil have a reference for that? CVE-2018-6353 was assigned and opened:

http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-6353

attritionorg commented Feb 2, 2018

@carnil have a reference for that? CVE-2018-6353 was assigned and opened:

http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-6353

@mithrandi

This comment has been minimized.

Show comment
Hide comment
@mithrandi

mithrandi Feb 2, 2018

Contributor

CVE-2018-6353 is for #3678, a different issue.

Contributor

mithrandi commented Feb 2, 2018

CVE-2018-6353 is for #3678, a different issue.

@carnil

This comment has been minimized.

Show comment
Hide comment
@carnil

carnil Feb 2, 2018

@attritionorg: it was apparently assigned by DWF project. I got that confirmation by MITRE, since I stumpled over the CVE when I was investigating CVE-2018-6353, which mentioned "a different vulnerability than CVE-2018-1000022". I queried about this MITRE, who confirmed that DWF has assigned the CVE (but not yet published back?).

As @mithrandi stated the CVE-2018-6353 is for #3678, whereas CVE-2018-1000022 is for #3374

carnil commented Feb 2, 2018

@attritionorg: it was apparently assigned by DWF project. I got that confirmation by MITRE, since I stumpled over the CVE when I was investigating CVE-2018-6353, which mentioned "a different vulnerability than CVE-2018-1000022". I queried about this MITRE, who confirmed that DWF has assigned the CVE (but not yet published back?).

As @mithrandi stated the CVE-2018-6353 is for #3678, whereas CVE-2018-1000022 is for #3374

@mautematico

This comment has been minimized.

Show comment
Hide comment
@mautematico

mautematico Apr 8, 2018

Someone could please delete file posted by @Ruethairat?

mautematico commented Apr 8, 2018

Someone could please delete file posted by @Ruethairat?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment