Skip to content
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

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

Open
wants to merge 2 commits into
base: master
from

Conversation

@promag
Copy link
Member

promag commented May 29, 2018

Fixes #13237.

@fanquake fanquake added the Wallet label May 29, 2018

src/wallet/wallet.cpp Outdated
@@ -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());

This comment has been minimized.

@laanwj

laanwj May 29, 2018

Member

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

@promag

This comment has been minimized.

Copy link
Member Author

promag commented May 29, 2018

@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

This comment has been minimized.

Copy link
Member

jamesob commented May 29, 2018

Concept ACK - seems like a nice consideration

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 29, 2018

@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

This comment has been minimized.

Copy link
Member Author

promag commented May 29, 2018

@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

This comment has been minimized.

Copy link
Member

laanwj commented May 29, 2018

@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 promag force-pushed the promag:2018-05-walletnotify branch May 29, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented May 29, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented May 30, 2018

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 30, 2018

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

This comment has been minimized.

Copy link
Member Author

promag commented May 30, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented May 30, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 30, 2018

Needs rebase due to merge of #13341

@promag promag force-pushed the promag:2018-05-walletnotify branch May 30, 2018

src/wallet/wallet.cpp Outdated
@@ -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, "'", "'\''");

This comment has been minimized.

@laanwj

laanwj May 31, 2018

Member

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?)

This comment has been minimized.

@promag

promag May 31, 2018

Author Member

Will do.

test/functional/feature_notifications.py Outdated
@@ -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 comment has been minimized.

@luke-jr

luke-jr Jul 7, 2018

Member

This would work even without %w properly escaped.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 8, 2018

Member

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 12, 2018

Needs rebase and comments addressed.

@laanwj laanwj added this to the 0.18.0 milestone Oct 11, 2018

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Nov 12, 2018

LGTM cef0327 but yeah needs rebase + comments addressed 👍

@promag promag force-pushed the promag:2018-05-walletnotify branch Nov 12, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 12, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 13, 2018

@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? :-)

@promag promag force-pushed the promag:2018-05-walletnotify branch Nov 13, 2018

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

@promag promag force-pushed the promag:2018-05-walletnotify branch to c8843e4 Nov 13, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 13, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14955 (Switch all RNG code to the built-in PRNG by sipa)
  • #11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)
  • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@promag promag force-pushed the promag:2018-05-walletnotify branch to ac77180 Nov 14, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Nov 15, 2018

@ken2812221 do you mind checking the last commit?

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Nov 15, 2018

Sure, will test

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Nov 17, 2018

ACK ac77180. I think it's good to test all available ascii characters. like 2f16218

std::wstring_convert<convert_typeX, wchar_t> converterX;

return converterX.to_bytes(wstr);
}

This comment has been minimized.

@ken2812221

ken2812221 Nov 19, 2018

Member

Can you explain that why ws2s and s2ws is required? I think it's fine to use std::string to add quotes.

Edit: Oh I know. It's unicode problem. But maybe it's ok if #13884 merged.

This comment has been minimized.

@ken2812221

ken2812221 Jan 10, 2019

Member

#13884 has been merged, you could use std::string instead of std::wstring and drop ws2s, s2ws now.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Nov 21, 2018

Rebased.

@promag promag force-pushed the promag:2018-05-walletnotify branch to 71d7063 Nov 21, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 21, 2018

@@ -1158,6 +1160,68 @@ fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
}
#endif

#ifdef WIN32
std::wstring ArgvQuote(const std::wstring& arg)

This comment has been minimized.

@luke-jr

luke-jr Nov 25, 2018

Member

This appears to be copied out of this MSDN article, at least some derivative thereof. There's no indication that it is licensed for free use. Please clarify the licensing terms (and move it to a separate commit with proper attribution to the true author), or rewrite it.

