addrman: Avoid crash on corrupt data, Force Check after deserialize #22734

maflcko · 2021-08-18T07:16:32Z

Assert should only be used for program internal logic errors, not to sanitize external user input.

The assert was introduced via the debug-only runtime option -checkaddrman in commit 803ef70, thus won't need a backport.

Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.

For example, if nLastSuccess is negative, it would later result in integer overflows. Thus, this patch fixes #22931.

Also,
Fixes #22503
Fixes #22504
Fixes #22519

Closes #22498

Steps to test:

mkdir -p /tmp/test_235/regtest/
echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'

Output before:

2021-09-10T11:28:37Z init message: Loading P2P addresses…
2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.

(program crashes)

Output after:

2021-09-10T11:26:00Z init message: Loading P2P addresses…
2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.

(program exits)

mzumsande · 2021-08-18T08:27:26Z

I agree that corrupt data shouldn't assert.

Check() used to be restricted to tests / use of the checkaddrman option, it was never run per default. This PR makes it run always for Unserialize(), so it introduces stricter rules for the level of inconsistencies we tolerate from peers.dat.
I think this change of default behavior should be mentioned in the OP and maybe somewhere in the code, because it also introduces new possibilities of p2p addrman poisoning to make us drop our peers.dat - so we should be confident all checks in Check() are correct before enabling this.

maflcko · 2021-08-18T08:30:03Z

Thanks, renamed PR title

mzumsande · 2021-08-18T08:41:22Z

Thanks! I think there is a bug in the addman_serdeser fuzz test (see CI fail) that would need fixing, see mzumsande@f484061

jnewbery · 2021-08-18T09:22:20Z

With this change, if we have a bug that causes us to write a peers.dat file that's corrupt in any way, then it would mostly be undetected - the addrman would be cleared and startup would continue as normal. As @mzumsande points out, if it's possible to trigger such a bug over p2p, then an adversary could cause victims to lose their entire addrman on restart.

It might be safer to do the ForceCheck, but then catch the exception, rename peers.dat to peers.bak, and terminate with some error message like "Corrupt peers.dat file found. addrman database has been wiped and peers.dat has been renamed to peers.bak. Please report this to the Bitcoin Core developers." That way, we'd get bug reports if there was a bug in our serialization code, and there may be a way to salvage the peers.bak file if needed.

vasild

Concept ACK fab42d7

I think that the check should also be run (unconditionally) before serialize and if a problem is detected then don't write the corrupted stuff on disk.

Keeping the corrupted peers.dat on unserialize seems like a reasonable thing to do, but right now there are no tools to salvage it, so it would be useless. Maybe just stop the startup and ask the user to remove (or rename) the damaged peers.dat?

Notice - if it is possible to remotely corrupt peers.dat, then currently master is even worse than this PR - reading a corrupted peers.dat and keep operating with a "corrupted" addrman could lead to undefined behavior.

src/addrman.h

naumenkogs · 2021-08-20T18:22:23Z

With this change, if we have a bug that causes us to write a peers.dat file that's corrupt in any way, then it would mostly be undetected - the addrman would be cleared and startup would continue as normal. As @mzumsande points out, if it's possible to trigger such a bug over p2p, then an adversary could cause victims to lose their entire addrman on restart.

It might be safer to do the ForceCheck, but then catch the exception, rename peers.dat to peers.bak, and terminate with some error message like "Corrupt peers.dat file found. addrman database has been wiped and peers.dat has been renamed to peers.bak. Please report this to the Bitcoin Core developers." That way, we'd get bug reports if there was a bug in our serialization code, and there may be a way to salvage the peers.bak file if needed.

I agree with John.

DrahtBot · 2021-08-21T02:22:30Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22974 (addrman: Improve performance of Good by mzumsande)
#22950 ([p2p] Pimpl AddrMan to abstract implementation details by amitiuttarwar)
#22910 ([RFC] Encapsulate asmap in NetGroupManager by jnewbery)
#22872 (log: improve checkaddrman logging with duration in milliseconds by jonatack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2021-08-21T12:45:28Z

I agree with @vasild that this is an unrelated bug that already exists. I've created a separate pull to fix that: #22762

vasild

ACK 4fd5305

