New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape rather than remove any printable characters in UAs #10731

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@luke-jr
Member

luke-jr commented Jul 3, 2017

Current Core strips out the !, + and = characters used by the UASF client and Knots to indicate whether BIP148 enforcement is enabled. This expands the allowed characters to all printable ASCII characters for the GUI, and escapes the disallowed-from-log ones in %XX format when printing to the log.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 3, 2017

Contributor

It seems risky to put quote chars in there.

Contributor

jgarzik commented Jul 3, 2017

It seems risky to put quote chars in there.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 3, 2017

Member

Hmm, you mean in case someone is reading the log and inserting it into a SQL database or something?

Member

luke-jr commented Jul 3, 2017

Hmm, you mean in case someone is reading the log and inserting it into a SQL database or something?

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 3, 2017

Contributor

@luke-jr Yep. Or it makes its way to the command line, and someone is lazy and fails to quote. We've seen this movie before :)

Contributor

jgarzik commented Jul 3, 2017

@luke-jr Yep. Or it makes its way to the command line, and someone is lazy and fails to quote. We've seen this movie before :)

@jonasschnelli jonasschnelli added the P2P label Jul 3, 2017

@luke-jr luke-jr changed the title from net_processing: Avoid filtering any printable characters from UAs in the log to SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~" Jul 3, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 3, 2017

Member

Reduced the subset to avoid quotes and other possibly dangerous characters, and just made it the default (which is only used for logging).

Member

luke-jr commented Jul 3, 2017

Reduced the subset to avoid quotes and other possibly dangerous characters, and just made it the default (which is only used for logging).

@gmaxwell

NAK.

! can divert shell processing (past event references), % and & can divert URIs and HTML contexts (by constructing other prohibited characters).

Do we really need to do things like this? Reviewing these sorts of things take time an effort that could better be spent elsewhere.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 4, 2017

Member

I can remove % and & (although IMO this is a bit too much nannying), but ! is needed to properly log existing UAs...

Member

luke-jr commented Jul 4, 2017

I can remove % and & (although IMO this is a bit too much nannying), but ! is needed to properly log existing UAs...

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 4, 2017

Member

Concept NACK per @gmaxwell. The existing safe chars should be enough to generate any ua comment that might render useful.

Member

MarcoFalke commented Jul 4, 2017

Concept NACK per @gmaxwell. The existing safe chars should be enough to generate any ua comment that might render useful.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 4, 2017

Member

@MarcoFalke I'm not talking about a speculative scenario. UAs using !, +, and = are already live on the network. This only fixes the display and logging of these.

And the existing safe chars uses the same limits for both -uacomment options as well as the log filtering. That particular combination makes it impossible to have code-only UA comments, hence why the usage of !, +, and = were necessary.

Member

luke-jr commented Jul 4, 2017

@MarcoFalke I'm not talking about a speculative scenario. UAs using !, +, and = are already live on the network. This only fixes the display and logging of these.

And the existing safe chars uses the same limits for both -uacomment options as well as the log filtering. That particular combination makes it impossible to have code-only UA comments, hence why the usage of !, +, and = were necessary.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 4, 2017

Member

(Also, the super-nanny approach of not allowing any possible "needs escaping" characters already sailed a long time ago: ; is one of the most dangerous such characters, and has been allowed from the start.)

Member

luke-jr commented Jul 4, 2017

(Also, the super-nanny approach of not allowing any possible "needs escaping" characters already sailed a long time ago: ; is one of the most dangerous such characters, and has been allowed from the start.)

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Jul 4, 2017

Member

; is one of the most dangerous such characters, and has been allowed from the start.)

@luke-jr that doesn't sound a good argument 😄.

Member

promag commented Jul 4, 2017

; is one of the most dangerous such characters, and has been allowed from the start.)

@luke-jr that doesn't sound a good argument 😄.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 6, 2017

Member

Do we really need to do things like this? Reviewing these sorts of things take time an effort that could better be spent elsewhere.

I agree, I'd rather not make this change.

Member

laanwj commented Jul 6, 2017