Furthermore, this article mentions that the quoting done herein is not sufficient if the commandline passes through cmd.exe, which it probably does in our case. Someone should test to confirm whether ^ works (if it does, we need it).

This comment has been minimized.

@promag

promag Nov 26, 2018

Author Member

Yes it's borrowed from there. I'll update accordingly.

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 26, 2018

Member

Rewriting it doesn't make the creators authorship disappear, just like translations don't make the copyright disappear. Though, very straightforward algorithms can't be licensed due to their simplicity. I suggest we at least put a link and author in a comment inside this function.

This comment has been minimized.

@luke-jr

luke-jr Nov 28, 2018

Member

"Rewrite" does not mean "obfuscate".

Again, AFAICT, this code is not licensed freely. (I don't think there's a strong argument that it is too simple for copyright.)

This comment has been minimized.

@ryanofsky

ryanofsky Jan 16, 2019

Contributor

re: #13339 (comment)

I don't think we should use this code. As Luke points out it's unlicensed, and it doesn't handle cmd.exe escaping, so it will break if the argument contains quotes.

But the problem is actually worse than that. There is no reliable way to escape argv values on windows because the windows OS doesn't have a concept of argument arrays. When you create a process on windows, you only pass a single command line string, not an array of strings like on unix. Parsing of the command line string into C main() arguments can happen different ways. The MSDN article describes one way implemented by the visual c++ runtime, but other runtimes like cygwin may parse arguments differently. And while cmd.exe will normally interpret the command, this only happens because COMSPEC is set by default to cmd.exe (it could also be unset or set to something like bash or powershell).

I agree with @laanwj in #13339 (comment) that using base64 is here probably overkill, but I don't think using evhttp_uriencode on windows like bitcoin-cli does would be a bad option:

char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);

