BIP-0062

[mvandeberg]

FC Implementation for BIP-0062 signature canonization.

Discussion in steemit/steem#1944

[goldibex]

just those comments

[goldibex]

missing eol

[goldibex]

I would really strongly recommend that we use a bignum library rather than perform bespoke bitwise arithmetic. Does steemd not have access to a bignum library (libgmp, etc.) that could do this heavy lifting? Alternatively, the secp256k1_fe type may be suitable. The library definitely has a constant-time comparison (secp256k1_fe_equal); but I don't know whether it implements >.

Also, i'm not sure if it matters, but this is not a constant-time comparison. to do that you would want something like the following:

uint8_t greater = 0;
uint8_t less = 0;
uint8_t a, b;

for (size_t i = 1; i < 31; i++) {
  a = c.data[33+i];
  b = n_2[i];
  greater |= (b - a) & ~less;
  less |= (a - b) & ~greater;
}
return ((greater | ~less) >> 7) != 0 && c.data[64] <= n_2[31];

If you want to completely exclude timing leaks you'd need to rearrange the final compound conditional into something bitwise.

[mvandeberg]

Also, i'm not sure if it matters, but this is not a constant-time comparison.

While you can make the argument that my algorithm is O(n) it is bounded about by n=31, same as yours. Mine can simply short circuit as soon as we see the first byte is < 0x7F.

Any bignum library we use will require converting the signature to native endian for comparison. This algorithm skips all of that by doing a byte-wise comparison.

The naive, but correct solution would be this:

const static boost::uint256_t n_2 = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0_cppui256;

boost::uint256_t sig = 0;

for( size_t = 0; i < 4; i++ )
{
   sig << 32;
   sig += boost::endian::little_to_native( *( uint64_t* )( c.data + 33 + ( i * 8 ) ) );
}

return sig <= n_2;

My preference would be readability. Even with thousands of extra signatures, the unit tests using the old canonical code only takes 2 more seconds to execute. As a percentage of our execution time, signature verification is not that long. I was previously told by Dan that it was significant overhead, but reindexing the blockchain with full signature verification maybe only added a minute of reindex time. At a 1 hour reindex time, the difference is almost not perceptible. We don't normally do signature verification during reindex. The big performance hit is while live and during a sync. Even then. undo states are the killer there and cause the order of magnitude slowdown.

[goldibex]

Sorry, as far as regards the comparison: the goal for me is not performance, it's security. Any comparison is always going to be O(n) in the length of the longer string. I was saying that your proposed implementation leaks timing information by short-circuiting. Not sure if that's even relevant here; just wanted to point it out.

Any bignum library we use will require converting the signature to native endian for comparison

Yes. For a minimal performance penalty you would gain a lot in readability and safety.

[mvandeberg]

@goldibex Updates have been pushed to use boost uint256_t

[goldibex]

@theoreticalbts should take a look, but lgtm

[sneak]

Are these test vectors from BIP62 itself?

I would like an external review/sanity check on this before it's merged.

[theoreticalbts]

According to here it says BIP-0062 is "withdrawn," is that concerning?

[theoreticalbts]

I am not familiar with this syntax:

constexpr boost::multiprecision::uint256_t n_2 = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0_cppui256;

Where is this syntax documented? Is it compatible with gcc? I can't find any information about it in Google.

I suggest avoiding this syntax and initializing this value from an array of some built-in integer type.

[theoreticalbts]

OK, now I have issue with the initialization of sig. It is initialized from the data at c.data + 33 which is a 256-bit value stored in big-endian form:

sig += boost::endian::big_to_native( *( uint64_t* )( c.data + 33 + ( i * 8 ) ) );

Where is the of compact_signature documented? How do you know that the value at offset 33 is a 256-bit big-endian value? What is the meaning of that value?

All we have is:

typedef fc::array<unsigned char,65> compact_signature;

[theoreticalbts]

I did some more reading of the code. In the openssl version of the code here it does:

auto bytes = i2d_ECDSA_SIG( sig, &result );

This seems fishy, first of all because bytes is ignored. It should check for unexpectedly short/long sizes, or 0 for error.

Anyway, if we combine with the DER encoding reference conveniently located in BIP 0062, we can get this reference for the offsets:

0 : 0x30
1 : [total-length]
2 : 0x02
3 : [R-length]
4 : [R]
4 + R-length : 0x02
5 + R-length : [S-length]
6 + R-length : [S]
6 + R-length + S-length : [sighash-type] 

So this explains 64 out of 65 bytes are 32-byte r and s values. What is the first byte?

It is initialized here to nRecId+27+4, which gives us three questions:

• (a) What is nRecId?
• (b) What is 27?
• (c) What is 4?

Questions (b)-(c) don't seem to be answerable unless we can get more information on the exact specification of compact_signature. Again, the only information about this which I could find in the fc source is:

typedef fc::array<unsigned char,65> compact_signature;

Turning now to question (a), we can see that nRecId is initialized by this loop which tries to call ECDSA_SIG_recover_key_GFp with four different values of the fifth parameter, and records the first i value which results in a successful recovery.

