search

bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
812 stars 296 forks source link

The `url` dependency does not prevent URL homograph attacks #1504

Open notmandatory opened 3 weeks ago

notmandatory commented 3 weeks ago

Describe the bug

The url crate that reqwest uses silently converts international domain names (IDNs). This makes BDK vulnerable to URL homograph attacks.

Blockchain clients that BDK uses should prevent URL homograph attacks by disallowing URLs with IDNs and not silently converting them.

To Reproduce

Use an IDN in a URL with the bdk_esplora blockchain client, it is silently converted to a valid domain name. This should also be tested with the bdk_electrum and any other blockchain data clients that accept user provided URLs.

Expected behavior

The bdk_esplora and any other BDK blockchain clients should return an error if an IDN is used in a URL.

Build environment

Additional context

From discord chat with @bluematt:

ugh, it looks like the url crate does IDN conversion silently without any homograph checks, which IMO makes it wholly unsuited for any kind of application that cares about security (or really anything but a browser). Post-0.1 we should really consider making switching off of reqwest and all the other rust HTTP clients that use url a priority, IMHO. ... I mean nothing but a browser should support IDNs at all IMO like, sorry you want an umlaut in your domain that hosts your electrum server, you're gonna have to deal with punycode

TheBlueMatt commented 3 weeks ago

IMO we should take this as a chance to take a hatchet to the massive dependency tree of the "normal" rust ecosystem HTTP clients (eg reqwest) and replace them all with minreq or whatever.

storopoli commented 3 weeks ago

We should discuss this tomorrow.