CAddrMan::Unserialize() already, before this PR, could throw if it encounters a corrupted peers.dat. I think it is logical to do the same if the consistency checks fail after the unserialize process has finished "successfully". Also, it is important enough and not too expensive to do the consistency checks always, unconditionally, regardless of -checkaddrman at the end of unserialize.

(the fuzzer is upset, probably needs some petting)

src/addrman.cpp

maflcko · 2021-09-15T09:00:09Z

(the fuzzer is upset, probably needs some petting)

Thanks. Force pushed to pet the fuzzer and fixup the docs. (no other code changes)

vasild

ACK fa6f5c9

maflcko · 2021-09-15T10:11:34Z

@mzumsande @naumenkogs @jnewbery Now that #22762 is merged, I rebased this pull. This should address your concerns.

naumenkogs · 2021-09-15T12:06:52Z

ACK fa6f5c9

This speeds up compilation of the whole program because the included header file is smaller. Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

Assert should only be used for program internal logic errors, not to sanitize external user input.

Can be reviewed with --color-moved=dimmed-zebra

maflcko · 2021-09-21T10:45:03Z

Rebased; Only trivial conflicts in the tests.

naumenkogs · 2021-09-21T10:59:19Z

ACK fa3669f

vasild

ACK fa3669f

jnewbery · 2021-09-21T15:48:03Z

Code review ACK fa3669f

…k after deserialize fa3669f fuzz: Move all addrman fuzz targets to one file (MarcoFalke) fa7a883 addrman: Replace assert with throw on corrupt data (MarcoFalke) fa29897 Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke) fae5c63 move-only: Move CAddrMan::Check to cpp file (MarcoFalke) Pull request description: Assert should only be used for program internal logic errors, not to sanitize external user input. The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70, thus won't need a backport. Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check. For example, if `nLastSuccess` is negative, it would later result in integer overflows. Thus, this patch fixes bitcoin#22931. Also, Fixes bitcoin#22503 Fixes bitcoin#22504 Fixes bitcoin#22519 Closes bitcoin#22498 Steps to test: ``` mkdir -p /tmp/test_235/regtest/ echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat ./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses' ``` Output before: ``` 2021-09-10T11:28:37Z init message: Loading P2P addresses… 2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16 bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed. (program crashes) ``` Output after: ``` 2021-09-10T11:26:00Z init message: Loading P2P addresses… 2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start. (program exits) ``` ACKs for top commit: naumenkogs: ACK fa3669f jnewbery: Code review ACK fa3669f vasild: ACK fa3669f Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6

…erflow:addrman.cpp" fa5e8c1 Revert "test: Add missing suppression signed-integer-overflow:addrman.cpp" (MarcoFalke) Pull request description: Forgot to do this in #22734 ACKs for top commit: fanquake: ACK fa5e8c1 Tree-SHA512: cabd741b73c828ae3c40df37d80022039e41e571fa196eebcb41e51c582064a1463a3677eb3b0e997ed141b32003802019474d2b739f0b606b4a16da4f585faa

…eger-overflow:addrman.cpp" fa5e8c1 Revert "test: Add missing suppression signed-integer-overflow:addrman.cpp" (MarcoFalke) Pull request description: Forgot to do this in bitcoin#22734 ACKs for top commit: fanquake: ACK fa5e8c1 Tree-SHA512: cabd741b73c828ae3c40df37d80022039e41e571fa196eebcb41e51c582064a1463a3677eb3b0e997ed141b32003802019474d2b739f0b606b4a16da4f585faa

Summary: ``` Assert should only be used for program internal logic errors, not to sanitize external user input. The assert was introduced via the debug-only runtime option -checkaddrman in commit 803ef70, thus won't need a backport. Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check. For example, if nLastSuccess is negative, it would later result in integer overflows. ``` Backport of [[bitcoin/bitcoin#22734 | core#22734]]. Depends on D12310. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12311

fanquake added the P2P label Aug 18, 2021

maflcko mentioned this pull request Aug 18, 2021

Check that CAddrMan::nKey is not null after deserialize #22498

Closed

maflcko changed the title ~~addrman: Avoid crash on corrupt data~~ addrman: Avoid crash on corrupt data, Run Check after deserialize Aug 18, 2021

