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
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels