Page MenuHomePhabricator

Fix user agent sanitizer
ClosedPublic

Authored by Fabien on Jan 9 2019, 10:55.

Details

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 4466
Build 6996: Bitcoin ABC Buildbot (legacy)
Build 6995: arc lint + arc unit

Event Timeline

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 ↗(On Diff #6566)

I'd like to move away from using GlobalConfig in tests unless absolutely necessary. Please use DummyConfig.

217 ↗(On Diff #6566)

This line should be unnecessary.

This revision now requires changes to proceed.Jan 9 2019, 17:23
src/test/net_tests.cpp
217 ↗(On Diff #6566)

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

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
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

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.

This revision now requires changes to proceed.Jan 10 2019, 14:34

Rebase on top of D2290 and change the logic according to feedback

Fabien edited the test plan for this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)
deadalnix added inline comments.
src/test/net_tests.cpp
61 ↗(On Diff #6605)

NetTestConfig ?

jasonbcox requested changes to this revision.Jan 14 2019, 20:26
jasonbcox added inline comments.
src/test/net_tests.cpp
61 ↗(On Diff #6605)

+1 on NetTestConfig

This revision now requires changes to proceed.Jan 14 2019, 20:26

DummyConfigNetTest => NetTestConfig

This revision is now accepted and ready to land.Jan 15 2019, 19:20
This revision was automatically updated to reflect the committed changes.