ecdsa.ParseDERSignature() succeeds for an invalid signature (append arbitrary data)

[benma]

Appending garbage data to a valid DER signature does not fail ParseDERSignature().

See for example this modification of the unit tests, which does not fail the unit tests:

master...benma:btcd:der-sigs

The documentation of ParseDerSignature() does not mention anything like this. I would expect signatures longer than 72 bytes to be invalid and return an error.

Is this a bug?

[benma]

Wdyt @guggero ?

[Roasbeef]

Hi @benma,

In the normal execution path, any calls to ParseDERSignature are preceded by sanity checks implemented by checkSignatureEncoding. checkSignatureEncoding prevents such signatures from being accepted as it has a raw max size check. Other sanity checks including the BIP 66 low-s constraint is also verified in that routine.

As far as ParseDerSignature w.r.t your example above: the encoded length is unchanged (second byte in the signature), so the signature can still be parsed as valid. The encoded length is used as a guide when parsing, causing the appended bytes to be ignored.

In summary, such a signature would never be accepted as valid via normal execution paths. Precondition checks in the VM will cause such signatures to be rejected before we attempt to fully parse them.

[benma]

@Roasbeef thanks for the response!

I can see that internally for btcd, there is no problem. However, ParseDERSignature() is an exported function and part of the btcec library that users other than btcd use. I think it would make sense if the function by itself only accepts valid DER-encoded signatures, or otherwise document in the docstring that it parses it only from the beginning of a potentially larger byte slice so that there are no surprises. Is there any downside into having ParseDERSignature() be more strict and fail for invalid sigs?

Out of curiosity (maybe also document that in the docstring if it makes sense): why does ecdsa.Signature wrap github.com/decred/dcrd/dcrec/secp256k1/v4/ecdsa.Signature but has a custom implementation of ParseDERSIgnature(). Why not use the upstream implementation, which seems to also check the maximum signature size?

[Roasbeef]

Is there any downside into having ParseDERSignature() be more strict and fail for invalid sigs?

I don't think so, we'd accept a PR to do so.

but has a custom implementation of ParseDERSIgnature(). Why not use the upstream implementation, which seems to also check the maximum signature size?

At the time of porting, it wasn't clear that the implementations matched up 100%, so we kept our existing logic, as changes to this function may end up triggering a consensus divergence in the absolute worst case (we reject parsing, other nodes accept one). I think we can consider switching to that version, but we'd want to do some extensive differential fuzzing first.

[SulaimanAminuBarkindo]

Hi @Roasbeef ,

Is this issue regarding the ParseDERSignature() function still open? Also, would it be acceptable to make it stricter to only accept valid DER-encoded signatures? If so, I would love to contribute.

[Roasbeef]

@SulaimanAminuBarkindo sure, we'd accept a contribution to make the external facing usage more consitent.

[SulaimanAminuBarkindo]

I will be working towards it.

[SulaimanAminuBarkindo]

Hi @Roasbeef,

I've started addressing the issue with ParseDERSignature() by adding stricter validation for signature lengths. I've opened a draft PR to showcase the updates so far.

Before proceeding, I'd appreciate your guidance on the scope of changes needed to ensure consistency and strictness in the function's behavior. Specifically, I'm focusing on enforcing stricter validation of signature lengths but want to ensure I'm addressing all relevant aspects of the issue.

Could you provide clarity on any additional changes or considerations we need to make? Are there other areas of the function's behavior we should improve or make more consistent?

Your insights would be greatly appreciated as I continue working on this. Looking forward to your feedback.

[guggero]

@SulaimanAminuBarkindo I looked at the other sanity checks we have and it looks like the maximum size check is the main thing that was missing.

[SulaimanAminuBarkindo]

Thanks for reviewing, @guggero I'll go ahead and switch the PR from draft to ready for review.