Sign.signedMessageToKey() can't be called because the Sign.SignatureData class is not public. about web3j

[conor10]

Makes sense. You're welcome to submit a PR too :)

[androlo]

Ok I will.

[eztierney]

I ran into the same thing yesterday (though I was trying to use signMessage). I put up a simple PR to make SignatureData public: #65

[androlo]

I am making one with a function that takes signature as byte[], which means you don't really need access to any of the internal classes, and a convenience method signedMessageToAddress with the same parameters, but it returns the Ethereum address instead of the public key (so really the same as ecrecover). Going to add the tests today, or maybe tomorrow, and submit.

Did this in Solidity btw. so I'm used to Ethereum crypto (https://github.com/androlo/standard-contracts#crypto).

[androlo]

Seems the implementation is missing some fundamentals. For example, signatures are not properly validated before doing recovery. The only check i find is at the start of the recovery function (https://github.com/web3j/web3j/blob/master/src/main/java/org/web3j/crypto/Sign.java#L97), in which both r and s are (explicitly) allowed to lie outside of their respective ranges.

Here is an example of an invalid signature passing recovery without exceptions, running from within the Sign test file:

    @Test
    public void testInvalidSignature() throws SignatureException {
    	BigInteger signedMessageToKey = Sign.signedMessageToKey(TEST_MESSAGE, new SignatureData((byte) 27, new byte[]{1}, new byte[]{0})); // s=0
    	System.out.println(signedMessageToKey.toString(16)); // 60ce6c46097eb1d137670db055e7d8af842e368f265e2eabe572f89e704b58083893f06452e0cc9b8ee13f7677a8d00eef8ff8a28363fadb400c77f308980b60
    }

I use signature recovery in bank software (and there are deadlines etc) so will go with an alternative, for now, though I could maybe help with some work later, if needed.

Didn't go over the signing but I think recovery will automatically work if the signature is created by the library itself, since those signatures looks like they're always well-formed. I would not recommend supporting user-provided signatures though, until this has been addressed.

Will leave open since the comment about the other PR is in here.

[conor10]

So you'd like to see explicit bounds checked on e, v, r, and s, as per the yellow paper?

ECDSARECOVER(e ∈ B32, v ∈ B1, r ∈ B32, s ∈ B32)

Was there anything else?