Insufficient BLS signature validation for lodestar+bls/herumi

[asanso]

The Lodestar's BLS signature validation when used in combination with bls/herumi is not sufficient.

The code assumes

   // Signatures all come from the wire (untrusted) are all bytes compressed, must be:
    // - Parsed from bytes
    // - Uncompressed
    // - subgroup_check
    // - consume in Pairing.aggregate as affine, or mul_n_aggregate as affine
    // Just send the raw signture recevied as bytes to the thread and verify there

This assumption is true for bls/blst where the validate parameter is used. But not in the bls/herumi case

[g11tech]

hi @dapplion, as per my analysis, bls/blst's validate is checking if the signature is a (G2) group point. bls/herumi is checking the order of the signature point w.r.t. G2 order (in signature constructor: isValidOrder ). will continue digging.

[dapplion]

hi @dapplion, as per my analysis, bls/blst's validate is checking if the signature is a (G2) group point. bls/herumi is checking the order of the signature point w.r.t. G2 order (in signature constructor: isValidOrder ). will continue digging.

@g11tech Can you try to validate this test vector?

export const POINT_NOT_IN_G2 = Buffer.from(
  "8123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
  "hex"
);

[g11tech]

this point fails sigValidate of bls/blst but passes isValidOrder of bls/herumi, which i guess is only checking the order of the point and not its group membership (of G2).

On further digging bls/herumi uses "bls-eth-wasm" under the hood to wrap the bls functionality in webassembley. however its signature class doesn't export any group validation method, just the order validation:

https://github.com/herumi/bls-eth-wasm/blob/master/src/bls.js#L550-L580

I will further keep digging through.

[dapplion]

this point fails sigValidate of bls/blst but passes isValidOrder of bls/herumi, which i guess is only checking the order of the point and not its group membership (of G2).

On further digging bls/herumi uses "bls-eth-wasm" under the hood to wrap the bls functionality in webassembley. however its signature class doesn't export any group validation method, just the order validation:

https://github.com/herumi/bls-eth-wasm/blob/master/src/bls.js#L550-L580

I will further keep digging through.

From Antonio's messages:

this is how prysm used to do [group verification] (when they had support for herumi)

https://github.com/prysmaticlabs/prysm/blob/develop/shared/bls/herumi/init.go#L13

[g11tech]

ok, this looks easy solve:

accidentally tried the POINT_NOT_IN_G2 test vector in the lodestar repository and realized that fromBytes was throwing error there for herumi as well, tried comparing the installed node_module's @chainsafe/eth with the @chainsafe/eth codebase.
finally figured out that yarn.lock of lodestar is using 0.4.8 (while package has ^0.4.4)

while chainsafe/eth code base's yarn.lock is using 0.4.4.

digging through the releases of bls-eth-wasm and figured out that that the underlying bls code was updated in bls-eth-wasm's 0.4.8.

locally updated the package to 0.4.8 in chainsafe/bls and now the fromBytes of chainsafe/bls also starts throwing error on the wasm's signature deserialize call:
previously in chainsafe/bls codebase (bls-eth-wasm: 0.4.4) which accepted this signature point:

post: yarn-upgrade bls-eth-wasm trying to build signature from this signature point throws error

will send a PR to update the yarn.lock and add this test in testcases in @chainsafe/bls

[asanso]

thanks @g11tech .
Question are you also passing validate =true or is it the default in https://github.com/ChainSafe/bls/blob/master/src/interface.ts#L42

[g11tech]

for bls/herumi this param is not even used and its signature deserialize request from the provided data itself fails (on updated bls-eth-wasm 0.4.8 which chainsafe/bls package uses) so it seems its always being validated inside the wasm module in the updated version:

in previous bls-eth-wasm version of 0.4.4 this call was success returning the signature.

[asanso]

so it seems its always being validated inside the wasm module in the updated version

well this is nice from the security point of view but it might break the semantic of the method (what if you pass validate=false or omit the parameter that AFAIK defaults to false?)

[g11tech]

well this is nice from the security point of view but it might break the semantic of the method (what if you pass validate=false or omit the parameter that AFAIK defaults to false?)

unfortunately herumi bls-eth-wasm API doesn't provide this option. But imho, semantically if not in G2 implies not a signature (for eth) and hence shouldn't be loadable into the signature structure.

Do you think there is a use of loading an invalid Signature point? my guess is not doing validation would be for the purposes of speed where one is sure of the correctness of signature.

[asanso]

Do you think there is a use of loading an invalid Signature point?

no but at this point i would remove the boolean from the signature of the method otherwise is a bug

[g11tech]

I can make it to default true, those who would want to skip validation could be explicit to pass false but will make it throw error in herumi and hence only work in blst

@dapplion also, let know what you would think interface/behavior should be?

(updated herumi wasm always does validation)

[asanso]

I can make it to default true,

nice this will also solve stuff like #2555

[g11tech]

@asanso PR for the above resolution in the chainsafe/bls package. Have also added a test point for invalid signature for not in G2.
ChainSafe/bls#106

Will create send another one for lodestar post merge of above for package update and making the validation flag to explicit true for readability purposes. 👍