Sanitize command strings before logging them. #5770

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:logsan changing 3 files +9 −9

gmaxwell commented at 9:22 AM on February 8, 2015: contributor

Normally bitcoin core does not display any network originated strings without sanitizing or hex encoding. This wasn't done for strcommand in many places.

This could be used to play havoc with a terminal displaying the logs, especially with printtoconsole in use.

Thanks to Evil-Knievel for reporting this issue.
laanwj added the label Bug on Feb 8, 2015
laanwj commented at 10:14 AM on February 8, 2015: member

utACK
jgarzik commented at 10:44 AM on February 8, 2015: contributor

ut ACK
paveljanik commented at 10:45 AM on February 8, 2015: contributor

ACK
fanquake commented at 11:07 AM on February 8, 2015: member

utACK
laanwj commented at 11:11 AM on February 8, 2015: member

There's another one here: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L3453

Edit: and here https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1974 although arguably, it's less important as it only logs outgoing messages.
paveljanik commented at 11:16 AM on February 8, 2015: contributor

Hmm, can't we call sanitize on the command string directly on input instead later on every LogPrint?
jonasschnelli commented at 11:54 AM on February 8, 2015: contributor

Somehow related: #5492
laanwj commented at 1:23 PM on February 8, 2015: member

The problem here is the output, hence the point is to sanitize on output, not the input. Operations on valid input may still result in output strings that are 'dangerous'. Filtering out the characters on input would result in the counter-intuitive behavior that "get\x1bblock"is an alias for "getblock" (for another example of input and output sanitization being confused only look as far as PHP's 'magic quotes' disaster, which has corrupted texts with extra \ and didn't solve the underlying SQL injection at all).

(Prematurely rejecting commands with control or non-ASCII characters would of course be more reasonable, as there are none of those that we handle anyway)

But in any case, we'd like to minimize code impact outside of immediate logging/debugging code. E.g. @gmaxwell and I have talked about changing LogPrintStr to replace with ' ' any control characters except a '\n' at the end of the string. This would solve this problem thoroughly without any potential side-impact on the code.

Edit: and this specific code change should go in as well, it'd just be a last ditch catch-all.

Sanitize command strings before logging them.

Normally bitcoin core does not display any network originated strings without
 sanitizing or hex encoding.  This wasn't done for strcommand in many places.

This could be used to play havoc with a terminal displaying the logs,
 especially with printtoconsole in use.

Thanks to Evil-Knievel for reporting this issue.

28d4cff0ed

gmaxwell commented at 8:01 PM on February 8, 2015: contributor

@laanwj good catch; I saw the one at 1974 but because it was outgoing I didn't hit it. Since it caught your attention to, I'll just add it; better than people in the future spending time thinking about it being okay or not.
laanwj merged this on Feb 9, 2015
laanwj closed this on Feb 9, 2015
laanwj referenced this in commit 32a8b6a9d7 on Feb 9, 2015
gmaxwell referenced this in commit 6b4163b972 on Feb 13, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels