interpreter: Use safer casting in IsValidSignatureEncoding(...) #12611

Use safer casting in IsValidSignatureEncoding(...).

This was found when looking at the c-lightning codebase which contains a copy of this function.

Can you explain what makes this safer? The types inside the existing cast are already unsigned, and AFAIK size_t is at least as large as unsigned int so I don't see how this affects overflow semantics.

Consider the case:

unsigned int lenR = 4294967295; // std::numeric_limits<unsigned int>::max()
unsigned int lenS = 0;

On my system the following then holds true:

(size_t)lenR + (size_t)lenS + 7 == 4294967302
(size_t)(lenR + lenS + 7) == 6

The latter case is the result of an unsigned integer overflow since 4294967295 + 7 cannot be represented as an unsigned int.

(Luckily both lenR and lenS are guaranteed to be in the closed range [0, 255] as the code is currently written.)

I see, and now that you point it out it makes sense. This PR is probably not the place for it, but I'm curious what you think about making these kinds of semantics better defined (e.g. by using -fwrapv, or related flags).

@eklitzke Is that an utACK? :-)

Regarding -ftrapv for catching signed arithmetic overflows – see #12686.

utACK https://github.com/bitcoin/bitcoin/pull/12611/commits/332ecf9b497dda7d4faaea50768b104dd2666bf5

utACK 332ecf9b497dda7d4faaea50768b104dd2666bf5

@sipa @laanwj @theuni Would you mind reviewing? :-)

(Luckily both lenR and lenS are guaranteed to be in the closed range [0, 255] as the code is currently written.)

This code is basically one constraint after another, each enabling the safety of the next check. The very first constraints (sig size checks) pretty much rule out conversion issues further down. So IMO it's not so much "luckily" that way, so much as it is necessarily that way.

Consider the case:

unsigned int lenR = 4294967295; // std::numeric_limits<unsigned int>::max()
unsigned int lenS = 0;

In that case, we would've already gone off the rails with an OOB read before the code in question:

unsigned int lenS = sig[5 + lenR];

I think this change actually obscures the code because it's not clear at all why you'd cast those two values individually.

The intent would be much more clear if the sig.size() were cast and stored as a uint8_t, then used for the rest of the function for comparison rather than size(). At least then the ranges would be 100% obvious. I'm not actually advocating for that change, though.

IMO this is fine as-is. -0.

I have no opinion.

Just change lenRand lenS to unsigner char?

@theuni Do you mean like this?

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 2cdff7e..afcb6d5 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -121,24 +121,26 @@ bool static IsValidSignatureEncoding(const std::vector<unsigned char> &sig) {
     if (sig.size() < 9) return false;
     if (sig.size() > 73) return false;

+    uint8_t signatureSize = static_cast<uint8_t>(sig.size());
+
     // A signature is of type 0x30 (compound).
     if (sig[0] != 0x30) return false;

     // Make sure the length covers the entire signature.
-    if (sig[1] != sig.size() - 3) return false;
+    if (sig[1] != signatureSize - 3) return false;

     // Extract the length of the R element.
     unsigned int lenR = sig[3];

     // Make sure the length of the S element is still inside the signature.
-    if (5 + lenR >= sig.size()) return false;
+    if (5 + lenR >= signatureSize) return false;

     // Extract the length of the S element.
     unsigned int lenS = sig[5 + lenR];

     // Verify that the length of the signature matches the sum of the length
     // of the elements.
-    if ((size_t)(lenR + lenS + 7) != sig.size()) return false;
+    if ((size_t)(lenR + lenS + 7) != signatureSize) return false;

     // Check whether the R element is an integer.
     if (sig[2] != 0x02) return false;

Ping @theuni :-)

There hasn't been any activity in the last month and half of the reviewers consider the current code already 100% safe and easier to read. Could be closed, maybe?

@MarcoFalke In a comment above @theuni wrote "The intent would be much more clear if the sig.size() were cast and stored as a uint8_t". I agree with that and that's why I asked for a clarification before updating the PR and asking for a re-review.

The intent would be much more clear if the sig.size() were cast and stored as a uint8_t, then used for the rest of the function for comparison rather than size(). At least then the ranges would be 100% obvious. I'm not actually advocating for that change, though.

This code is safe as-is. While your proposed changes looks fine too, I just don't see any benefit in risking the change.

@theuni Thanks! Now closing :-)