Page MenuHomePhabricator

Fix user agent sanitizer
ClosedPublic

Authored by Fabien on Wed, Jan 9, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Wed, Jan 9, 10:55
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Jan 9, 10:55
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Wed, Jan 9, 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.Wed, Jan 9, 17:23
Fabien added inline comments.Wed, Jan 9, 17:50
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

Fabien updated this revision to Diff 6583.Thu, Jan 10, 12:20

Avoid using GlobalConfig in tests

deadalnix added inline comments.Thu, Jan 10, 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.Thu, Jan 10, 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.Thu, Jan 10, 12:29
Fabien requested review of this revision.Thu, Jan 10, 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.

Fabien updated this revision to Diff 6584.Thu, Jan 10, 13:34

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

deadalnix added inline comments.Thu, Jan 10, 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.Thu, Jan 10, 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.Thu, Jan 10, 14:34
Fabien updated this revision to Diff 6605.Fri, Jan 11, 14:59

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

Fabien edited the summary of this revision. (Show Details)Fri, Jan 11, 14:59
Fabien edited the test plan for this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)
deadalnix accepted this revision.Fri, Jan 11, 17:35
deadalnix added inline comments.
src/test/net_tests.cpp
61 ↗(On Diff #6605)

NetTestConfig ?

jasonbcox requested changes to this revision.Mon, Jan 14, 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.Mon, Jan 14, 20:26
Fabien updated this revision to Diff 6662.Tue, Jan 15, 07:19

DummyConfigNetTest => NetTestConfig

deadalnix accepted this revision.Tue, Jan 15, 12:53
jasonbcox accepted this revision.Tue, Jan 15, 19:20
This revision is now accepted and ready to land.Tue, Jan 15, 19:20
This revision was automatically updated to reflect the committed changes.