Do not pass in command line arguments to QApplication #16578

achow101 · 2019-08-09T19:52:41Z

QApplication takes the command line arguments and parses them itself for some built in command line arguments that it has. We don't want any of those built in arguments, so instead give it dummy arguments.

To test, you can use the -reverse option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned.

After this patch, -reverse will now give a startup error since we do not support this argument.

QApplication takes the command line arguments and parses them itself for some built in command line arguments that it has. We don't want any of those built in arguments, so instead give it dummy arguments.

promag · 2019-08-09T19:56:36Z

We don't want any of those built in arguments

Care to explain why?

achow101 · 2019-08-09T20:03:19Z

Care to explain why?

They can do unexpected things and cause issues. Qt can also just add more arguments that do other stuff. We have a larger attack surface by allowing this.

promag · 2019-08-09T21:02:00Z

For reference see

fanquake · 2019-08-10T01:19:06Z

@theuni might want to comment here, as he was asking about if the changes in #16386 would remove the ability to add qt-specific runtime args to bitcoin-qt ?.

laanwj · 2019-08-10T11:10:45Z

src/qt/bitcoin.cpp

-BitcoinApplication::BitcoinApplication(interfaces::Node& node, int &argc, char **argv):
-    QApplication(argc, argv),
+static int qt_argc = 1;
+static const char* qt_argv = "bitcoin-qt";


these can be inside the function scope

if you remove the const here, you don't need to const_cast below (which is dangerous, as now this will be put in a .rodata section and Qt might try to write it)

these can be inside the function scope

What function? Constructor?

My suggestion is to make the constructor private and add a static BitcoinApplication::create() where you could declare a ~~temporary~~ static argc and argv and the app.

Please note:

Warning: The data referred to by argc and argv must stay valid for the entire lifetime of the QCoreApplication object.

* if you remove the `const` here, you don't need to `const_cast` below (which is dangerous, as now this will be put in a `.rodata` section and Qt might try to write it)

GCC complains if it is non const.

warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]

My suggestion is to make the constructor private and add a static BitcoinApplication::create() where you could declare a ~~temporary~~ static argc and argv and the app.

That requires a copy constructor, and QApplication does not have one. Qt explicitly does not want things to be copied.

GCC complains if it is non const.

Could be:

Suggested change

static const char* qt_argv = "bitcoin-qt";

static char qt_arg[] = "bitcoin-qt";

static char* qt_argv = qt_arg;

?

That requires a copy constructor, and QApplication does not have one. Qt explicitly does not want things to be copied.

Eh, whatever you do, please don't copy around QApplication objects, even if you could. It's a singleton.
It's also definitely not necessary to solve this.

laanwj · 2019-08-10T11:10:53Z

Concept ACK

hebasto

Concept ACK

hebasto · 2019-08-10T11:41:50Z

src/qt/bitcoin.cpp

-BitcoinApplication::BitcoinApplication(interfaces::Node& node, int &argc, char **argv):
-    QApplication(argc, argv),
+static int qt_argc = 1;
+static const char* qt_argv = "bitcoin-qt";


Please note:

Warning: The data referred to by argc and argv must stay valid for the entire lifetime of the QCoreApplication object.

laanwj · 2019-08-10T11:44:54Z

@hebasto that's why the variables are static, it holds just as true for static variables within a function scope

fanquake · 2019-08-10T11:47:01Z

Concept ACK

practicalswift · 2019-08-10T18:13:55Z

Concept ACK: smaller attack surface is better.

jonasschnelli · 2019-08-10T19:15:47Z

Concept ACK.
Codewise, I agree with @laanwj that casting const away and passing it to QApplication seems unfortunate.

promag · 2019-08-10T19:35:35Z

How about this

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 5ce4f3c19..f38235b15 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -169,11 +169,8 @@ void BitcoinCore::shutdown()
     }
 }

-static int qt_argc = 1;
-static const char* qt_argv = "bitcoin-qt";
-
 BitcoinApplication::BitcoinApplication(interfaces::Node& node):
-    QApplication(qt_argc, const_cast<char **>(&qt_argv)),
+    QApplication(argc, const_cast<char **>(&argv)),
     coreThread(nullptr),
     m_node(node),
     optionsModel(nullptr),
diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
index 3869193a3..d8b7f3b36 100644
--- a/src/qt/bitcoin.h
+++ b/src/qt/bitcoin.h
@@ -55,6 +55,10 @@ private:
 class BitcoinApplication: public QApplication
 {
     Q_OBJECT
+
+    // Some wild comment here...
+    int argc{1};
+    const char* argv{"bitcoin-qt"};
 public:
     explicit BitcoinApplication(interfaces::Node& node);
     ~BitcoinApplication();

achow101 · 2019-08-10T20:08:39Z

How about this

Results in a segfault.

FWIW, making the argv const and then casting it is how QCoreApplication does it internally when you say argc is 0. The only reason that I did not choose to just do QApplication(0, 0) is because it results in the warning QSettings::value: Empty key passed being printed a couple of times (at least for me using KDE, this might actually just be a KDE thing).

laanwj · 2019-08-14T10:48:31Z

@achow101 ~~But why add the const at all if you're going to cast it away?~~

Edit: oh, because of the string constant. I get it, the string is read-only, which is fine, Qt is not going to change the contents of arguments, but the array needs to be writable as it might remove arguments.

laanwj · 2019-08-14T10:51:50Z

ACK a2714a5

hebasto · 2019-08-14T10:54:45Z

Technically, there is a solution without const.

achow101 · 2019-08-14T17:46:19Z

Technically, there is a solution without const.

I'm going to leave it as is. I feel that doing the same thing that Qt does internally is safe and there's no reason that we should not follow suit.

hebasto · 2019-08-14T17:55:40Z

FWIW, making the argv const and then casting it is how QCoreApplication does it internally when you say argc is 0.

Indeed.

ACK a2714a5

fanquake

ACK a2714a5 - Have tested that arguments like -reverse are no longer being passed through and result in an error.

a2714a5 Give QApplication dummy arguments (Andrew Chow) Pull request description: QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) that it has. We don't want any of those built in arguments, so instead give it dummy arguments. To test, you can use the `-reverse` option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned. After this patch, `-reverse` will now give a startup error since we do not support this argument. ACKs for top commit: laanwj: ACK a2714a5 hebasto: ACK a2714a5 fanquake: ACK a2714a5 - Have tested that arguments like `-reverse` are no longer being passed through and result in an error. Tree-SHA512: 983bd948ca6999f895b6662b58c37e33af7ed61fdd600c6b4623febb87ec06a92c66e3b3300783530110cc711902793ef82d751d7f563696c4c3a8416b2b1f51

fanquake · 2019-08-15T08:47:34Z

Added for backport in #16617.

…ation a2714a5 Give QApplication dummy arguments (Andrew Chow) Pull request description: QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) that it has. We don't want any of those built in arguments, so instead give it dummy arguments. To test, you can use the `-reverse` option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned. After this patch, `-reverse` will now give a startup error since we do not support this argument. ACKs for top commit: laanwj: ACK a2714a5 hebasto: ACK a2714a5 fanquake: ACK a2714a5 - Have tested that arguments like `-reverse` are no longer being passed through and result in an error. Tree-SHA512: 983bd948ca6999f895b6662b58c37e33af7ed61fdd600c6b4623febb87ec06a92c66e3b3300783530110cc711902793ef82d751d7f563696c4c3a8416b2b1f51

laanwj · 2019-09-12T05:45:03Z

What about QT_QPA_PLATFORM or one of the other environment variables?

the non-specific "larger attack surface" one cited in comments

We don't want to be too specific about this yet.

QApplication takes the command line arguments and parses them itself for some built in command line arguments that it has. We don't want any of those built in arguments, so instead give it dummy arguments. Github-Pull: bitcoin#16578 Rebased-From: a2714a5

Sjors · 2019-10-01T17:06:46Z

I made an issue about the test_bitcoin-qt -platform cocoa: #17013. I see @ryanofsky already spotted it. A QT_QPA_PLATFORM environment variable works for me.

Sjors · 2019-10-02T16:14:08Z

This also breaks the URI handler on macOS, but only when opening from the command line, not when opening from a browser. See #17025.

0b18ea6 util: Filter control characters out of log messages (Wladimir J. van der Laan) ac30fc4 build: Factor out qt translations from build system (Wladimir J. van der Laan) 3b8af5f build: update boost macros to latest upstream (fanquake) b12defc Test that joinpsbts randomly shuffles the inputs (Andrew Chow) eb07d22 Shuffle inputs and outputs after joining psbts (Andrew Chow) 1175410 addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. (practicalswift) c52dd12 Handle the result of posix_fallocate system call (Luca Venturini) f792b25 torcontrol: Use the default/standard network port for Tor hidden services, even if the internal port is set differently (Luke Dashjr) 9fe8d28 Bugfix: QA: Run tests with UPnP disabled (Luke Dashjr) 1d12e52 Add vertical spacer (Josu Goñi) d764141 depends: add patch to common dependencies (fanquake) 56815e9 Give QApplication dummy arguments (Andrew Chow) 9d389d0 util: No translation of `Bitcoin Core` in the copyright (MarcoFalke) 87908e9 scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke) a44e18f build: Stop translating PACKAGE_NAME (MarcoFalke) 7bd8f4e rpc: Fix getblocktemplate CLI example (#16594) (Emil Engler) 1cc06a1 doc: Fix typos in COPYRIGHT (Chuf) Pull request description: Backports some commits to the `0.18` branch: * #16596 - rpc: Fix getblocktemplate CLI example * #16615 - doc: Fix typos in COPYRIGHT * #16291 - gui: Stop translating PACKAGE_NAME (without the `make translate` commit) * #16578 - Do not pass in command line arguments to QApplication * #16051 - depends: add patch to common dependencies * #16090 - Add vertical spacer * #15651 - torcontrol: Use the default/standard network port for Tor hidden services, even if the internal port is set differently * #15650 - Handle the result of posix_fallocate system call * #16646 - Bugfix: QA: Run tests with UPnP disabled * #16212 - addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. * #16512 - rpc: Shuffle inputs and outputs after joining psbts * #16870 - build: update boost macros to latest upstream for improved error reporting * #16982 - build: Factor out qt translations from build system * #17095 - util: Filter control characters out of log messages ACKs for top commit: laanwj: ACK 0b18ea6 Tree-SHA512: 37f0e5afc20975f4d1506e8662eda2ae0125f2f424a852818b5af2c3b8db78fc1c365b83571aa80ca63c885ca314302190b891a50ff3851fda9b9238455a5627

Summary: a2714a5c69f0b0506689af04c3e785f71ee0915d Give QApplication dummy arguments (Andrew Chow) Pull request description: QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) that it has. We don't want any of those built in arguments, so instead give it dummy arguments. To test, you can use the `-reverse` option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned. After this patch, `-reverse` will now give a startup error since we do not support this argument. --- Backport of Core [[bitcoin/bitcoin#16578 | PR16578]] Test Plan: ninja all check check-functional Check that `bitcoin-qt -reverse -regtest` gives an error with this patch and reverses the GUI without i.t Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7747

achow101 · 2021-02-01T16:33:24Z

Disclosure of the vulnerability this PR fixes: https://achow101.com/2021/02/0.18-uri-vuln

A possible followup now that this is public would be to explicitly allow safe Qt arguments instead of flatly disabling all of them.

practicalswift · 2021-02-02T08:49:46Z

@achow101 Very nice find and thanks for sharing! Has this one been assigned a CVE yet?

hebasto · 2021-03-30T22:39:47Z

Has this one been assigned a CVE yet?

CVE-2021-3401

- bitcoin/bitcoin#16578 - https://achow101.com/2021/02/0.18-uri-vuln (CVE-2021-3401) - https://www.twitch.tv/videos/897351419 p.s. about explicit constructor for KomodoApplication class, according C++ Standart 12.3.1 "a default constructor may be an explicit constructor", so, let it be here as is (with explicit keyword).

Fixes URI Argument Injection in Ravencoin-Qt Ref: https://achow101.com/2021/02/0.18-uri-vuln Ref: bitcoin/bitcoin#16578

…ation a2714a5 Give QApplication dummy arguments (Andrew Chow) Pull request description: QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) that it has. We don't want any of those built in arguments, so instead give it dummy arguments. To test, you can use the `-reverse` option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned. After this patch, `-reverse` will now give a startup error since we do not support this argument. ACKs for top commit: laanwj: ACK a2714a5 hebasto: ACK a2714a5 fanquake: ACK a2714a5 - Have tested that arguments like `-reverse` are no longer being passed through and result in an error. Tree-SHA512: 983bd948ca6999f895b6662b58c37e33af7ed61fdd600c6b4623febb87ec06a92c66e3b3300783530110cc711902793ef82d751d7f563696c4c3a8416b2b1f51

Fixes URI Argument Injection in Ravencoin-Qt Ref: https://achow101.com/2021/02/0.18-uri-vuln Ref: bitcoin/bitcoin#16578

Give QApplication dummy arguments

a2714a5

QApplication takes the command line arguments and parses them itself for some built in command line arguments that it has. We don't want any of those built in arguments, so instead give it dummy arguments.

DrahtBot added GUI Tests labels Aug 9, 2019

fanquake removed the Tests label Aug 10, 2019

laanwj reviewed Aug 10, 2019

View reviewed changes

hebasto reviewed Aug 10, 2019

View reviewed changes

fanquake added this to the 0.19.0 milestone Aug 12, 2019

fanquake approved these changes Aug 15, 2019

View reviewed changes

fanquake merged commit a2714a5 into bitcoin:master Aug 15, 2019

fanquake mentioned this pull request Aug 15, 2019

[0.18.2] Backports #16617

Merged

Sjors mentioned this pull request Oct 1, 2019

macOs: "test_bitcoin-qt -platform cocoa" no longer works #17013

Closed

Sjors mentioned this pull request Oct 2, 2019

Duplicate send entries when opening URI from command line #17025

Closed

laanwj mentioned this pull request Oct 18, 2019

Replace #16578 with a more specific solution #17187

Closed

hebasto mentioned this pull request Jan 7, 2021

macOS 11 (Big Sur) visual bugs bitcoin-core/gui#136

Closed

2 tasks

hebasto mentioned this pull request Jan 21, 2021

Use "fusion" style on macOS Big Sur with old Qt bitcoin-core/gui#177

Merged

DeckerSU mentioned this pull request Jul 12, 2021

Patch CVE-2021-3401 pbcllc/sfusd-core#12

Merged

This was referenced Nov 3, 2021

URI Argument Injection in Dogecoin-Qt dogecoin/dogecoin#2665

Closed

URI Argument Injection in BTCGPU-Qt BTCGPU/BTCGPU#431

Open

URI Argument Injection in Ravencoin-Qt RavenProject/Ravencoin#1110

Open

yixiao5428 mentioned this pull request Nov 12, 2021

URI Argument Injection in DigiByte-Qt digibyte/digibyte#234

Open

yixiao5428 mentioned this pull request Nov 19, 2021

URI Argument Injection in Dash-Qt dashpay/dash#4578

Closed

Munkybooty mentioned this pull request Nov 25, 2021

Backports 0.19 pr8 dashpay/dash#4586

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	static const char* qt_argv = "bitcoin-qt";
	static char qt_arg[] = "bitcoin-qt";
	static char* qt_argv = qt_arg;

Do not pass in command line arguments to QApplication #16578

Do not pass in command line arguments to QApplication #16578

Uh oh!

Conversation

achow101 commented Aug 9, 2019

Uh oh!

promag commented Aug 9, 2019

Uh oh!

achow101 commented Aug 9, 2019

Uh oh!

promag commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Aug 10, 2019

Uh oh!

laanwj Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Aug 10, 2019

Choose a reason for hiding this comment

Uh oh!

promag Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 10, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 10, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 10, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 10, 2019

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 10, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 10, 2019

Uh oh!

fanquake commented Aug 10, 2019

Uh oh!

practicalswift commented Aug 10, 2019

Uh oh!

jonasschnelli commented Aug 10, 2019

Uh oh!

promag commented Aug 10, 2019

Uh oh!

achow101 commented Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 14, 2019

Uh oh!

hebasto commented Aug 14, 2019

Uh oh!

achow101 commented Aug 14, 2019

Uh oh!

hebasto commented Aug 14, 2019

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 15, 2019

Uh oh!

laanwj commented Sep 12, 2019

promag commented Aug 9, 2019 •

edited

Loading

laanwj Aug 10, 2019 •

edited

Loading

promag Aug 10, 2019 •

edited

Loading

achow101 Aug 10, 2019 •

edited

Loading

achow101 commented Aug 10, 2019 •

edited

Loading

laanwj commented Aug 14, 2019 •

edited

Loading

Sjors commented Oct 1, 2019 •

edited

Loading