net: Sanitize message type for logging #21872

laanwj commented at 3:35 PM on May 6, 2021: member

Use SanitizeString when logging message errors to make sure that the message type is sanitized. I have checked all logging in net.cpp.
For the MESSAGESTART error don't inspect and log header details at all: receiving invalid start bytes makes it likely that the packet isn't even formatted as valid P2P message. Logging the four unexpected start bytes (as hex) should be enough.
Update p2p_invalid_messages.py test to check this.
Improve error messages in a second commit.

Issue reported by gmaxwell.

net: Sanitize message type for logging

- Use `SanitizeString` when logging message errors to make sure that the
message type is sanitized.

- For the `MESSAGESTART` error don't inspect and log header details at
all: receiving invalid start bytes makes it likely that the packet isn't
even formatted as valid P2P message. Logging the four unexpected start
bytes should be enough.

- Update `p2p_invalid_messages.py` test to check this.

Issue reported by gmaxwell.

955eee7680

laanwj added the label P2P on May 6, 2021

laanwj commented at 3:37 PM on May 6, 2021: member

BTW, is there any specific reason that these errors are SHOUTY? If not, I'm ok with changing that in this PR if people like.

practicalswift commented at 3:41 PM on May 6, 2021: contributor

Concept ACK: attacker controlled input should be sanitized before being written to console or log

laanwj commented at 3:44 PM on May 6, 2021: member

Concept ACK: attacker input should be sanitized before being written to console or log

FWIW: the auto-escaping in the logging catches this so it doesn't become a security issue. Still, sanitizing explicitly is better.

sipa commented at 3:47 PM on May 6, 2021: member

Concept ACK. No reason why this is shouty, IMO.

jonatack commented at 3:57 PM on May 6, 2021: member

ACK 955eee76803c098978cf0bbc7f1f6d3c230544e2

Subawit approved

laanwj commented at 5:50 AM on May 7, 2021: member

Concept ACK. No reason why this is shouty, IMO.

Added a commit that improves (imo) the error messages' formatting and consistency.

practicalswift commented at 6:45 AM on May 7, 2021: contributor

cr ACK 3f0ad69c8bf4b63c6f28753191f540c4722fe8b3: patch looks correct

in src/net.cpp:757 in 3f0ad69c8b outdated

 754 |                   m_node_id);
 755 |          out_err_raw_size = msg->m_raw_message_size;
 756 |          msg = std::nullopt;
 757 |      } else if (!hdr.IsCommandValid()) {
 758 | -        LogPrint(BCLog::NET, "HEADER ERROR - COMMAND (%s, %u bytes), peer=%d\n",
 759 | +        LogPrint(BCLog::NET, "Header error: Invalid command (%s, %u bytes), peer=%d\n",

MarcoFalke commented at 8:40 AM on May 7, 2021:

3f0ad69c8bf4b63c6f28753191f540c4722fe8b3: I think the reason that all this used upper case was that COMMAND was initially picked as log message and to not break grepping the debug log, it was kept this way. Since you are breaking grep COMMAND here, might as well change it to message type to better reflect the meaning?

laanwj commented at 9:27 AM on May 7, 2021:

Agree, 'command' means nothing in a P2P network that doesn't even have ad-hoc hierarchy, will change it to message type.

MarcoFalke approved

MarcoFalke commented at 8:41 AM on May 7, 2021: member

Happy to re-ACK if you address the log message rewording nit.

ACK 3f0ad69c8bf4b63c6f28753191f540c4722fe8b3 🔵

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 270af806f6b761fc0bf9964c17ba689a56cec58d3e1039d4ff453c5cf3588d53 -

</details>

net: Clarify message header validation errors

Make the errors less shouty and more descriptive.

09205b33aa

laanwj force-pushed on May 7, 2021

laanwj commented at 9:35 AM on May 7, 2021: member

Force-pushed 3f0ad69c8bf4b63c6f28753191f540c4722fe8b3..09205b33aa74e385caa2803aa6febc18ad1efa32 to address @MarcoFalke 's comment.

MarcoFalke commented at 9:46 AM on May 7, 2021: member

re-ACK 09205b33aa74e385caa2803aa6febc18ad1efa32 only change is log message fixup 🔂

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash f658aa634cb1d656d9594bbd24ef430ebb676368e8471418799441bcfbcd150d -

</details>

practicalswift commented at 3:39 PM on May 7, 2021: contributor

re-ACK 09205b33aa74e385caa2803aa6febc18ad1efa32

Nice small log message improvement added since last review. Remember: log messages are part of the UI. An often overlooked part :)

MarcoFalke merged this on May 9, 2021

MarcoFalke closed this on May 9, 2021

sidhujag referenced this in commit 898f4a1b44 on May 9, 2021

gwillen referenced this in commit b7ec414b10 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022