maflcko changed the title ~~addrman: Avoid crash on corrupt data, Run Check after deserialize~~ addrman: Avoid crash on corrupt data, Force Check after deserialize Aug 18, 2021

vasild reviewed Aug 18, 2021

View reviewed changes

src/addrman.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 21, 2021

[addrman] Move serialization code to cpp #22740

Merged

maflcko added the Waiting for author label Aug 21, 2021

DrahtBot added the Needs rebase label Sep 1, 2021

maflcko removed the Waiting for author label Sep 1, 2021

mzumsande mentioned this pull request Sep 9, 2021

Signed Integer-overflow in CAddrInfo::IsTerrible #22931

Closed

maflcko added the Waiting for author label Sep 10, 2021

maflcko force-pushed the 2108-addrmanNoCrash branch 2 times, most recently from faecb49 to 4fd5305 Compare September 10, 2021 11:24

DrahtBot removed the Needs rebase label Sep 10, 2021

This was referenced Sep 11, 2021

log: improve checkaddrman logging with duration in milliseconds #22872

Closed

[p2p] Pimpl AddrMan to abstract implementation details #22950

Merged

maflcko mentioned this pull request Sep 14, 2021

fuzz: Reset addrman when consistency check fails #22939

Closed

vasild approved these changes Sep 15, 2021

View reviewed changes

src/addrman.cpp Show resolved Hide resolved

maflcko removed the Waiting for author label Sep 15, 2021

maflcko force-pushed the 2108-addrmanNoCrash branch from 4fd5305 to fa6f5c9 Compare September 15, 2021 08:59

vasild approved these changes Sep 15, 2021

View reviewed changes

This was referenced Sep 15, 2021

scripted-diff: update license URLs to https #22975

Closed

addrman: Improve performance of Good #22974

Merged

net: Encapsulate asmap in NetGroupManager #22910

Merged

DrahtBot added the Needs rebase label Sep 20, 2021

MarcoFalke added 4 commits September 21, 2021 10:07

move-only: Move CAddrMan::Check to cpp file

fae5c63

This speeds up compilation of the whole program because the included header file is smaller. Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

Refactor: Turn the internal addrman check helper into a forced check

fa29897

addrman: Replace assert with throw on corrupt data

fa7a883

Assert should only be used for program internal logic errors, not to sanitize external user input.

fuzz: Move all addrman fuzz targets to one file

fa3669f

Can be reviewed with --color-moved=dimmed-zebra

maflcko force-pushed the 2108-addrmanNoCrash branch from fa6f5c9 to fa3669f Compare September 21, 2021 08:23

DrahtBot removed the Needs rebase label Sep 21, 2021

vasild approved these changes Sep 21, 2021

View reviewed changes

Bossday mentioned this pull request Sep 21, 2021

. #23055

Closed

maflcko mentioned this pull request Sep 21, 2021

fuzz: Move all addrman fuzz targets to one file #22940

Closed

maflcko merged commit a8a272a into bitcoin:master Sep 21, 2021

maflcko deleted the 2108-addrmanNoCrash branch September 21, 2021 16:26

maflcko mentioned this pull request Sep 22, 2021

test: Revert "Add missing suppression signed-integer-overflow:addrman.cpp" #23059

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

addrman: Avoid crash on corrupt data, Force Check after deserialize #22734

addrman: Avoid crash on corrupt data, Force Check after deserialize #22734

Uh oh!

Conversation

maflcko commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Aug 18, 2021

Uh oh!

maflcko commented Aug 18, 2021

Uh oh!

mzumsande commented Aug 18, 2021

Uh oh!

jnewbery commented Aug 18, 2021

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naumenkogs commented Aug 20, 2021

Uh oh!

DrahtBot commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Aug 21, 2021

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 15, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 15, 2021

Uh oh!

naumenkogs commented Sep 15, 2021

Uh oh!

maflcko commented Sep 21, 2021

Uh oh!

naumenkogs commented Sep 21, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 21, 2021

Uh oh!

Uh oh!

maflcko commented Aug 18, 2021 •

edited

Loading

vasild left a comment •

edited

Loading

DrahtBot commented Aug 21, 2021 •

edited

Loading

vasild left a comment •

edited

Loading