Escape rather than remove any printable characters in UAs #10731

luke-jr commented at 9:57 AM on July 3, 2017: member

Current Core strips out the !, + and = characters used by the UASF client and Knots to indicate whether BIP148 enforcement is enabled. This expands the allowed characters to all printable ASCII characters for the GUI, and escapes the disallowed-from-log ones in %XX format when printing to the log.

jgarzik commented at 10:03 AM on July 3, 2017: contributor

It seems risky to put quote chars in there.

luke-jr commented at 10:18 AM on July 3, 2017: member

Hmm, you mean in case someone is reading the log and inserting it into a SQL database or something?

jgarzik commented at 10:25 AM on July 3, 2017: contributor

@luke-jr Yep. Or it makes its way to the command line, and someone is lazy and fails to quote. We've seen this movie before :)

jonasschnelli added the label P2P on Jul 3, 2017

luke-jr force-pushed on Jul 3, 2017

luke-jr renamed this:
~~net_processing: Avoid filtering any printable characters from UAs in the log~~
SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~"
on Jul 3, 2017

luke-jr commented at 9:52 PM on July 3, 2017: member

Reduced the subset to avoid quotes and other possibly dangerous characters, and just made it the default (which is only used for logging).

gmaxwell commented at 7:12 AM on July 4, 2017: contributor

NAK.

! can divert shell processing (past event references), % and & can divert URIs and HTML contexts (by constructing other prohibited characters).

Do we really need to do things like this? Reviewing these sorts of things take time an effort that could better be spent elsewhere.

luke-jr commented at 7:49 AM on July 4, 2017: member

I can remove % and & (although IMO this is a bit too much nannying), but ! is needed to properly log existing UAs...

MarcoFalke commented at 8:23 AM on July 4, 2017: member

Concept NACK per @gmaxwell. The existing safe chars should be enough to generate any ua comment that might render useful.

luke-jr commented at 8:30 AM on July 4, 2017: member

@MarcoFalke I'm not talking about a speculative scenario. UAs using !, +, and = are already live on the network. This only fixes the display and logging of these.

And the existing safe chars uses the same limits for both -uacomment options as well as the log filtering. That particular combination makes it impossible to have code-only UA comments, hence why the usage of !, +, and = were necessary.

luke-jr commented at 8:50 AM on July 4, 2017: member

(Also, the super-nanny approach of not allowing any possible "needs escaping" characters already sailed a long time ago: ; is one of the most dangerous such characters, and has been allowed from the start.)

promag commented at 9:53 AM on July 4, 2017: member

; is one of the most dangerous such characters, and has been allowed from the start.) @luke-jr that doesn't sound a good argument :smile:.

laanwj commented at 6:00 PM on July 6, 2017: member

Do we really need to do things like this? Reviewing these sorts of things take time an effort that could better be spent elsewhere.

I agree, I'd rather not make this change.

luke-jr commented at 8:39 PM on July 6, 2017: member

@laanwj You yourself added shell characters in #4983 ...

How do we get this fixed? Would it help to reduce it to just !, +, and = so it only addresses the real-world issue?

gmaxwell commented at 10:43 PM on July 6, 2017: contributor

! is shell problematic

jonasschnelli commented at 7:27 AM on July 13, 2017: contributor

Closing because seems not something we should do.

jonasschnelli closed this on Jul 13, 2017

luke-jr commented at 7:35 AM on July 13, 2017: member

Please reopen. This is a bug fix for a present issue, for which no alternate solutions have been proposed.

We already use/allow "shell-problematic" characters (such as parenthesis and semicolon), and worrying about them in log files is pretty absurd anyway. My bitcoin logs already have ! from other sources anyway.

jonasschnelli commented at 7:46 AM on July 13, 2017: contributor

