Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGINGdb4aa8ecde0a: Fix user agent sanitizer
rABCdb4aa8ecde0a: Fix user agent sanitizer

Summary

The current user agent sanitizer behavior is to drop invalid characters
in the comment section and truncate its length if necessary, along with
putting warnings in log.
Note that due to a bug, invalid chars are actually not dropped from the
comment section despite the log states it.

The current behavior is not really user friend, and the bug could lead
to BIP0014 violation.

This diff moves the checks at init time, to allow for the node startup
to fail with an error message if the requirements on the user agent are
not met. This is actually the same behavior as bitcoin core.

Depends on D2290

Test Plan

make check
./test/functional/test_runner.py

./src/bitcoind -uacomment=tara:tata, check it returns an error message
at startup and fails to start

./src/qt/bitcoin-qt -uacomment=tara:tata, check it displays an error message in
a popup then exits when the OK button is clicked

./src/qt/bitcoin-qt -uacomment=tara:tata, go to the debug window and check the
user agent matches /Bitcoin ABC:0.18.6(EB32.0; taratata)/

./src/bitcoind -uacomment=taratata
./src/bticoin-cli getnetworkinfo

Check the "subversion" field matches `/Bitcoin ABC:0.18.6(EB32.0;
taratata)/`

echo "uacomment=taratata" > uacomment.conf
./src/bitcoind -conf=$PWD/uacomment.conf
./src/bticoin-cli getnetworkinfo

Check the "subversion" field matches `/Bitcoin ABC:0.18.6(EB32.0;
taratata)/`

echo "uacomment=tara:tata" > uacomment.conf
./src/bitcoind -conf=$PWD/uacomment.conf

Check it returns an error message at startup and fails to start, then
rm uacomment.conf

Diff Detail

Repository

rABC Bitcoin ABC

Branch

user_agent_sanitize

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4452
Build 6968: Bitcoin ABC Buildbot (legacy)
Build 6967: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 9 2019, 10:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 9 2019, 10:55

Herald added a reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4452: Diff 6566.Jan 9 2019, 11:06

Fabien added a child revision: D2276: [tests] Add uacomment tests.Jan 9 2019, 11:09

jasonbcox requested changes to this revision.Jan 9 2019, 17:23

jasonbcox added a subscriber: jasonbcox.

jasonbcox added inline comments.

src/test/net_tests.cpp
215	I'd like to move away from using GlobalConfig in tests unless absolutely necessary. Please use DummyConfig.
217	This line should be unnecessary.

This revision now requires changes to proceed.Jan 9 2019, 17:23

Fabien added inline comments.Jan 9 2019, 17:50

src/test/net_tests.cpp
217	At the moment you're right, but this will prevent failing the test if the default block size gets increased, so I'd better leave it

Avoid using GlobalConfig in tests

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

deadalnix added inline comments.Jan 10 2019, 12:28

src/test/net_tests.cpp
62 ↗	(On Diff #6583)	You don't need to postfix the name, boost tests are namespaced
246 ↗	(On Diff #6583)	What is so bad about it that we want to remove these characters ?

deadalnix requested changes to this revision.Jan 10 2019, 12:29

deadalnix added inline comments.

src/util.cpp
332 ↗	(On Diff #6583)	This in itself would deserve a test.

This revision now requires changes to proceed.Jan 10 2019, 12:29

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4465: Diff 6583.Jan 10 2019, 12:32

Fabien requested review of this revision.Jan 10 2019, 13:29

Fabien added inline comments.

src/test/net_tests.cpp
62 ↗	(On Diff #6583)	DummyConfig already exists
246 ↗	(On Diff #6583)	'/', ':', '(', ')' are forbidden by bip0014, the other chars are historically restricted to avoid any remote attack using their interpretation by various protocols. Here is the original comment about this restriction: /** * safeChars chosen to allow simple messages/URLs/email addresses, but avoid anything * even possibly remotely dangerous like & or > */
src/util.cpp
332 ↗	(On Diff #6583)	As long as this is clearly stated (see the comment in util.h) that this function is for testing purpose only, I think a test is overkill. If you still want a test, I need to add one for all the other functions that are defined for testing purpose, and would better do this in other diffs.

Update the sanitizer test to cover all the chars forbidden by bip0014

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4466: Diff 6584.Jan 10 2019, 13:45

deadalnix added inline comments.Jan 10 2019, 14:15

src/test/net_tests.cpp
246 ↗	(On Diff #6583)	It still doesn't really explains what the problem is.

deadalnix requested changes to this revision.Jan 10 2019, 14:34

deadalnix added inline comments.

src/test/net_tests.cpp
246 ↗	(On Diff #6583)	If the use of these character is simply discouraged, then I don't see a reason to do anything else than warn. If the use of these character is a serious issue, then it'd be great to know what that issue is and mebe report it to the user instead of changing the comment behind his back. If the use of these character is a security concern, then it would be great to address that concern to begin with, as it seems pretty serious and this isn't cutting it.
src/util.cpp
332 ↗	(On Diff #6583)	I don't see any reason to not test this. This provides a contract for the API. If this ends up being too large for this diff, then it can be done it its own diff + test. The test case for that function should not be very long, and it will protect us when doing refactoring going forward.