wallet: Replace %w by wallet name in -walletnotify script #13339

promag · 2018-05-29T13:45:55Z

laanwj · 2018-05-29T15:15:06Z

src/wallet/wallet.cpp

@@ -988,6 +988,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    if (!strCmd.empty())
    {
        boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
+        boost::replace_all(strCmd, "%w", GetName());


As the wallet name can contain any character, we need shell-escaping here.

promag · 2018-05-29T16:11:05Z

@laanwj see 4fa29f8, what would be the solution in that case? only escape '? IIUC there is no function to do that as it depends on the shell.

jamesob · 2018-05-29T16:27:07Z

Concept ACK - seems like a nice consideration

laanwj · 2018-05-29T17:17:55Z

@laanwj see 4fa29f8, what would be the solution in that case? only escape '? IIUC there is no function to do that as it depends on the shell.

The best way would be to use an invocation (use one of the exec* functions) that passes arguments without involving the shell. That's quite a big change, though, and existing usages of the arguments might assume a shell is involved.

Lacking that, you can escape for bash and most other shells by surrounding with ' and replacing single-quotes with '"'"'.

It's extremely important to be careful about this, I have seen terrible things happen due to unproperly escaped shell input, and I will NACK this unless this is addressed.

promag · 2018-05-29T17:29:56Z

@laanwj exec* yes that would be breaking change. Agree on the NACK reasons. I think I'll go with https://unix.stackexchange.com/a/30936. I'll change the test to cover more cases.

laanwj · 2018-05-29T17:52:28Z

@laanwj exec* yes that would be breaking change. Agree on the NACK reasons. I think I'll go with https://unix.stackexchange.com/a/30936. I'll change the test to cover more cases.

Agree. Right - things to test would be $VAR and ${VAR} in the name, $(command) as well as various quote characters.

promag · 2018-05-29T20:13:21Z

Agree. Right - things to test would be $VAR and ${VAR} in the name, $(command) as well as various quote characters.

I don't understand what you mean. You suggest the wallet name can have $VAR?

laanwj · 2018-05-30T01:33:28Z

I don't understand what you mean. You suggest the wallet name can have $VAR?

Yes, those sequences could be part of the wallet name. They are the typical ways to fool shell escaping and do arbitrary command execution, so I suggested them for testing whether shell escaping is working as expected.

jonasschnelli · 2018-05-30T08:37:54Z

Not sure if we want to further extend walletnotify.
A more future-proof way is probably the recently created RPC approach #13262.
It would still be possible to ship a little python script that emulates the walletnotify "API".

promag · 2018-05-30T09:04:49Z

IMO we should consider adding this to fully support multi wallets without taking into account #13262. For existing systems that are based on -walletnotify changing to listsincetx would be more problematic. Beside that, currently listsincetx requires a connection for each wallet. I do consider listsincetx better than -walletnotify though.

laanwj · 2018-05-30T10:06:15Z

I think it's fine to add this to walletnotify, too. Agree that another notification system would be better, but this is useful to users in the short term. It's is a small change to the code.

maflcko · 2018-05-30T13:58:35Z

Needs rebase due to merge of #13341

laanwj · 2018-05-31T06:01:05Z

src/wallet/wallet.cpp

@@ -988,6 +988,9 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    if (!strCmd.empty())
    {
        boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
+        std::string name = GetName();
+        boost::replace_all(name, "'", "'\''");


Please factor this out to a function ShellEscape which does boost::replace_all(name, "'", "'\''");; return "'" + name + "'". We might need that in other places at some point, and it makes it an easier to review unit.
(also it's possible that for WIN32/cmd this needs something else?)

luke-jr · 2018-06-12T21:28:12Z

test/functional/feature_notifications.py

@@ -24,7 +24,8 @@ def setup_network(self):
                            "-blocknotify=echo %%s >> %s" % self.block_filename],
                           ["-blockversion=211",
                            "-rescan",
-                            "-walletnotify=echo %%s >> %s" % self.tx_filename]]
+                            "-wallet=t- 1",
+                            "-walletnotify=echo %%w\:%%s >> %s" % self.tx_filename]]


This would work even without %w properly escaped.

Could just use '...%w\:%s >> {}'.format(self.tx_filename) to avoid this confusion.

laanwj · 2018-09-12T09:30:10Z

Needs rebase and comments addressed.

meshcollider · 2018-11-12T14:43:33Z

LGTM cef0327 but yeah needs rebase + comments addressed 👍

practicalswift · 2018-11-13T12:36:26Z

@promag This no longer compiles:

error: no member named 'replace_all' in namespace 'boost'

Perhaps a good opportunity to get rid of boost::replace_all? :-)

test/functional/feature_notifications.py

luke-jr · 2019-10-21T21:23:09Z

test/functional/feature_notifications.py


 class NotificationsTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 2
        self.setup_clean_chain = True

    def setup_network(self):
+        self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHAR_BLACKLIST)


Maybe prefix it with "wallet" so you can tab-complete if you ever need to touch it...

This looks potentially dangerous...

Maybe prefix it with "wallet" so you can tab-complete if you ever need to touch it...

I like luke's suggestion to prefix with something. Maybe a prefix like "feature_notifications" to give a hint where the file comes from, and to help with tab completion

~~Just prefixed with wallet.~~ not yet

src/util/system.cpp

laanwj · 2019-12-13T14:25:44Z

ACK 8267184

promag · 2020-01-06T03:42:35Z

BTW #17878 adds support for wallet ZMQ notifications which include the wallet name.

ryanofsky · 2020-01-08T21:36:40Z

src/wallet/wallet.cpp

@@ -1156,6 +1156,11 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    if (!strCmd.empty())
    {
        boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
+#ifdef WIN32
+        boost::replace_all(strCmd, "%w", GetName());


This line is unsafe since it can corrupt wallet names and trigger arbitrary shell commands to run from an RPC. Suggested fix from previous thread #13339 (comment) is 26e0017

promag · 2020-01-09T16:14:07Z

Thanks @ryanofsky, rebased, squashed and added you as co author. Updated test to take into account behavior difference in windows.

ryanofsky

Code review ACK a89a170

ryanofsky · 2020-01-09T16:23:25Z

test/functional/feature_notifications.py


 class NotificationsTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 2
        self.setup_clean_chain = True

    def setup_network(self):
+        self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHAR_BLACKLIST)


This looks potentially dangerous...

Maybe prefix it with "wallet" so you can tab-complete if you ever need to touch it...

I like luke's suggestion to prefix with something. Maybe a prefix like "feature_notifications" to give a hint where the file comes from, and to help with tab completion

ryanofsky · 2020-01-09T16:35:56Z

test/functional/feature_notifications.py

@@ -13,13 +13,19 @@
    connect_nodes,
 )

+# Linux allow all characters other than \x00


Even with various locales? How is Unicode handled?

Should be no issue here. This remains in unicode and utf8 formats until it hits bitcoin filesystem code which uses utf8 directly on non-windows systems, and converts to wide characters for windows apis.

ryanofsky · 2020-01-09T17:02:10Z

test/functional/feature_notifications.py

+# Windows disallow control characters (0-31) and /\?%:|"<>
+FILE_CHAR_START = 32 if os.name == 'nt' else 1
+FILE_CHAR_END = 128
+FILE_CHAR_BLACKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'


What about macOS, BSD, Amiga, etc?

Would be nice to autodetect this in the test.

It shouldn't be too hard to autodetect by just writing to files and seeing what succeeds, but in the worst case this test just doesn't work on an obscure platform and the fix is trivial, so I don't think it's is a big deal

test/functional/feature_notifications.py

jb55 · 2020-01-10T03:42:59Z

Linux Tested ACK 1c335d5

src/wallet/init.cpp

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

promag · 2020-02-17T00:40:19Z

Replaced with @ryanofsky amended commit.

laanwj · 2020-02-17T10:58:39Z

ACK 4e9efac

4e9efac test: Check wallet name in -walletnotify script (João Barbosa) 9a5b5ee wallet: Replace %w by wallet name in -walletnotify script (João Barbosa) Pull request description: Fixes #13237. ACKs for top commit: laanwj: ACK 4e9efac Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203

…fy script 4e9efac test: Check wallet name in -walletnotify script (João Barbosa) 9a5b5ee wallet: Replace %w by wallet name in -walletnotify script (João Barbosa) Pull request description: Fixes bitcoin#13237. ACKs for top commit: laanwj: ACK 4e9efac Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203

…cript Summary: test: Check wallet name in -walletnotify script (João Barbosa) wallet: Replace %w by wallet name in -walletnotify script (João Barbosa) Pull request description: Fixes #13237. https://github.com/bitcoin/bitcoin/pull/13339/files --- Backport of Core [[bitcoin/bitcoin#13339 | PR13339]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8129

