Page MenuHomePhabricator
Differential D17149

modernize DecodeBase{32,64}, reject incorrect base64 in HTTP auth
ClosedPublic
Actions

Authored by PiRK on Nov 16 2024, 10:34.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC5dc47a349314: modernize DecodeBase{32,64}, reject incorrect base64 in HTTP auth
Summary

Reject incorrect base64 in HTTP auth

In addition, to make sure that no call site ignores the invalid
decoding status, make the pf_invalid argument mandatory.

https://github.com/bitcoin/bitcoin/pull/25001/commits/a4377a0843636eae0aaf698510fc6518582545db

Make DecodeBase{32,64} always return vector, not string

Base32/base64 are mechanisms for encoding binary data. That they'd
decode to a string is just bizarre. The fact that they'd do that
based on the type of input arguments even more so.

https://github.com/bitcoin/bitcoin/pull/25001/commits/a65931e3ce66d87b8f83d67ecdbb46f137e6a670

Make DecodeBase{32,64} return optional instead of taking bool*

https://github.com/bitcoin/bitcoin/pull/25001/commits/78f3ac51b7d073d12da6a3b9b7d80d91e04ce3a7

This is a partial backport of core#25001

Depends on D17148

Test Plan

ninja all check-all bitcoin-fuzzers

Diff Detail

Event Timeline

PiRK created this revision.Nov 16 2024, 10:34
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 16 2024, 10:34
PiRK requested review of this revision.Nov 16 2024, 10:34
teamcity edited the summary of this revision. (Show Details)Nov 16 2024, 10:34
PiRK added a comment.Nov 16 2024, 10:36

Note that I squashed these 3 commits because they kept touching the same lines. The intermediate states in-between commits was not very interesting to have in the git history.

PiRK added a child revision: D17150: Make DecodeBase{32,64} take string_view arguments.Nov 16 2024, 10:37
Harbormaster completed remote builds in B31165: Diff 50887.Nov 16 2024, 10:47
teamcity added a comment.Nov 16 2024, 10:47
Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

[379/575] Linking C executable src/secp256k1/ecmult-bench
[380/575] Linking C executable src/secp256k1/internal-bench
[381/575] Building CXX object src/CMakeFiles/server.dir/rpc/avalanche.cpp.o
[382/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockindex.cpp.o
[383/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/coins.cpp.o
[384/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[385/575] Building CXX object src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o
[386/575] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[387/575] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[388/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[389/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/random.cpp.o
[390/575] Building CXX object src/CMakeFiles/server.dir/txmempool.cpp.o
[391/575] Building CXX object src/CMakeFiles/server.dir/torcontrol.cpp.o
[392/575] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[393/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[394/575] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[395/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[396/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[397/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[398/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[399/575] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[400/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[401/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[402/575] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[403/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[404/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[405/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/validation.cpp.o
[406/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/txmempool.cpp.o
[407/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[408/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[409/575] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[410/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[411/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[412/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[413/575] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[414/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[415/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[416/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[417/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[418/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[419/575] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[420/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[421/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[422/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[423/575] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_interpreter.cpp.o
[424/575] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[425/575] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_formatter.cpp.o
[426/575] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[427/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[428/575] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana.cpp.o
[429/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[430/575] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[431/575] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[432/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[433/575] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[434/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[435/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[436/575] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
PiRK updated this revision to Diff 50892.Nov 16 2024, 11:00

rebase

Harbormaster completed remote builds in B31170: Diff 50892.Nov 16 2024, 11:38
teamcity added a comment.Nov 16 2024, 11:39
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

tests/test_iguana.py::test_invalid_inputindex ##teamcity[testStarted timestamp='2024-11-16T11:34:10.910' captureStandardOutput='false' flowId='tests.test_iguana.test_invalid_inputindex' metainfo='test_invalid_inputindex' name='tests.test_iguana.test_invalid_inputindex']
PASSED                     [ 25%]##teamcity[testFinished timestamp='2024-11-16T11:34:10.925' duration='14' flowId='tests.test_iguana.test_invalid_inputindex' name='tests.test_iguana.test_invalid_inputindex']

tests/test_iguana.py::test_sig_push_only ##teamcity[testStarted timestamp='2024-11-16T11:34:10.926' captureStandardOutput='false' flowId='tests.test_iguana.test_sig_push_only' metainfo='test_sig_push_only' name='tests.test_iguana.test_sig_push_only']
PASSED                          [ 30%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.018' duration='92' flowId='tests.test_iguana.test_sig_push_only' name='tests.test_iguana.test_sig_push_only']

tests/test_iguana.py::test_script_sig_success ##teamcity[testStarted timestamp='2024-11-16T11:34:11.019' captureStandardOutput='false' flowId='tests.test_iguana.test_script_sig_success' metainfo='test_script_sig_success' name='tests.test_iguana.test_script_sig_success']
PASSED                     [ 35%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.103' duration='83' flowId='tests.test_iguana.test_script_sig_success' name='tests.test_iguana.test_script_sig_success']

tests/test_iguana.py::test_script_sig_invalid_opcode_encoding ##teamcity[testStarted timestamp='2024-11-16T11:34:11.103' captureStandardOutput='false' flowId='tests.test_iguana.test_script_sig_invalid_opcode_encoding' metainfo='test_script_sig_invalid_opcode_encoding' name='tests.test_iguana.test_script_sig_invalid_opcode_encoding']
PASSED     [ 40%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.158' duration='54' flowId='tests.test_iguana.test_script_sig_invalid_opcode_encoding' name='tests.test_iguana.test_script_sig_invalid_opcode_encoding']

tests/test_iguana.py::test_script_pub_key_success ##teamcity[testStarted timestamp='2024-11-16T11:34:11.159' captureStandardOutput='false' flowId='tests.test_iguana.test_script_pub_key_success' metainfo='test_script_pub_key_success' name='tests.test_iguana.test_script_pub_key_success']
PASSED                 [ 45%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.227' duration='67' flowId='tests.test_iguana.test_script_pub_key_success' name='tests.test_iguana.test_script_pub_key_success']

tests/test_iguana.py::test_script_pub_key_failure ##teamcity[testStarted timestamp='2024-11-16T11:34:11.228' captureStandardOutput='false' flowId='tests.test_iguana.test_script_pub_key_failure' metainfo='test_script_pub_key_failure' name='tests.test_iguana.test_script_pub_key_failure']
PASSED                 [ 50%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.250' duration='21' flowId='tests.test_iguana.test_script_pub_key_failure' name='tests.test_iguana.test_script_pub_key_failure']

tests/test_iguana.py::test_script_pub_key_empty_stack ##teamcity[testStarted timestamp='2024-11-16T11:34:11.251' captureStandardOutput='false' flowId='tests.test_iguana.test_script_pub_key_empty_stack' metainfo='test_script_pub_key_empty_stack' name='tests.test_iguana.test_script_pub_key_empty_stack']
PASSED             [ 55%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.287' duration='36' flowId='tests.test_iguana.test_script_pub_key_empty_stack' name='tests.test_iguana.test_script_pub_key_empty_stack']

tests/test_iguana.py::test_script_pub_key_false_stack ##teamcity[testStarted timestamp='2024-11-16T11:34:11.288' captureStandardOutput='false' flowId='tests.test_iguana.test_script_pub_key_false_stack' metainfo='test_script_pub_key_false_stack' name='tests.test_iguana.test_script_pub_key_false_stack']
PASSED             [ 60%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.310' duration='21' flowId='tests.test_iguana.test_script_pub_key_false_stack' name='tests.test_iguana.test_script_pub_key_false_stack']

tests/test_iguana.py::test_script_pub_key_cleanstack ##teamcity[testStarted timestamp='2024-11-16T11:34:11.311' captureStandardOutput='false' flowId='tests.test_iguana.test_script_pub_key_cleanstack' metainfo='test_script_pub_key_cleanstack' name='tests.test_iguana.test_script_pub_key_cleanstack']
PASSED              [ 65%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.325' duration='12' flowId='tests.test_iguana.test_script_pub_key_cleanstack' name='tests.test_iguana.test_script_pub_key_cleanstack']

tests/test_iguana.py::test_redeem_script_success ##teamcity[testStarted timestamp='2024-11-16T11:34:11.325' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_success' metainfo='test_redeem_script_success' name='tests.test_iguana.test_redeem_script_success']
PASSED                  [ 70%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.355' duration='29' flowId='tests.test_iguana.test_redeem_script_success' name='tests.test_iguana.test_redeem_script_success']

tests/test_iguana.py::test_redeem_script_error ##teamcity[testStarted timestamp='2024-11-16T11:34:11.356' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_error' metainfo='test_redeem_script_error' name='tests.test_iguana.test_redeem_script_error']
PASSED                    [ 75%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.371' duration='14' flowId='tests.test_iguana.test_redeem_script_error' name='tests.test_iguana.test_redeem_script_error']

tests/test_iguana.py::test_redeem_script_exception ##teamcity[testStarted timestamp='2024-11-16T11:34:11.371' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_exception' metainfo='test_redeem_script_exception' name='tests.test_iguana.test_redeem_script_exception']
PASSED                [ 80%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.386' duration='13' flowId='tests.test_iguana.test_redeem_script_exception' name='tests.test_iguana.test_redeem_script_exception']

tests/test_iguana.py::test_redeem_script_empty_stack ##teamcity[testStarted timestamp='2024-11-16T11:34:11.386' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_empty_stack' metainfo='test_redeem_script_empty_stack' name='tests.test_iguana.test_redeem_script_empty_stack']
PASSED              [ 85%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.401' duration='14' flowId='tests.test_iguana.test_redeem_script_empty_stack' name='tests.test_iguana.test_redeem_script_empty_stack']

tests/test_iguana.py::test_redeem_script_false ##teamcity[testStarted timestamp='2024-11-16T11:34:11.402' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_false' metainfo='test_redeem_script_false' name='tests.test_iguana.test_redeem_script_false']
PASSED                    [ 90%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.418' duration='15' flowId='tests.test_iguana.test_redeem_script_false' name='tests.test_iguana.test_redeem_script_false']

tests/test_iguana.py::test_redeem_script_cleanstack ##teamcity[testStarted timestamp='2024-11-16T11:34:11.418' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_cleanstack' metainfo='test_redeem_script_cleanstack' name='tests.test_iguana.test_redeem_script_cleanstack']
PASSED               [ 95%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.433' duration='13' flowId='tests.test_iguana.test_redeem_script_cleanstack' name='tests.test_iguana.test_redeem_script_cleanstack']

tests/test_iguana.py::test_redeem_script_input_sigchecks ##teamcity[testStarted timestamp='2024-11-16T11:34:11.434' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_input_sigchecks' metainfo='test_redeem_script_input_sigchecks' name='tests.test_iguana.test_redeem_script_input_sigchecks']
PASSED          [100%]##teamcity[testFinished timestamp='2024-11-16T11:34:11.489' duration='54' flowId='tests.test_iguana.test_redeem_script_input_sigchecks' name='tests.test_iguana.test_redeem_script_input_sigchecks']


============================== 20 passed in 0.76s ==============================
[212/489] Running pow test suite
PASSED: pow test suite
[227/489] Running seeder test suite
PASSED: seeder test suite
[231/489] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[486/489] Running bitcoin test suite
PASSED: bitcoin test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1
PiRK added a comment.EditedNov 16 2024, 14:49
In D17149#389136, @teamcity wrote:

That failure seems unrelated to this diff. I can rebase later and investigate the failure separately.

11:34:10   FAILED: src/avalanche/test/CMakeFiles/check-avalanche-processor_tests /work/abc-ci-builds/build-without-wallet/src/avalanche/test/CMakeFiles/check-avalanche-processor_tests
11:36:48   cd /work/abc-ci-builds/build-without-wallet/src/avalanche/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-without-wallet/test/log/avalanche-processor_tests.log /work/abc-ci-builds/build-without-wallet/src/avalanche/test/test-avalanche --run_test=processor_tests --logger=HRF,message:JUNIT,message,avalanche-processor_tests.xml --catch_system_errors=no -- -printtoconsole=1
11:36:48   Segmentation fault (core dumped)
PiRK updated this revision to Diff 50920.Nov 18 2024, 09:31

rebase

Fabien requested changes to this revision.Nov 18 2024, 09:53
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/qt/test/paymentservertests.cpp
92 ↗(On Diff #50920)

this is shadowing the above data var, can you please rename ?

This revision now requires changes to proceed.Nov 18 2024, 09:53
Harbormaster completed remote builds in B31181: Diff 50920.Nov 18 2024, 10:00
PiRK updated this revision to Diff 50921.Nov 18 2024, 10:03

fix var shadowing and move comment to the related line of code

Fabien accepted this revision.Nov 18 2024, 10:08
This revision is now accepted and ready to land.Nov 18 2024, 10:08
Harbormaster completed remote builds in B31182: Diff 50921.Nov 18 2024, 10:21
Closed by commit rABC5dc47a349314: modernize DecodeBase{32,64}, reject incorrect base64 in HTTP auth (authored by Pieter Wuille <pieter@wuille.net>, committed by PiRK). · Explain WhyNov 18 2024, 10:23
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC5dc47a349314: modernize DecodeBase{32,64}, reject incorrect base64 in HTTP auth.

Revision Contents
Changeset List

PathSize
src/
7 lines
7 lines
23 lines
2 lines
13 lines
qt/
test/
61 lines
5 lines
test/
19 lines
19 lines
fuzz/
14 lines
2 lines
util/
8 lines
8 lines
28 lines

Diff 50887

View Options

src/httprpc.cpp

Loading...
View Options

src/i2p.cpp

Loading...
View Options

src/netaddress.cpp

Loading...
View Options

src/psbt.h

Loading...
View Options

src/psbt.cpp

Loading...
View Options

src/qt/test/paymentservertests.cpp

Loading...
View Options

src/qt/walletview.cpp

Loading...
View Options

src/test/base32_tests.cpp

Loading...
View Options

src/test/base64_tests.cpp

Loading...
View Options

src/test/fuzz/base_encode_decode.cpp

Loading...
View Options

src/test/fuzz/psbt.cpp

Loading...
View Options

src/util/message.cpp

Loading...
View Options

src/util/strencodings.h

Loading...
View Options

src/util/strencodings.cpp

Loading...