Page MenuHomePhabricator

[net] Ignore unlikely timestamps in version messages
ClosedPublic

Authored by jasonbcox on May 11 2020, 02:52.

Details

Summary

This avoids undefined behavior and prevents timejacking attacks
that use negative timestamps. The genesis block time was chosen since
it's an easy timestamp to pick as a lower bound that we safely assume
all nodes will be greater than. Any timestamp lower than that is not
expected.

This should be safe to deploy without a flag day activation since
substantial differences in nTime between nodes should only occur if
mocktime was explicitly set. This is not expected (especially on
mainnet) for any currently in-consensus BCH nodes.

This should be safe for functional tests as well, since we typically
only mock time from the genesis time to well in the future (regtest's
genesis time is also greater than mainnet genesis block time).

Depends on D6023 so that tests will pass using timestamp values +/- 2^31
Depends on D6022 so that we can assume GetTime() will not return a negative
number.

Test Plan
ninja check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Branch
time-offset-underflow
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10694
Build 19185: Default Diff Build & Tests
Build 19184: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.May 11 2020, 08:00
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/net_processing.cpp
2291 ↗(On Diff #19878)

You can get the value from the chainparams genesis

2299 ↗(On Diff #19878)

Should the peer get banned if such a value is sent ?

This revision now requires changes to proceed.May 11 2020, 08:00
deadalnix requested changes to this revision.May 11 2020, 14:50
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/net_processing.cpp
2291 ↗(On Diff #19878)

And in fact you should.

2299 ↗(On Diff #19878)

Or at least tagged as misbehaving.

src/net_processing.cpp
2299 ↗(On Diff #19878)

I opted not to outright ban since the timestamp is easily ignored without consequence. Marking as misbehaving is fine, though this behavior seems incredibly arbitrary when compared to other misbehaving-but-dont-ban examples in the codebase.

  • Use genesis time from chainparams
  • Use Misbehaving() instead of just logging
deadalnix requested changes to this revision.May 11 2020, 18:00
deadalnix added inline comments.
src/net_processing.cpp
2296

Maybe 20 or something? The ban threshold is 100, so 1 is effectively irrelevant.

This revision now requires changes to proceed.May 11 2020, 18:00

Bump misbehaving factor to 20

Snippet of first build failure:

[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.gettime
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.util_time_GetTime
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_IsDigit
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ParseInt32
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ParseInt64
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ParseUInt32
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ParseUInt64
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ParseDouble
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_FormatParagraph
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_FormatSubVersion
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ParseFixedPoint
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_LockDirectory
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_DirIsWritable
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ConvertBits
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ToLower
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_ToUpper
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_tests.test_Capitalize
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] util_threadnames_tests.util_threadnames_test_rename_threaded
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] validation_block_tests.processnewblock_signals_ordering
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] validation_tests.block_subsidy_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] validation_tests.subsidy_limit_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] validation_tests.validation_load_external_block_file
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] versionbits_tests.versionbits_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] versionbits_tests.versionbits_computeblockversion
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] work_comparator_tests.work_comparator
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] server_tests.server_IsDeprecatedRPCEnabled
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] db_tests.getwalletenv_file
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] db_tests.getwalletenv_directory
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] db_tests.getwalletenv_g_dbenvs_multiple
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] db_tests.getwalletenv_g_dbenvs_free_instance
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] coinselector_tests.bnb_search_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] coinselector_tests.knapsack_solver_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] coinselector_tests.ApproximateBestSubset
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] coinselector_tests.SelectCoins_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_default
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_custom
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_does_not_exist
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_is_not_directory
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_is_not_relative
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_no_trailing
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] init_tests.walletinit_verify_walletdir_no_trailing2
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] psbt_wallet_tests.psbt_updater_test
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] psbt_wallet_tests.parse_hd_keypath
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.scan_for_wallet_transactions
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.importmulti_rescan
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.importwallet_rescan
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.coin_mark_dirty_immature_credit
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.ComputeTimeSmart
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.LoadReceiveRequests
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.ListCoins
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_tests.wallet_disableprivkeys
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] walletdb_tests.write_erase_name
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] walletdb_tests.write_erase_purpose
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] walletdb_tests.write_erase_destdata
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] walletdb_tests.no_dest_fails
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_crypto_tests.passphrase
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_crypto_tests.encrypt
[19:09:06]i:		 [Bitcoin_ABC_unit_tests_with_next_upgrade_activated] wallet_crypto_tests.decrypt
[19:09:10]W:	 [Step 1/2] Process exited with code 1
[19:09:11]E:	 [Step 1/2] Process exited with code 1 (Step: Command Line)

Each failure log is accessible here:
Bitcoin ABC functional tests: abc-version-message.py

src/net_processing.cpp
2296

That was my thought too, but I was also trying to match similar surrounding code. Since a higher number is preferred, I'll bump those too in a separate patch.

Rebase on the right stack so tests pass

This revision is now accepted and ready to land.May 11 2020, 22:45