…fy script 4e9efac test: Check wallet name in -walletnotify script (João Barbosa) 9a5b5ee wallet: Replace %w by wallet name in -walletnotify script (João Barbosa) Pull request description: Fixes bitcoin#13237. ACKs for top commit: laanwj: ACK 4e9efac Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203

fanquake added the Wallet label May 29, 2018

laanwj reviewed May 29, 2018

View reviewed changes

promag force-pushed the 2018-05-walletnotify branch from 4fa29f8 to 764ee97 Compare May 29, 2018 18:14

promag force-pushed the 2018-05-walletnotify branch from 764ee97 to cef0327 Compare May 30, 2018 14:21

laanwj reviewed May 31, 2018

View reviewed changes

DrahtBot mentioned this pull request Jun 14, 2018

Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python #13448

Merged

maflcko added the Needs rebase label Jun 16, 2018

luke-jr reviewed Jul 7, 2018

View reviewed changes

laanwj added this to the 0.18.0 milestone Oct 11, 2018

maflcko added the Up for grabs label Nov 12, 2018

promag force-pushed the 2018-05-walletnotify branch from cef0327 to 4132f02 Compare November 12, 2018 16:53

DrahtBot removed the Needs rebase label Nov 12, 2018

promag force-pushed the 2018-05-walletnotify branch from 0292b09 to f43c5e2 Compare November 13, 2018 13:59

maflcko removed the Up for grabs label Nov 13, 2018

maflcko reviewed Nov 13, 2018

View reviewed changes

test/functional/feature_notifications.py Outdated Show resolved Hide resolved

promag force-pushed the 2018-05-walletnotify branch from f43c5e2 to c8843e4 Compare November 13, 2018 18:14

luke-jr reviewed Oct 21, 2019

View reviewed changes

src/util/system.cpp Show resolved Hide resolved

promag mentioned this pull request Oct 22, 2019

util: add RunCommandParseJSON #15382

Merged

1 task

jb55 mentioned this pull request Dec 20, 2019

walletnotify - empty details section for watch-only addresses #17784

Closed

ryanofsky requested changes Jan 8, 2020

View reviewed changes

promag force-pushed the 2018-05-walletnotify branch 3 times, most recently from e19d9c1 to 1c335d5 Compare January 9, 2020 16:12

ryanofsky reviewed Jan 9, 2020

View reviewed changes

ryanofsky approved these changes Jan 9, 2020

View reviewed changes

meshcollider reviewed Jan 15, 2020

View reviewed changes

src/wallet/init.cpp Outdated Show resolved Hide resolved

wallet: Replace %w by wallet name in -walletnotify script

9a5b5ee

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

promag force-pushed the 2018-05-walletnotify branch from 1c335d5 to 56d2307 Compare January 15, 2020 11:47

test: Check wallet name in -walletnotify script

4e9efac

DrahtBot mentioned this pull request Feb 12, 2020

Remove CWalletTx merging logic from AddToWallet #9381

Merged

promag force-pushed the 2018-05-walletnotify branch from 56d2307 to 4e9efac Compare February 17, 2020 00:38

laanwj merged commit 4e9efac into bitcoin:master Feb 17, 2020

promag deleted the 2018-05-walletnotify branch February 17, 2020 11:11

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: Replace %w by wallet name in -walletnotify script #13339

wallet: Replace %w by wallet name in -walletnotify script #13339

Uh oh!

Conversation

promag commented May 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented May 29, 2018

Uh oh!

jamesob commented May 29, 2018

Uh oh!

laanwj commented May 29, 2018

Uh oh!

promag commented May 29, 2018

Uh oh!

laanwj commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 29, 2018

Uh oh!

laanwj commented May 30, 2018

Uh oh!

jonasschnelli commented May 30, 2018

Uh oh!

promag commented May 30, 2018

Uh oh!

laanwj commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 12, 2018

Uh oh!

meshcollider commented Nov 12, 2018

Uh oh!

practicalswift commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jan 9, 2020 • 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!

Uh oh!

laanwj commented Dec 13, 2019

Uh oh!

promag commented Jan 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 9, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jb55 commented Jan 10, 2020

Uh oh!

Uh oh!

promag commented Feb 17, 2020

Uh oh!

laanwj commented May 29, 2018 •

edited

Loading

laanwj commented May 30, 2018 •

edited

Loading

practicalswift commented Nov 13, 2018 •

edited

Loading

promag Jan 9, 2020 •

edited

Loading