Details

Reviewers

Fabien
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING016abbe42e83: [net] Ignore unlikely timestamps in version messages
rABC016abbe42e83: [net] Ignore unlikely timestamps in version messages

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

jasonbcox created this revision.May 11 2020, 02:52

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 11 2020, 02:52

jasonbcox requested review of this revision.May 11 2020, 02:52

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10680: Diff 19878.May 11 2020, 03:02

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.

jasonbcox added inline comments.May 11 2020, 16:48

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

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10694: Diff 19904.May 11 2020, 17:49

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

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

deadalnix accepted this revision.May 11 2020, 19:01

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) failed.

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

Harbormaster completed remote builds in B10699: Diff 19921.May 11 2020, 19:09

jasonbcox added inline comments.May 11 2020, 19:18

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.