supplying invalid blinding factors to rawblindrawtransaction causes node to crash

[dgpv]

if invalid value blinder is supplied, this assert is triggered:

elements/src/blind.cpp

Line 517 in dd1623a

assert(ret);

if invalid asset blinder is supplied, this CHECK() in secp256k1 is triggered:

elements/src/secp256k1/src/modules/generator/main_impl.h

Line 178 in dd1623a

CHECK(ret);

Triggered with this small patch to qa/rpc-tests/confidential_transactions.py (tested on 0.14.1 branch, but the failing code is the same on master. to trigger first case, you just put ab in place of value blinder, not asset blinder as in this patch):

diff --git a/qa/rpc-tests/confidential_transactions.py b/qa/rpc-tests/confidential_transactions.py
index 61393fc6e..93c7b5cc5 100755
--- a/qa/rpc-tests/confidential_transactions.py
+++ b/qa/rpc-tests/confidential_transactions.py
@@ -470,6 +470,12 @@ class CTTest (BitcoinTestFramework):
         except JSONRPCException:
             pass
 
+        try:
+            ab = 'FF'*32
+            blindtx = self.nodes[0].rawblindrawtransaction(rawtx, [unspent[0]["blinder"], unspent[1]["blinder"]], [unspent[0]["amount"], unspent[1]["amount"]], [unspent[0]["asset"], unspent[1]["asset"]], [unspent[0]["assetblinder"], ab])
+        except JSONRPCException as e:
+            print(e)
+
         blindtx = self.nodes[0].rawblindrawtransaction(rawtx, [unspent[0]["blinder"], unspent[1]["blinder"]], [unspent[0]["amount"], unspent[1]["amount"]], [unspent[0]["asset"], unspent[1]["asset"]], [unspent[0]["assetblinder"], unspent[1]["assetblinder"]])
         signtx = self.nodes[0].signrawtransaction(blindtx)
         txid = self.nodes[0].sendrawtransaction(signtx["hex"])

While the impact of the bug is not very serious, as this can only be triggered with authenticated rpc request and results only in node crash, I believe the crash is not correct response to invalid user input.

First case can be fixed by checking ret and returning failure, instead of dying on assert.

But the second case is not that easy, and would require checking that a given blinder represents valid scalar before passing it to secp256k1_generator_generate_blinded. I did not find appropriate function in secp256k1 that would allow to check this condition before passing the data to secp256k1_generator_generate_blinded.

[instagibbs]

Interesting report, thanks! Will look and see if we need to get support upstream in secp-zkp.

[instagibbs]

@apoelstra says secp256k1_ec_seckey_verify is what we want.

[dgpv]

If CHECK is removed in upstream secp-zkp, then pre-verification would not be needed. OTOH, that means there would be delay in fixing the bug, so maybe it should be fixed with pre-verification

[dgpv]

secp256k1_ec_seckey_verify also checks for zero scalar, but some code in wallet.cpp passes zero asset blinders to BlindTransaction(), so in addition to secp256k1_ec_seckey_verify there would need to be a check for zero scalar (probably, memcmp with static array of nuls), in that case it should not fail.

[instagibbs]

oof ok. That's annoying.

[dgpv]

Maybe it is better to just wait for remove of the CHECK in upstream secp-zkp, considering that the impact of the bug is small

[dgpv]

Just for reference, a patch that fixes this on 0.14 branch (not doing PR because 0.14 is outdated, but I don't have time now to properly build and test on master)

diff --git a/src/blind.cpp b/src/blind.cpp
index 10fd46454..a8d3f50db 100644
--- a/src/blind.cpp
+++ b/src/blind.cpp
@@ -247,6 +247,7 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
 
     int ret;
     int nBlindAttempts = 0, nIssuanceBlindAttempts = 0, nSuccessfullyBlinded = 0;
+    static const unsigned char zero_scalar[32] = {0};
 
     //Surjection proof prep
 
@@ -278,6 +279,10 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
                 return -1;
             }
         } else {
+            if (secp256k1_ec_seckey_verify(secp256k1_blind_context, input_asset_blinding_factors[i].begin()) != 1 &&
+                memcmp(zero_scalar, input_asset_blinding_factors[i].begin(), 32) != 0 ) {
+		return -1;
+            }
             ret = secp256k1_generator_generate_blinded(secp256k1_blind_context, &targetAssetGenerators[totalTargets], input_assets[i].begin(), input_asset_blinding_factors[i].begin());
             assert(ret == 1);
         }