Do we really need to do things like this? Reviewing these sorts of things take time an effort that could better be spent elsewhere.

I agree, I'd rather not make this change.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 6, 2017

Member

@laanwj You yourself added shell characters in #4983 ...

How do we get this fixed? Would it help to reduce it to just !, +, and = so it only addresses the real-world issue?

Member

luke-jr commented Jul 6, 2017

@laanwj You yourself added shell characters in #4983 ...

How do we get this fixed? Would it help to reduce it to just !, +, and = so it only addresses the real-world issue?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 6, 2017

Member

! is shell problematic

Member

gmaxwell commented Jul 6, 2017

! is shell problematic

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 13, 2017

Member

Closing because seems not something we should do.

Member

jonasschnelli commented Jul 13, 2017

Closing because seems not something we should do.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 13, 2017

Member

Please reopen. This is a bug fix for a present issue, for which no alternate solutions have been proposed.

We already use/allow "shell-problematic" characters (such as parenthesis and semicolon), and worrying about them in log files is pretty absurd anyway. My bitcoin logs already have ! from other sources anyway.

Member

luke-jr commented Jul 13, 2017

Please reopen. This is a bug fix for a present issue, for which no alternate solutions have been proposed.

We already use/allow "shell-problematic" characters (such as parenthesis and semicolon), and worrying about them in log files is pretty absurd anyway. My bitcoin logs already have ! from other sources anyway.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 13, 2017

Member

But @luke-jr: your changing the default charset for the general function SanitizeString() where you break the assumption that this function produces a sanitized/safe string?
If you wan't to fix a bug, I think you should find a ways that doesn't break that – reasonable – assumption.
Example: there are open pull requests who want to use SanitizeString for user feedback (via CLI, RPC). Not sure if this is a good idea or not but it clearly shows that SanitizeString() should include shell/pipe safety.

Member

jonasschnelli commented Jul 13, 2017

But @luke-jr: your changing the default charset for the general function SanitizeString() where you break the assumption that this function produces a sanitized/safe string?
If you wan't to fix a bug, I think you should find a ways that doesn't break that – reasonable – assumption.
Example: there are open pull requests who want to use SanitizeString for user feedback (via CLI, RPC). Not sure if this is a good idea or not but it clearly shows that SanitizeString() should include shell/pipe safety.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 13, 2017

Member

reopening

Member

jonasschnelli commented Jul 13, 2017

reopening

@jonasschnelli jonasschnelli reopened this Jul 13, 2017

@luke-jr luke-jr changed the title from SanitizeString: Expand upon allowed characters in logging to include "!#%&*+=^{}~" to Escape rather than remove any printable characters in UAs Jul 13, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 13, 2017

Member

Rewrote this to only allow the full set of printable characters in the GUI where it should be harmless, and to escape them in %XX format when printing to the log.

Member

luke-jr commented Jul 13, 2017

Rewrote this to only allow the full set of printable characters in the GUI where it should be harmless, and to escape them in %XX format when printing to the log.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

Can we instead remove exposing of subver entirely? I'm really tired of people "voting" by spinning up sybil attacks, its just not in any way useful.

Contributor

TheBlueMatt commented Jul 14, 2017

Can we instead remove exposing of subver entirely? I'm really tired of people "voting" by spinning up sybil attacks, its just not in any way useful.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 17, 2017

Member

@TheBlueMatt Whether we do or don't, it's outside the scope of this PR.

Member

luke-jr commented Jul 17, 2017

@TheBlueMatt Whether we do or don't, it's outside the scope of this PR.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 27, 2017

Member

@ryanofsky (addressed your nits btw)

Member

luke-jr commented Jul 27, 2017

@ryanofsky (addressed your nits btw)

@laanwj laanwj added the Docs label Sep 6, 2017

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 7, 2017

Member

There seems to be no conceptual acknowledgment to do this. Closing for now.

Member

MarcoFalke commented Sep 7, 2017

There seems to be no conceptual acknowledgment to do this. Closing for now.

@MarcoFalke MarcoFalke closed this Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment