Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not connect a corrupted block #1087

Open
yixiao5428 opened this issue Jul 17, 2021 · 0 comments · Fixed by #1126
Open

Do not connect a corrupted block #1087

yixiao5428 opened this issue Jul 17, 2021 · 0 comments · Fixed by #1126

Comments

@yixiao5428
Copy link

This is a security vulnerability.

In the ConnectBlock() function of src/validation.cpp, it misses the check for whether the block is corrupted or not. If the block is corrupted, it should be abort immediately, otherwise it will repeat trying to connect the corrupted block.

A possible solution is to add a corruption check inner the if statement of line 2399 in src/validation.cpp.

Similar fix from Bitcoin: bitcoin/bitcoin@0e7c52d.

Reported by 6004ed5feaa31ae9df36b5dbc60f0fa53255a5fb734334082c6d202405fc738c.

fdoving added a commit to fdoving/Ravencoin that referenced this issue Nov 7, 2021
Manual backport of bitcoin#0e7c52dc6cbb8fd881a0dd57a6167a812fe71dc4

Fixes issue RavenProject#1087

Commit message by author sdaftuar:
The call to CheckBlock() in ConnectBlock() is redundant with calls to it
prior to storing a block on disk. If CheckBlock() fails with an error
indicating the block is potentially corrupted, then shut down
immediately, as this is an indication that the node is experiencing
hardware issues.  (If we didn't shut down, we could go into an infinite
loop trying to reconnect this same bad block, as we're not setting the
block's status to FAILED in the case where there is potential
corruption.)

If CheckBlock() fails for some other reason, we'll end up flagging this
block as bad (perhaps some prior software version "let a bad block in",
as the comment indicates), and not trying to connect it again, so this
case should be properly handled.
fdoving added a commit to fdoving/Ravencoin that referenced this issue Nov 7, 2021
Manual backport of bitcoin#0e7c52dc6cbb8fd881a0dd57a6167a812fe71dc4

Fixes issue RavenProject#1087

Commit message by author sdaftuar:
The call to CheckBlock() in ConnectBlock() is redundant with calls to it
prior to storing a block on disk. If CheckBlock() fails with an error
indicating the block is potentially corrupted, then shut down
immediately, as this is an indication that the node is experiencing
hardware issues.  (If we didn't shut down, we could go into an infinite
loop trying to reconnect this same bad block, as we're not setting the
block's status to FAILED in the case where there is potential
corruption.)

If CheckBlock() fails for some other reason, we'll end up flagging this
block as bad (perhaps some prior software version "let a bad block in",
as the comment indicates), and not trying to connect it again, so this
case should be properly handled.
@fdoving fdoving added this to the 4.7.0 milestone Dec 6, 2021
@fdoving fdoving self-assigned this Dec 6, 2021
@fdoving fdoving linked a pull request Dec 15, 2021 that will close this issue
fdoving added a commit to fdoving/Ravencoin that referenced this issue Jan 18, 2022
Manual backport of bitcoin#0e7c52dc6cbb8fd881a0dd57a6167a812fe71dc4

Fixes issue RavenProject#1087

Commit message by author sdaftuar:
The call to CheckBlock() in ConnectBlock() is redundant with calls to it
prior to storing a block on disk. If CheckBlock() fails with an error
indicating the block is potentially corrupted, then shut down
immediately, as this is an indication that the node is experiencing
hardware issues.  (If we didn't shut down, we could go into an infinite
loop trying to reconnect this same bad block, as we're not setting the
block's status to FAILED in the case where there is potential
corruption.)

If CheckBlock() fails for some other reason, we'll end up flagging this
block as bad (perhaps some prior software version "let a bad block in",
as the comment indicates), and not trying to connect it again, so this
case should be properly handled.
hans-schmidt pushed a commit that referenced this issue Feb 9, 2022
Manual backport of bitcoin#0e7c52dc6cbb8fd881a0dd57a6167a812fe71dc4

Fixes issue #1087