@@ -396,7 +401,6 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
 
 
     //Running total of newly blinded outputs
-    static const unsigned char diff_zero[32] = {0};
     unsigned char blind[nToBlind][32];
     unsigned char asset_blind[nToBlind][32];
     secp256k1_pedersen_commitment commit;
@@ -512,7 +516,9 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
 
                 // Generate value we intend to insert
                 ret = secp256k1_pedersen_blind_generator_blind_sum(secp256k1_blind_context, &blindedAmounts[0], &assetblindptrs[0], &blindptrs[0], nBlindAttempts + nBlindsIn, nIssuanceBlindAttempts + nBlindsIn);
-                assert(ret);
+                if(ret != 1) {
+                    return -1;
+                }
 
                 // Resulting blinding factor can sometimes be 0
                 // where inputs are the negations of each other
@@ -522,7 +528,7 @@ int BlindTransaction(std::vector<uint256 >& input_blinding_factors, const std::v
                 // becomes just (r'), if this is 0, we can just
                 // abort and not blind and the math adds up.
                 // Count as success(to signal caller that nothing wrong) and return early
-                if (memcmp(diff_zero, &blind[nBlindAttempts-1][0], 32) == 0) {
+                if (memcmp(zero_scalar, &blind[nBlindAttempts-1][0], 32) == 0) {
                    return ++nSuccessfullyBlinded;
                 }
             }

[apoelstra]

The check is removed upstream; we could just update the version of secp256k1-zkp in elements now.

[dgpv]

The updated secp-zkp fixes the crash via CHECK, what is within the secp lib, but the asserts in Elements code are still there,

elements/src/blind.cpp

Line 522 in ee642f5

assert(ret);

will cause crash if invalid value blinder is supplied via RPC, and

elements/src/blind.cpp

Line 287 in ee642f5

assert(ret == 1);

that will cause crash if invalid asset blinder is supplied

I think the asserts should be replaced with checks that return -1 if ret is 0

[instagibbs]

Yes this should be reopened.

…

On Fri, Jun 7, 2019, 7:41 PM Dmitry Petukhov ***@***.***> wrote: The updated secp-zkp fixes the crash via CHECK, what is within the secp lib, but the asserts in Elements code are still there, https://github.com/ElementsProject/elements/blob/ee642f5862df4835e6c4c74739c0a950264b6828/src/blind.cpp#L522 will cause crash if invalid value blinder is supplied via RPC, and https://github.com/ElementsProject/elements/blob/ee642f5862df4835e6c4c74739c0a950264b6828/src/blind.cpp#L287 that will cause crash if invalid asset blinder is supplied — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#540?email_source=notifications&email_token=ABMAFU7UFZFX6ZOZE4VYBRDPZKMTLA5CNFSM4HBZ5362YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGQJPQ#issuecomment-499975358>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMAFU2JEFKKUT5HSQU3N23PZKMTLANCNFSM4HBZ536Q> .

[dgpv]

@stevenroose can you reopen this, please ? The original issue is not yet resolved

[stevenroose]

I'm adding that test case to the functional test. I agree we shouldn't assert validity of keys when we get them from the user..

[instagibbs]

@stevenroose thanks!

[stevenroose]

So #661 covers those two asserts and those are indeed the ones hit when providing incorrect blinders.

The blind.cpp file is full of asserts, most of them seem to be from self-generated information, though. Luckily that file is only exposed to RPC users.

[instagibbs]

UnblindConfidentialPair is exposed via wallet scanning.

[stevenroose]

Yeah UnblindConfidentialPair is the only one without asserts :)