But you will also have to replace % with ^% for cmd.exe. And for non-windows builds it would be safest to stick with your current ShellEscape (evhttp_uriencode won't escape ~).

@@ -73,6 +73,8 @@
#include <malloc.h>
#endif

#include <boost/algorithm/string/replace.hpp>

This comment has been minimized.

@practicalswift

practicalswift Nov 26, 2018

Member

Consider adding a project local ReplaceAll(…) instead of bringing in boost::replace_all.

This comment has been minimized.

@luke-jr

luke-jr Jan 10, 2019

Member

Using dependencies is better than rewriting them.

This comment has been minimized.

@practicalswift

practicalswift Jan 10, 2019

Member

@luke-jr Sometimes, but not always.

This is new code and we're mostly using our own string handling helpers already.

Personally I think it is a mistake to add new code depending on Boost: I want to see declining Boost usage without unnecessary increases like this one.

This comment has been minimized.

@promag

promag Jan 10, 2019

Author Member

@practicalswift boost::replace_all is already a dependency.

Beside that, I'm considering EncodeBase64 the wallet name/path. WDYT?

This comment has been minimized.

@laanwj

laanwj Jan 16, 2019

Member

replace_all is already used in four places. I'd suggest to simply use it here, too.
Rewriting it could always be done, it's irrelevant to his PR.

Edit: Regarding EncodeBase64, no I don't think that's useful here. It'd add another hoop to client scripts to decode it again, adding complexity and potential bugs. It should be possible to quote correctly. It's not trivail but also we're not the first software to do this.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 21, 2019

Needs rebase
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 6, 2019

What is the status here? This is still tagged for 0.18.

@laanwj laanwj removed this from the 0.18.0 milestone Feb 6, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Feb 6, 2019

What is the status here? This is still tagged for 0.18.

I don't think this this can merged currently due to concerns in #13339 (comment).

I think the current shell escaping in this PR looks good when WIN32 isn't defined. But the windows shell escaping is cumbersome and probably broken and uses unlicensed, undocumented, and uncredited(!) code. I think encoding is the better way to go on windows, and good options would be

  1. URI encoding with evhttp_uriencode and ^% replacement for cmd.exe. Example: My Wallet! as My^%20Wallet^%21.
  2. C encoding. Example: My Wallet! as My\x20Wallet\x21
  3. Base64 encoding. My Wallet! as TXkgV2FsbGV0IQ==

Again on UNIX I think it would be most natural to stick with plain shell escaping and not require any decoding on the receiving end.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Feb 6, 2019

If boost process was an option I would expose an env var with the plain wallet name, after all %w is a bit awkward.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 6, 2019

I think encoding is the better way to go on windows,

Ugh, let's not have different interfaces for *nix vs Windows please...

Considering how Windows works (application just gets a long string and parses it how it likes), maybe we should just use *nix quoting and make the target application deal with it?

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Feb 6, 2019

maybe we should just use *nix quoting and make the target application deal with it?

This won't work for two reasons:

  1. cmd.exe is script interpreter that uses special characters. The target application will have no way of accessing the original string if cmd.exe has interpreted and executed it.
  2. In many scripting environments it is not straightforward to get access to the command line string even if it hasn't been mangled by the COMSPEC interpreter. You need to bypass the argv parsing and call the GetCommandLine API, which is easy to do from a C program, but awkward to do from other languages. In python you either need to use ctypes, or install win32 packages which aren't bundled with python. I think the situation is similar with other languages like ruby, java, javascript. By contrast, it is trivial to add: if sys.platform == "win32": wallet = urllib.unquote(wallet) in python if you need your hook script to have windows compatibility.

If boost process was an option I would expose an env var with the plain wallet name, after all %w is a bit awkward.

This is a nice idea, and you don't need boost to implement it since you can rely on the script interpreter.

It would make Luke happy I think because it would avoid creating any differences between platforms on the receiving end and avoid the need for decoding on the receiving end on windows. It would require adding some tricky code that hasn't been written yet to escape for cmd.exe, but basically you could say:

#ifdef WIN32
runCommand(strprintf("SET BITCOIN_WALLET=%s & %s", CmdExeEscape(wallet_name), script_command));
#else
runCommand(strprintf("BITCOIN_WALLET=%s %s", ShellEscape(wallet_name), script_command));
#endif

I might slightly prefer one of my previous suggestions from #13339 (comment) over this just avoid having a CmdExeEscape function in bitcoin. But I think at this point there are number of reasonable options to choose from.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 7, 2019

Passing the environment on the command line would need to be escaped too.

What's wrong with putenv?

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Feb 7, 2019

It's not thread safe, we would have to serialize command executions.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Feb 7, 2019

Passing the environment on the command line would need to be escaped too.

It would only need to be escaped for cmd.exe interpreter, not for the argv parsing that happens in msvcrt. Escaping only for cmd.exe is simpler because msvcrt escaping is crazy by comparison and can also confuse cmd.exe (see the msdn article or the cmd.exe+msvcrt escaping code I wrote years ago). Also IMO, it is much more reasonable to require COMSPEC=cmd.exe than to require client program to use a particular C runtime.

To summarize alternatives for escaping:

  1. Pass as shell escaped argv, implementing cmd.exe + msvcrt escaping on windows. This requires ugliest, most fragile escaping and will be broken with a different comspec, or with a different c library.
  2. Pass as shell escaped environment variable, #13339 (comment). This requires simpler escaping, just prefixing special characters with ^, and will be broken with a different comspec but not a different c library.
  3. Pass as url-encoded (%21) or c-string escaped (\x21) or base64 encoded argument, #13339 (comment). This is probably the simplest thing for bitcoin to implement, but would a put small burden on client programs, which would have to call urllib.unquote or a similar function, on windows only.

My preference would probably be 3, then 2, then 1. And of course it is possible to avoid escaping entirely if you are willing to call CreateProcess directly, or use a library which does this like boost. I think all of these options have some drawbacks, but they mostly aren't terrible and it's not like you don't have options to pick from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.