Add a way to limit deserialized string lengths and use it. #4655

pull sipa wants to merge 1 commits into bitcoin:master from sipa:limitstr changing 4 files +46 −14

sipa commented at 9:01 PM on August 7, 2014: member

No description provided.
sipa renamed this:
~~Add a way to limit deserialized string lengths and use it for subver~~
Add a way to limit deserialized string lengths and use it.
on Aug 7, 2014
Diapolo commented at 5:49 AM on August 8, 2014: none

What about the allowed lengths? Does this need a BIP or a definition anywhere or are they defined already?
laanwj added the label Improvement on Aug 8, 2014
laanwj added the label P2P on Aug 8, 2014
Add a way to limit deserialized string lengths
and use it for most strings being serialized.
216e9a4456

in src/main.cpp:None in 4aae821e90 outdated

4182 | @@ -4183,7 +4183,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4183 |          if (fDebug)
4184 |          {
4185 |              string strMsg; unsigned char ccode; string strReason;
4186 | -            vRecv >> strMsg >> ccode >> strReason;
4187 | +            vRecv >> LIMITED_STRING(strMsg, 12) >> ccode >> LIMITED_STRING(strReason, 111);

laanwj commented at 7:05 AM on August 8, 2014:

s/12/CMessageHeader::COMMAND_SIZE ?

Diapolo commented at 9:55 AM on August 9, 2014: none

@sipa Still my question from above...
BitcoinPullTester commented at 3:48 PM on August 9, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4655_216e9a4456207f5ae9cd85926521851e11a26d92/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
gmaxwell commented at 1:06 AM on August 10, 2014: contributor

While we're in this space, we might want to also make GetWarnings run SanitizeString on its output— there are a couple of different paths for text to end up there (including alerts) and we should probably belt and suspenders that nothing weird comes out. (As a rule we should avoid using strings on the network, and where we do use them always sanitize them both for length and content).
gmaxwell commented at 10:45 PM on August 10, 2014: contributor

ACK. (Also, I've had this running on the network in valgrind for two days without issue)
jgarzik commented at 1:29 AM on August 11, 2014: contributor

ut ACK
acksthispull commented at 7:48 PM on August 11, 2014: none

Tested, ACK.
sipa commented at 12:26 PM on August 17, 2014: member

@laanwj ACK?
laanwj commented at 7:46 AM on August 18, 2014: member

ACK
laanwj merged this on Aug 18, 2014
laanwj closed this on Aug 18, 2014
laanwj referenced this in commit 21e7a5690f on Aug 18, 2014
davecgh cross-referenced this on Aug 19, 2016 from issue Why max user agent length in btcd is different from bitcoin? by sbwdlihao
bitcoin locked this on Sep 8, 2021

Contributors

Labels