This code raises a lot of questions. First of all, several ignored return values and the comment on this line do not exactly inspire confidence.

Looking at the openssl docs the function ECDSA_SIG_recover_key_GFp does not seem to exist. After some grepping around, despite being named with the OpenSSL naming scheme, the function is actually a very long and confusing sequence of abstract algebra implemented in fc itself!

From a wider perspective, there are plenty of questions as well. Why might the key recovery sometimes fail? Why does it try four values 0...3? Why not one value, or five values, or a thousand values? Why values 0..3? Would four other integer values like 137, 200, 355, 999 work just as well? It is not obvious from the sequence of abstract algebra in ECDSA_SIG_recover_key_GFp what the purpose of recid is.

[mvandeberg]

I am going to try to answer as many questions as I can. They aren't going in a single comment, so bear with me.

[mvandeberg]

According to here it says BIP-0062 is "withdrawn," is that concerning?

I found this response on bitcoin talk that addresses this.

Also, this Bitcoin PR adds just the S requirement that we want to implement as a soft fork to Bitcoin clients.

[mvandeberg]

I am not familiar with this syntax:
constexpr boost::multiprecision::uint256_t n_2 = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0_cppui256;

It is a language extension of C++11. This specific use is documented by Boost here. It requires C++11, and gcc 4.7+ or clang 3.3+. All of the standards and versions are sufficiently old enough that I do not have a problem incorporating this syntax into the project.

[mvandeberg]

The documentation of secp256k1_ecdsa_sign_compact

/** Create a compact ECDSA signature (64 byte + recovery id).
 *  Returns: 1: signature created
 *           0: the nonce generation function failed, or the secret key was invalid.
 *  In:      ctx:    pointer to a context object, initialized for signing (cannot be NULL)
 *           msg32:  the 32-byte message hash being signed (cannot be NULL)
 *           seckey: pointer to a 32-byte secret key (cannot be NULL)
 *           noncefp:pointer to a nonce generation function. If NULL, secp256k1_nonce_function_default is used
 *           ndata:  pointer to arbitrary data used by the nonce generation function (can be NULL)
 *  Out:     sig:    pointer to a 64-byte array where the signature will be placed (cannot be NULL)
 *                   In case 0 is returned, the returned signature length will be zero.
 *           recid:  pointer to an int, which will be updated to contain the recovery id (can be NULL)
 */

And secp256k1_ecdsa_recover_compact

/** Recover an ECDSA public key from a compact signature.
 *  Returns: 1: public key successfully recovered (which guarantees a correct signature).
 *           0: otherwise.
 *  In:      ctx:        pointer to a context object, initialized for verification (cannot be NULL)
 *           msg32:      the 32-byte message hash assumed to be signed (cannot be NULL)
 *           sig64:      signature as 64 byte array (cannot be NULL)
 *           compressed: whether to recover a compressed or uncompressed pubkey
 *           recid:      the recovery id (0-3, as returned by ecdsa_sign_compact)
 *  Out:     pubkey:     pointer to a 33 or 65 byte array to put the pubkey (cannot be NULL)
 *           pubkeylen:  pointer to an int that will contain the pubkey length (cannot be NULL)
 */

Our array is (recovery id + 64 byte array). As documented here, the recovery id is in [0,3].

this stack exchange thread is the best I can find on the meaning of 27 + 4 + recovery id. Because we only use compact signatures, we offset the value by 31.

[mvandeberg]

Are these test vectors from BIP62 itself?

I was unable to find specific BIP62 test vectors. I am assuming it is because BIP62 itself was withdrawn. The bounds on S were enforced as a softfork in a later PR. (Mentioned above)

I would like an external review/sanity check on this before it's merged.

100% agree

[goldibex]

@theoreticalbts Please note that our signatures are _not_ DER-encoded. They use secp256k1's compact signature format (1-byte header, 32-byte r, 32-byte s). Note that r and s are 32-digit big-endian, twos-complement, base-256 numbers. They are _not_ DER bitstrings. This was Michael's powerful insight: the code of the current check is cargo-culted from BIP-0066. There the code assumes r and s are DER-encoded bitstrings. That's the source of the present pain. And 1000% agree with sneak that we need an external review here.

…

On Fri, Dec 29, 2017 at 8:16 AM Michael Vandeberg ***@***.***> wrote: Are these test vectors from BIP62 itself? I was unable to find specific BIP62 test vectors. I am assuming it is because BIP62 itself was withdrawn. The bounds on S were enforced as a softfork in a later PR. (Mentioned above) I would like an external review/sanity check on this before it's merged. 100% agree — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK7e63_TH8r8aOJE59ZzpKScvLevzQJks5tFRBhgaJpZM4RN7wk> .

[tarassh]

@goldibex What about loop it self, is it okay to generate signature in the loop? There is a function secp256k1_ecdsa_signature_normalize which should do it.

you can find more here: https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L461

[sneak]

Additionally, due to the sensitive and critical nature of this code, I believe we should refactor it with this change to be maximally readable/comprehendable whilst maximally avoiding footguns. I can weigh in further with specific recommendations if need be.