Commit message by author sdaftuar:
The call to CheckBlock() in ConnectBlock() is redundant with calls to it
prior to storing a block on disk. If CheckBlock() fails with an error
indicating the block is potentially corrupted, then shut down
immediately, as this is an indication that the node is experiencing
hardware issues.  (If we didn't shut down, we could go into an infinite
loop trying to reconnect this same bad block, as we're not setting the
block's status to FAILED in the case where there is potential
corruption.)

If CheckBlock() fails for some other reason, we'll end up flagging this
block as bad (perhaps some prior software version "let a bad block in",
as the comment indicates), and not trying to connect it again, so this
case should be properly handled.
hans-schmidt added a commit that referenced this issue Aug 21, 2022
* Re-Add Feature: Add P2SH support - Core Protocol Development Proposal 001 (PR #873)

Merged, as we want to continue testing on develop.

* Default to c++17 (#1020)

Update configure.ac to c++17

Update ax_cxx_compile_stdcxx.m4 from bitcoin
    Copied from bitcoin master branch.

Use C++11 member initializer in CNodeState
    Manually copied/adapted from bitcoin#21370

net_processing:
 IteratorComparator add const. [c++17]

* [backport] gui, wallet: random abort (segmentation fault) btc#9683 (#1021)

backport from bitcoin #9683

* refactor: ravengui cleanups. (#1022)

* Require miniupnpc v10 (#1023)

Ref: CVE-2017-8798
    bitcoin#10414
    bitcoin#15993

    configure-checks for miniupnpc API >=10.
    This commit does not include the compile-time checks from bitcoin#15993.

* refactor: remove unneccesary parantheses. (#1024)

Remove compiler warning.

* testnet updated seeds (#1027)

Added 7 nodes picked from my 3 testnet wallets.
Better than the two we know do not respond to anything.

* verification.cpp update variables to be const (#1028)

We don't want these to change.
    Fixes issue #800.

* contrib: Dockerfile updated (#1026)

* contrib: Dockerfile updated

    Use Ubuntu 20.04.
    Drop outdated bitcoin-ppa.
    Build Berkeley DB 4.8 from source with install_db4.sh.

    Usage: 'docker build -t ravencoin:4.7 -f contrib/Dockerfile .'

* Remove obsolete Dockerfiles.

* build: update Expat 2.4.1 change source to github. (#1033)

Expat renamed files because of CVE-2013-0340/CWE-776.
    This switches to v.2.4.1.
    Source changed from sourceforge to github.

doc: update dependencies.md

* gui: option to hide text in toolbar (#1030)

* gui: option to hide text in toolbar

Add option to only show icons in toolbar.

Closes #478

* Don't require restart.

* Update comment.

* depends: libevent update (btc#21991) (#1032)

Update libevent to 2.1.12-stable.

    https://raw.githubusercontent.com/libevent/libevent/release-2.1.12-stable/ChangeLog

* gui: add version to modal overlay (#1034)

* testnet switch to the correct portnumber for seeds (#1035)

* Drop buggy TableViewLastColumnResizingFixer class (btc#204) (#1036)

backport bitcoin#204

    In Qt 5 the last column resizing with dragging its left edge works
    out-of-the-box.
    The current TableViewLastColumnResizingFixer implementation could put
    the last column content out of the view port and confuse a user.

* Move app out of if, and remove qt version checks (#1038)

moved app out of the if.
    removed extra qt version check.

* fix typo (#1039)

* gui: toolbar move code to match order in toolbar (#1040)

Code segment moved to match the order it appears
    Switched alt-<num> hotkeys to match the correct order.
    Set hotkey for Restricted assets (ALT+8).

* p2p: add fixed seeds more often. (#1042)

* translation: update transifex client config. (#1043)

* consensus: correct verification of transactions pre p2sh-asset activation (#1019)

Handle pre-fork padded P2SH-ish transactions like v4.3.2.1.

* gui: osx darkmode (#1025)

* Fix handling of nonstandard txns and "-acceptnonstdtxn" (#1048)

* GUI: Reissue asset spinboxes. Disable if unit is max. (#1050)

Disable units spinbox if max units is already set.

    Set minvalue for spinbox after setting value, to allow
    updates when the number of units in the selected asset is
    lower than the number of units in the previous.

* gui: Manage assets - update list when shown. (#1060)

Update asset list when the qcombobox is shown.
    Fixes #1059

* Introduce a Shuffle for FastRandomContext and use it in wallet (#1057)

Backport part of bitcoin#14624

bitcoin#3db746beb407f7cdd9cd6a605a195bef1254b4c0

* Release: version 4.7.0test2 (#1063)

Bump version number for second test release.

* net: Do not force dns seeds by default. (#1070)

It does not make sense to force dns seeds by default.

* GUI: Get my words, re-lock wallet if not bip44. (#1068)

If the wallet does not have words, an error message is shown.
This happens after the wallet is unlocked, to check for words.
This change locks the wallet after showing the error.

* depends: Bump Qt version to 5.12.11 (#1067)

backport of bitcoin#22054

Slightly modified for Ravencoin.

Co-Authored-by: fdov <fd21@pm.me>

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

* Update translations (#1074)

* build: github action enable Skip Duplicate Actions. (#1078)

Setup Skip Duplicate Actions in the workflow.
    The reason for doing this, is that githubs path-ignore feature
    does not play well with required checks on master.

    Skip Duplicate Actions include a better paths_ignore feature.

    https://github.com/marketplace/actions/skip-duplicate-actions

* Updated Copyright Year Using Python Dev Tool (#1080)

* depends: zeromq 4.3.4 (#1066)

CVE-2021-20236

* [GUI] Fixed display for atomic swap transactions in wallet (#1069)

* Added support for displaying atomic swap transactions properly in wallet.

* Added back section removed during testing.

* Added tr() calls, Fixed whitespace, Fixed compiler warnings

* ws

* gui: set window minimum size to usable default (#1075)

Set minimum to 875x700, even that is only usable if
The toolbar is set to icons only.

Set some minimum values for layouts in overview and manage assets.

* Bip39: Add support multilanguage (#1085)

* Add support multilanguage to BIP39 for import words

* Generate words multilanguage in GUI

* Add more languages to tests BIP39

* Add test multilanguage to test_runner.py

* Diferent required minimun words for each language

* Add warning spaces to Japanese

* Format test wallet util list words

* Backport: Remove useless mapRequest tracking

Manual backport of bitcoin/bitcoin@beef7ec
Fixes issue #1114
CVE-2018-17145

* lockedpool: avoid sensitive data in core files (Linux and FreeBSD)

Manual backport of bitcoin PR#18443 and bitcoin PR#15633.

Use madvise on Linux and FreeBSD to avoid sensitive data from secure_allocator to be written to swap and core-files.

bitcoin/bitcoin@d831831
bitcoin/bitcoin@f852030

* Update README.md

* Update README.md

* Update feature_assets_p2sh.py

SyntaxWarning: "is" with a literal. Did you mean "=="?

* http: Release work queue after event base finish (btc#19033) (#1031)

backport of bitcoin#19033.

This fixes a race between http_request_cb and StopHTTPServer where
the work queue is used after release. - promag @ bitcoin.

* Add asset support for IsFromMe (#1103)

* Security: Fix for issue #1110, CVE-2021-3401 (#1111)

Fixes URI Argument Injection in Ravencoin-Qt
    Ref: https://achow101.com/2021/02/0.18-uri-vuln
    Ref: bitcoin/bitcoin#16578

* fix: include array to fix building for osx. (#1122)

* gui: Wallet -> Get my words, auto close. (#1125)

Messasgebox with words will auto close after 5 min (300 sec.)

* backport: Shut down if trying to connect a corrupted block (#1126)

Manual backport of bitcoin#0e7c52dc6cbb8fd881a0dd57a6167a812fe71dc4

Fixes issue #1087

Commit message by author sdaftuar:
The call to CheckBlock() in ConnectBlock() is redundant with calls to it
prior to storing a block on disk. If CheckBlock() fails with an error
indicating the block is potentially corrupted, then shut down
immediately, as this is an indication that the node is experiencing
hardware issues.  (If we didn't shut down, we could go into an infinite
loop trying to reconnect this same bad block, as we're not setting the
block's status to FAILED in the case where there is potential
corruption.)

If CheckBlock() fails for some other reason, we'll end up flagging this
block as bad (perhaps some prior software version "let a bad block in",
as the comment indicates), and not trying to connect it again, so this
case should be properly handled.

* fixes #1072 (#1128)

* gui: optional seed extension word rename (#1132)

* Re-word in the import dialog.
    * Passphrase is used multiple times, renaming this to Seed Extension Word.
    * Add better tooltip.

Co-authored-by: kinkajou <k1nk4j0u@protonmail.com>
Co-authored-by: Hraf <hrafnagaldr1@protonmail.com>
Co-authored-by: LSJI07

Co-authored-by: kinkajou <k1nk4j0u@protonmail.com>
Co-authored-by: Hraf <hrafnagaldr1@protonmail.com>

* gui: minimum sizes and margins (#1133)

* Increase minimum window size to 1024x700
    * Adjust layouts most pages, to be usable at the new minimum window size.
    * Most marings are reduced.

* gui: search for string in whole asset name. (#1134)

* AssetFilterProxy expanded to be able to search
      for strings in the whole asset name.
      Previously only the prefix was searchable.

* gui: Create Assets tooltip wording (#1138)

Remove 'main/sub/unique' from the tooltip.
    The list was incomplete, and adding everything would make it too long.

* Update translations (#1140)

* depends: increase download timeout and retries (#1145)

* Increased download timeout from 10 to 30.
    * Increased retries from 3 to 5.

* backport: [validation] Crash if disconnecting a block fails (#1159)

Manual backport of bitcoin PR 15305

If we're unable to disconnect a block during normal operation, then that is a
failure of our local system (such as disk failure) or the chain that we are on
(eg CVE-2018-17144), but cannot be due to failure of the (more work) chain that
we're trying to validate.

We should abort rather than stay on a less work chain.

* backport: wallet: Fix Char as Bool in Wallet (#1160)

Manual backport of bitcoin PR #16572

* backport: gui: Set CConnman byte counters earlier to avoid uninitialized reads (#1161)

Manual backport of bitcoin PR #17906

Initialize CConnman byte counters during construction, so GetTotalBytesRecv()
and GetTotalBytesSent() methods don't return garbage before Start() is called.

Change shouldn't have any effect outside of the GUI. It just fixes a race
condition during a qt test that was observed on travis:
https://travis-ci.org/bitcoin/bitcoin/jobs/634989685

* backport: rpc: Fix data race (UB) in InterruptRPC() (#1162)

manual backport of bitcoin PR # 14993

rpc: Fix data race (UB) in InterruptRPC()

* backport: fix uninitialized read when stringifying an addrLocal (#1163)

Manual backport of bitcoin PR 14728

Reachable from either place where SetIP is used when our best-guess
addrLocal for a peer is IPv4, but the peer tells us it's reaching us at
an IPv6 address.

In that case, SetIP turns an IPv4 address into an IPv6 address without
setting the scopeId, which is subsequently read in GetSockAddr during
CNetAddr::ToStringIP and passed to getnameinfo. Fix by ensuring every
constructor initializes the scopeId field with something.

* backport: Introduce a maximum size for locators. (#1164)

Manual backport of bitcoin PR #13907

The largest sensible size for a locator is log in the number of blocks.
 But, as noted by Coinr8d on BCT a maximum size message could encode a
 hundred thousand locators.  If height were used to limit the messages
 that could open new attacks where peers on long low diff forks would
 get disconnected and end up stuck.

Ideally, nodes first first learn to limit the size of locators they
 send before limiting what would be processed, but common implementations
 back off with an exponent of 2 and have an implicit limit of 2^32
 blocks, so they already cannot produce locators over some size.

This sets the limit to an absurdly high amount of 101 in order to
 maximize compatibility with existing software.

* [qt] Fix potential memory leak in newPossibleKey(ChangeCWallet *wallet) (#1167)

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>

* backport: Make CWallet::FundTransaction atomic (bitcoin #11864) (#1173)

* [wallet] Tidy up CWallet::FundTransaction

* [wallet] Make CWallet::FundTransaction atomic

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

* docs: Update dependencies for Ubuntu 21.10 (#1180)

* Fix mining bug - lockup in CreateNewBlock loop

This bug was discovered in Dec-2021 when two blocks were mined within one second.
Pool operators locked up and required restarting.

It happened because when CheckTxInputs finds a double spend, ConnectBlock fails but
didn't mark the bad txn to be flushed from the mempool.

* Fix typo

* Remove redundant locks (#1169)

* SetAddressBook(...) is locking cs_wallet internally
* DelAddressBook(...) is locking cs_wallet internally

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>

* gui: create and reissue asset views - browse ipfs button (#1144)

* gui: create asset view - browse ipfs button

    * Add button to open ipfs-hash in the configured ipfs-viewer.
      This can be handy to verify your ipfs-hash is correct, before creating the asset.

    * Remove a check that enabled the Check Availability button once the `Add IPFS/Txid hash` was enabled.

* gui: reissue asset view, browse ipfs button.

* Add button to open ipfs-hash in the configured ipfs-viewer.
  This can be handy to verify your ipfs-hash is correct, before reissuing the asset.

* backport: net: Add missing locks in net.{cpp,h} (bitcoin #11744) (#1170)

* net: Add missing locks in net.{cpp,h}

* writing variable 'nTotalBytesRecv' requires holding mutex 'cs_totalBytesRecv' exclusively
* writing variables 'nTotalBytesSent'/'nMaxOutboundTotalBytesSentInCycle'/'nMaxOutboundCycleStartTime' require holding mutex 'cs_totalBytesSent' exclusively
* writing variable 'nMaxOutboundTimeframe'/'nMaxOutboundLimit' require holding mutex 'cs_totalBytesSent' exclusively
* writing variable 'vAddedNodes' requires holding mutex 'cs_vAddedNodes' exclusively

* Add Clang thread safety analysis annotations

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
Co-authored-by: hans-schmidt <43421934+hans-schmidt@users.noreply.github.com>

* Revert "backport: Shut down if trying to connect a corrupted block (#1126)" (#1185)

This reverts commit d3243c1.

* Fix createrawtransaction transferwithmessage RPC (#1113)

Fixes issue #1102.

Removed some brackets.
Removed one asset_quantity.
Adjusted indentation.

* Fix bug with in memory qualifier address checking

* Revert p2sh (#1201)

* Revert "consensus: correct verification of transactions pre p2sh-asset activation (#1019)"

This reverts commit 46aad1a.

* Revert "Re-Add Feature: Add P2SH support - Core Protocol Development Proposal 001 (PR #873)"

This reverts commit 8c31e2b.

* Fix function test failures which depend on nonstandard transactions (#1202)

* Update nMinimumChainWork, defaultAssumeValid, checkpointData, chainTxData (#1203)

* Update release version to v4.6.1 in prep for develop merge into master (#1204)

Co-authored-by: HyperPeek <hyperpeek@gmail.com>
Co-authored-by: fdov <fd21@pm.me>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: BrockWood <brocks.work@gmail.com>
Co-authored-by: Ben Abraham <ben.d.abraham@gmail.com>
Co-authored-by: Henry_arcu89 <57180462+henry-arcu89@users.noreply.github.com>
Co-authored-by: kralverde <80051564+kralverde@users.noreply.github.com>
Co-authored-by: jeroz1 <44138303+jeroz1@users.noreply.github.com>
Co-authored-by: Volodymyr Biloshytskyi <volbilnexus@gmail.com>
Co-authored-by: StayAtHomeProgrammer <75764463+StayAtHomeProgrammer@users.noreply.github.com>
Co-authored-by: kinkajou <k1nk4j0u@protonmail.com>
Co-authored-by: Hraf <hrafnagaldr1@protonmail.com>
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
Co-authored-by: classObject <16977295+classObject@users.noreply.github.com>
Co-authored-by: Tron <TronBlack@gmail.com>
Co-authored-by: blondfrogs <8285518+blondfrogs@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants