Skip to content

Disable bitcoin: URI handling on Windows for the 0.6 release#991

Merged
gavinandresen merged 1 commit intobitcoin:masterfrom
gavinandresen:disableWinURI
Mar 26, 2012
Merged

Disable bitcoin: URI handling on Windows for the 0.6 release#991
gavinandresen merged 1 commit intobitcoin:masterfrom
gavinandresen:disableWinURI

Conversation

@gavinandresen
Copy link
Copy Markdown
Contributor

I don't think bitcoin: URI handling on Windows has had nearly enough testing, so this disables it for the 0.6 release.

I'm all for re-enabling it for a 0.6.1 and/or 0.7, but only we take a step back and do two things:

  1. Can we get an IPC solution that doesn't require monkey-patching boost::interprocess ?
  2. I'm nervous that attackers might be able to craft bitcoin: URIs that Do Bad Things. I would suggest that we disable Linux bitcoin: URI handling until that is fixed, but I consider bitcoin: URI handling on Linux experimental anyway because users have to know how to manually enable it.

@TheBlueMatt
Copy link
Copy Markdown
Contributor

  1. Fix boost::interprocess::detail::winapi::get_last_bootup_time #986 is a dead-simple patch which solves a known and well-documented issue in boost::interprocesses. What it does is clear and simple and there is no reason to avoid it.
  2. This one has been discussed over and over again (we've had one pull request or another sitting around getting slow discussion about the possibility of attackers hitting bitcoin with URIs since before 0.4). At this point, I have yet to hear anything remotely convincing that could indicate an attacker could do anything remotely useful to attack this.
  3. We already have to have a significant amount of time dedicated to 0.6rc5 testing before we can release, blocking URI handling on windows for 0.6 is really not the way to go. It is a very simple feature with one very minor bug (which manifests itself in several ugly ways). If there are still issues after Fix boost::interprocess::detail::winapi::get_last_bootup_time #986 is merged in 0.6rc5, then yea Id say disable it until 0.7.
  4. I really, really believe URI support is a very important usability feature (much more important than P2SH or many of the other things we've spent serious time implementing). It offers users a much, much more user-friendly way of making payto links and gives a much more professional and clean feeling to paying online merchants (which is one of the biggest use-cases for bitcoin today).

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 26, 2012

Re: URLs that do bad things, any special reason that you think this is suddenly an issue? You ACKed it first time and I don't think this aspect changed.

Given that there are no fatal bugs in boost::interprocess and the Qt URL parser, I think it's pretty unlikely there are security problems.

@gavinandresen
Copy link
Copy Markdown
Contributor Author

  1. Fix boost::interprocess::detail::winapi::get_last_bootup_time #986 is "a somewhat-hacky workaround" to a boost bug.
  2. It has been discussed over and over again, and, apparently, been essentially untested.
  3. Adding even more things to test in the 5->final release process is not a good argument.
  4. I have strong reservations about bitcoin: URIs, especially given the recent report of Javascript-based bitcoin address rewriting trojans. I think we need a better solution to the problem: "how do we confirm that we're paying who we think we're paying?"

RE: why do I suddenly think bad URLs are an issue: because when I see stupid-simple things like the setup.nsi file launching bitcoin.exe instead of bitcoin-qt.exe getting fixed this close to a final release it drives home the point that THIS FEATURE HAS NOT BEEN SUFFICIENTLY TESTED.

Excuse me for yelling, this just feels like the wallet encryption (mis-)feature all over again.

@sipa
Copy link
Copy Markdown
Member

sipa commented Mar 26, 2012

Since apparently URI's didn't work at all in windows releases so far, I really don't mind disabling URI's for now to get 0.6.0 out, and re-enable them as soon as they have actually been tested.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 26, 2012

I'm fine with disabling it for this release.

@TheBlueMatt
Copy link
Copy Markdown
Contributor

  1. Somewhat hacky in the sense that it doesnt bother fixing the issues with its COM usage, but instead just works around broken COM code. Just because its "hacky" doesnt mean its bad nor is it a reason to avoid it.
  2. It was very thoroughly tested aside from the ipc issues (which only cropped up in bitcoin-qt, and were not there using the same code in wx).
  3. Not really adding anything, because its already there, been tested on Linux/Windows, just caused a few bugs on Windows, which were found thanks to it being tested.
  4. Yes, js trojans can just as easily rewrite URIs as addresses, but what is the difference? It doesnt change the security but makes it easier to pay...
    As for how we confirm we are paying who we think we are, any sane merchant should be using SSL and sane xss protection, which all but removes any threat of js trojans (aside from browser plugins hijacking stuff). If you want to try to block that, you can try to use aliases like the old https requests but that only moderately helps. Any trojan can go and get its nice paypal.com domain with Cyrillic a's and then you are equally as screwed... This gets into the rat race issue very, very quickly...

The setup.nsi issue is an artifact of how old this stuff really is. Its been around forever, and so many people have seen it/looked over it/written it that I completely disagree with the assumption that it is untested. It was written by luke, rewritten by me, commented on by laanjw, gmaxwell, etc, etc. And has been in 0.6 since rc1.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 26, 2012

Right, the MITM issue is completely separate, and isn't worse with embedded URLs than with embedded addresses in pages or mails. The only way to protect against that would be to sign the URLs, but that brings the the whole web-of-trust/chain-of-trust shebang into bitcoin and should be considered carefully...

@TheBlueMatt
Copy link
Copy Markdown
Contributor

If everyone thinks this needs disabled for 0.6, thats fine, though Im really tired of this getting kicked down the road, it was ready to go for 0.4...

@sipa
Copy link
Copy Markdown
Member

sipa commented Mar 26, 2012

There are certainly risks involved when using URI's to transfer information, but this does not "worsen" the situation if even a copy-pasted static base58 address can get intercepted and replaced by a trojan. I hope we can enable this feature quickly. I also don't mind using very boost-specific patches to workaround a known bug on windows platforms.

That said, I don't mind delaying it to 0.6.1 or very soon for 0.7.0 if nobody even noticed it didn't work since 0.6.0rc1, when the dirty hacks can be sufficiently tested.

gavinandresen added a commit that referenced this pull request Mar 26, 2012
Disable bitcoin: URI handling on Windows for the 0.6 release
@gavinandresen gavinandresen merged commit c289d95 into bitcoin:master Mar 26, 2012
coblee referenced this pull request in litecoin-project/litecoin Jul 17, 2012
Disable bitcoin: URI handling on Windows for the 0.6 release
ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Mar 6, 2018
Remove dead code related to SizeForkExpiration consensus parameter
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Oct 30, 2019
35ec3f8 [Build] Remove OpenSSL version check (warrows)
b8fb2fc [Refactor] Remove unused OpenSSL includes (warrows)
6152805 Remove openssl info from init/log and from Qt debug window (Jonas Schnelli)

Pull request description:

  Up until now we've been checking for OpenSSL version at configure time. It turns out that it's not needed anymore. We still use OpenSSL in a few places, but it's not consensus critical anymore.
  So this PR removes the check at configure time and cleans up a few places where includes were done of OpenSSL unnecessarily.

ACKs for top commit:
  Fuzzbawls:
    ACK 35ec3f8
  random-zebra:
    ACK 35ec3f8 and merging...

Tree-SHA512: e1752261b7257c3392d319bac7749c61aa17ce33705ffc41c0cec7ab21dfb11941747f37ed82e3275c90a03e76ee59e016b1983edcf648bdb30a79083bb50f04
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants