Page MenuHomePhabricator

Refactor signature encoding checks
ClosedPublic

Authored by Mengerian on Feb 1 2019, 13:19.

Details

Summary

Isolate ECDSA-specific parts in preparation for Schnorr.

Next steps would be:

  • Disallow 64 byte ECDSA signatures in CheckRawECDSASignatureEncoding when SCRIPT_ENABLE_SCHNORR flag is set (D2469)
  • Add Schnorr-specific functions for checking signature encodings
Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sigencoding
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4813
Build 7689: Bitcoin ABC Buildbot (legacy)
Build 7688: arc lint + arc unit

Event Timeline

Herald added a subscriber: schancel. ยท View Herald Transcript
markblundeberg added inline comments.
src/script/sigencoding.cpp
226 โ†—(On Diff #7090)

This is backwards. When I add Schnorr, I can't put if (Schnorrflag & Size == 65) return true; at the start of this. Even Schnorr signatures need to have a valid hashtype byte, so before returning true we must first check the hashtype byte.

CheckTransactionECDSASignatureEncoding however must have if (Schnorrflag & Size == 65) return false; at the start since it gets directly called from CHECKMULTISIG. Since it's the inner function for CheckTransactionSignatureEncoding this would then also block schnorr signatures for CHECKSIG too.

This revision now requires changes to proceed.Feb 1 2019, 13:39

(CheckTransactionECDSASignatureEncoding could also be called CheckTransactionSignatureEncodingButNo65ByteSignaturesAllowedIfSchnorrEnabled, at least that is the function that CHECKMULTISIG must call per specification.)

@markblundeberg Have a look at D2478. Does it make more sense with that one added after?

I split the refactor into two, this one to pave the way for banning 64-byte ECDSA signatures, and D2478 to make it easy to add Schnorr.

My idea is to add the code to restrict 64-byte signature inside CheckRawECDSASignatureEncoding, and add the code allowing 64-byte signatures (with no DER checks) in CheckRawSignatureEncoding

Maybe it would be less confusing if I combine the diffs into one.

I think I will combine D2478 with this Diff.

It seems splitting them just made it more confusing

@markblundeberg Have a look at D2478. Does it make more sense with that one added after?

I split the refactor into two, this one to pave the way for banning 64-byte ECDSA signatures, and D2478 to make it easy to add Schnorr.

My idea is to add the code to restrict 64-byte signature inside CheckRawECDSASignatureEncoding, and add the code allowing 64-byte signatures (with no DER checks) in CheckRawSignatureEncoding

Maybe it would be less confusing if I combine the diffs into one.

OK yup, the end result after the second diff looks good ๐Ÿ‘ .

Combine D2478 in this Diff.
it's less confusing this way

OK looks good, I added a couple of comments showing how this will have to integrate with the schnorr flag

src/script/sigencoding.cpp
174 โ†—(On Diff #7093)

my diffs will add just before this line
if(size==64 & schnorr) return true;

218 โ†—(On Diff #7093)

my diffs will add here:
if(size==64 & schnorr) set error & return false;

This revision is now accepted and ready to land.Feb 1 2019, 15:26
src/script/sigencoding.cpp
218 โ†—(On Diff #7093)

correction: size == 65

src/script/sigencoding.cpp
174 โ†—(On Diff #7093)

Yes, agreed.

218 โ†—(On Diff #7093)

I was actually imagining the 64-byte ban being inside CheckRawECDSASignatureEncoding

But it works in either place.

src/script/sigencoding.cpp
218 โ†—(On Diff #7093)

Oh! Good point, yes that works too, even better...

deadalnix requested changes to this revision.Feb 1 2019, 16:50
deadalnix added inline comments.
src/script/sigencoding.cpp
27 โ†—(On Diff #7093)

It has nothing to do with ECDSA, it is DER encoding. Please adjust the name accordingly. CheckRawECDSASignatureEncoding is the only part doing anything ECDSA specific.

244 โ†—(On Diff #7093)

Because both have the exact same structure, as the exception of the CheckRawSignatureEncoding call, it's probaby a good idea to build a template.

template<typename F> static bool CheckTransactionSignatureEncodingImpl(const valtype &vchSig, uint32_t flags, ScriptError *serror, F fun) {
  // Empty signature. Not strictly DER encoded, but allowed to provide a
  // compact way to provide an invalid signature for use with CHECK(MULTI)SIG
  if (vchSig.size() == 0) {
      return true;
  }

  if (!fun(vchSig | boost::adaptors::sliced(0, vchSig.size() - 1), flags, serror)) {
      // serror is set
      return false;
  }

  return CheckSighashEncoding(vchSig, flags, serror);
}

bool CheckTransactionSignatureEncoding(const valtype &vchSig, uint32_t flags,
                                     ScriptError *serror) {
  return CheckTransactionSignatureEncodingImpl(vchSig, flags, serror,
      [](const slicedvaltype &sig, uint32_t flags, ScriptError *serror) {
        return CheckRawSignatureEncoding(sig, flags, serror);
      });
}

And so on.

This revision now requires changes to proceed.Feb 1 2019, 16:50

rename IsValidECDSASignatureEncoding to IsValidDERSignatureEncoding

Mengerian marked an inline comment as done.

Use template for CheckTransactionSignatureEncoding and
CheckTransactionECDSASignatureEncoding

Mengerian marked an inline comment as done.EditedFeb 1 2019, 18:47

OK, I put in the template to implement CheckTransactionSignatureEncoding and CheckTransactionECDSASignatureEncoding

src/script/sigencoding.cpp
27 โ†—(On Diff #7093)

OK, renamed to IsValidDERSignatureEncoding

244 โ†—(On Diff #7093)

OK, I got the template working.

The compiler didn't like the parameter names (vchSig, flags, serror) repeated, so I used the names "templateSig", "templateFlags", and "templateSerror" because I couldn't think of anything better. Let me know if you would prefer other variable names.

This revision is now accepted and ready to land.Feb 1 2019, 22:22
This revision was automatically updated to reflect the committed changes.
Mengerian marked an inline comment as done.