But @luke-jr: your changing the default charset for the general function SanitizeString() where you break the assumption that this function produces a sanitized/safe string? If you wan't to fix a bug, I think you should find a ways that doesn't break that – reasonable – assumption. Example: there are open pull requests who want to use SanitizeString for user feedback (via CLI, RPC). Not sure if this is a good idea or not but it clearly shows that SanitizeString() should include shell/pipe safety.

jonasschnelli commented at 7:46 AM on July 13, 2017: contributor

reopening

jonasschnelli reopened this on Jul 13, 2017

net: Drop CNode::strSubVer since it isn't used or useful 903ac8d37c

luke-jr renamed this:
~~SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~"~~
Escape rather than remove any printable characters in UAs
on Jul 13, 2017

luke-jr force-pushed on Jul 13, 2017

luke-jr commented at 8:29 AM on July 13, 2017: member

Rewrote this to only allow the full set of printable characters in the GUI where it should be harmless, and to escape them in %XX format when printing to the log.

in src/utilstrencodings.cpp:33 in 9439269a26 outdated

  30 |      {
  31 | -        if (SAFE_CHARS[rule].find(str[i]) != std::string::npos)
  32 | +        if (SAFE_CHARS[rule].find(str[i]) != std::string::npos) {
  33 |              strResult.push_back(str[i]);
  34 | +        } else if (escape) {
  35 | +            strResult += strprintf("%02x", str[i]);

ryanofsky commented at 10:41 AM on July 13, 2017:

This isn't really escaping, because the output is ambiguous (there no way to detect escape sequences and recover the original string). Probably would be better to change to something like strprintf("%%%02X", str[i]) or strprintf("\\x%02X", str[i]) to use a more standard url-style or c-style escape format.

Also, if you do url-style or c-style escaping you should make sure the % or \ escape character is itself escaped by tweaking the SAFE_CHARS.find condition or just not including the character in SAFE_CHAR lists.

luke-jr commented at 10:44 AM on July 13, 2017:

Indeed, it was supposed to be %%%02x

Fixed

in src/utilstrencodings.cpp:33 in 7d88296eca outdated

  30 |      {
  31 | -        if (SAFE_CHARS[rule].find(str[i]) != std::string::npos)
  32 | +        if (SAFE_CHARS[rule].find(str[i]) != std::string::npos) {
  33 |              strResult.push_back(str[i]);
  34 | +        } else if (escape) {
  35 | +            strResult += strprintf("%%%02x", str[i]);

ryanofsky commented at 10:53 AM on July 13, 2017:

You will also want to remove the % character from SAFE_CHARS_PRINTABLE so it will be escaped itself, or alternately add a && (!escape || str[i] != '%') clause to the find check above.

ryanofsky commented at 10:56 AM on July 13, 2017:

Maybe use %02X instead of %02x since it helps escape sequences stand out a little more, and is the more common way you see percent encoding done.

luke-jr commented at 6:34 AM on July 17, 2017:

Done

TheBlueMatt commented at 7:31 PM on July 14, 2017: member

Can we instead remove exposing of subver entirely? I'm really tired of people "voting" by spinning up sybil attacks, its just not in any way useful.

luke-jr commented at 6:26 AM on July 17, 2017: member

@TheBlueMatt Whether we do or don't, it's outside the scope of this PR.

luke-jr force-pushed on Jul 26, 2017

SanitizeString: Support optional escaping of disallowed characters 1e588b4df2

net_processing: Escape rather than remove any printable characters in UAs 69577aec48

SanitizeString: If escaping, ensure '%' itself is always escaped c1ff31f0e2

luke-jr force-pushed on Jul 27, 2017

luke-jr commented at 4:51 AM on July 27, 2017: member

@ryanofsky (addressed your nits btw)

laanwj added the label Docs and Output on Sep 6, 2017

MarcoFalke commented at 3:58 PM on September 7, 2017: member

There seems to be no conceptual acknowledgment to do this. Closing for now.

MarcoFalke closed this on Sep 7, 2017

DrahtBot locked this on Sep 8, 2021