Sanitize uacomment #6647

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:MarcoFalke-2015-uacommentFix changing 4 files +36 −12

MarcoFalke commented at 10:55 AM on September 7, 2015: member
- SanitizeString() can be requested to be more strict
- Actually apply SanitizeString() to uacomments
MarcoFalke cross-referenced this on Sep 7, 2015 from issue Handle reserved symbols in uacomment (BIP-0014) by MarcoFalke
in src/utilstrencodings.h:None in 63d8457f97 outdated
```
  21 | @@ -22,7 +22,15 @@
  22 |  /** This is needed because the foreach macro can't get over the comma in pair<t1, t2> */
  23 |  #define PAIRTYPE(t1, t2)    std::pair<t1, t2>
  24 |  
  25 | -std::string SanitizeString(const std::string& str);
  26 | +static const int SAFE_CHARS_DEFAULT = 0; //!< Default rule in SanitizeString()
```
jonasschnelli commented at 11:50 AM on September 7, 2015:

nit: would a enum not be more adequate for a such operation?

laanwj commented at 2:32 PM on September 8, 2015:

+1 for using enum. This is a perfect fit for one.

Not sure passing in the characters in a string is better. The idea makes sense, but having it like this keeps open the possibility of optimizing SanitizeString to use something else than string.find() on every character.
in src/utilstrencodings.cpp:None in 63d8457f97 outdated
```
  25 | -    string strResult;
  26 | +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890 .,;_/:?@()", // SAFE_CHARS_DEFAULT
  27 | +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890 .,;_?@" // SAFE_CHARS_UACOMMENT
  28 | +};
  29 | +
  30 | +string SanitizeString(const string& str, int rule)
```
jonasschnelli commented at 11:54 AM on September 7, 2015:

Instead of the slightly static rule parameter, would a charset represented as string with additional forbidden chars not be more flexible. Instead SanitizeString("Comment2 .,_?@", SAFE_CHARS_UACOMMENT) it would then be SanitizeString("Comment2 .,_?@", "/:()"). Not sure if we wan't to have use case specific handling within SanitizeString().
jonasschnelli commented at 11:55 AM on September 7, 2015: contributor

Concept ACK.

in src/init.cpp:None in 63d8457f97 outdated

1013 | @@ -1014,8 +1014,11 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1014 |  
1015 |      RegisterNodeSignals(GetNodeSignals());
1016 |  
1017 | -    // format user agent, check total size
1018 | -    strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, mapMultiArgs.count("-uacomment") ? mapMultiArgs["-uacomment"] : std::vector<string>());
1019 | +    // sanitize comments per BIP-0014, format user agent and check total size
1020 | +    std::vector<string> uacomments;
1021 | +    BOOST_FOREACH(string cmt, mapMultiArgs["-uacomment"])

laanwj commented at 2:34 PM on September 8, 2015:

Alternative: error out if SanitizeString(x) != x, instead of silently dropping characters

MarcoFalke commented at 11:09 PM on September 10, 2015:

Made it to return error~~, but I am thinking about using warnings instead for the whole uacomment thing.~~

laanwj added the label P2P on Sep 8, 2015
MarcoFalke force-pushed on Sep 9, 2015
MarcoFalke force-pushed on Sep 9, 2015
MarcoFalke commented at 1:49 PM on September 9, 2015: member

Force pushed changes requested in the comments.

[uacomment] Sanitize per BIP-0014

* SanitizeString() can be requested to be more strict
* Throw error when SanitizeString() changes uacomments
* Fix tests

1c1b1b315f

MarcoFalke force-pushed on Sep 16, 2015
laanwj merged this on Sep 22, 2015
laanwj closed this on Sep 22, 2015
laanwj referenced this in commit a3babc826d on Sep 22, 2015
laanwj commented at 9:37 AM on September 22, 2015: member

Tested ACK
MarcoFalke deleted the branch on Sep 22, 2015
str4d cross-referenced this on May 18, 2017 from issue mapArgs is modified by setgenerate RPC without locking by daira
str4d cross-referenced this on May 19, 2017 from issue Misc upstream PRs by str4d
str4d cross-referenced this on Jul 14, 2017 from issue Bitcoin 0.12 P2P/Net PRs 1 by str4d
str4d cross-referenced this on Dec 19, 2017 from issue Implement uacomment config parameter by str4d
zkbot referenced this in commit f1aeaec471 on Mar 21, 2018
zkbot referenced this in commit c97906e265 on Apr 13, 2018
MarcoFalke cross-referenced this on Jun 7, 2021 from issue fuzz harness for SanitizeString and x-reference fixes by baptistapedro
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6634 Handle reserved symbols in uacomment (BIP-0014)#6655 Issues in master after libevent merge #22171 fuzz harness for SanitizeString and x-reference fixes