Chia-Network/bls-signatures

Since recent commit, library allows loading invalid G1 point

guidovranken opened this issue · 5 comments

guidovranken commented

OSS-Fuzz issue: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36141

#include <relic_conf.h>
#include <bls.hpp>

int main(void)
{
    const std::vector<uint8_t> compressed = {0xBA, 0xD3, 0x9F, 0x0E, 0x40, 0x16, 0xD2, 0xCF, 0xAF, 0x53, 0xE0, 0x7B, 0xF8, 0x69, 0x89, 0xE5, 0x20, 0x6F, 0xBB, 0xCB, 0x19, 0x8F, 0x8A, 0x75, 0x1B, 0x30, 0x3F, 0xA2, 0x6E, 0xD9, 0x7D, 0xE7, 0xD3, 0xC0, 0x1E, 0x16, 0xC5, 0xA3, 0xFF, 0x9F, 0xDA, 0x23, 0xBC, 0x66, 0x52, 0x27, 0xB8, 0x2C};
    const auto G1 = bls::G1Element::FromBytes(::bls::Bytes(compressed));
    char g1str1[1024];
    char g1str2[1024];

    g1_t g1;
    G1.ToNative(g1);
    ep_norm(g1, g1);

    fp_write_str(g1str1, 1024, g1->x, 10);
    fp_write_str(g1str2, 1024, g1->y, 10);
    printf("%s\n%s\n", g1str1, g1str2);
    return 0;
}

Before 2dd0730 this would throw an exception, but now it prints:

126589124254035351430032133896859437490532361545310169422483711444917464845957755648618213790137282713188785720705
3528709069421201840454100343321618556956462755990806234474727904232292420508190339919994057278289579339323948967883

As far as I can tell, it should throw an exception; blst and py_ecc also refuse to load this point. Not throwing an exception is probably in violation of the BLS spec.

To reproduce this issue, on x64 Linux:

git clone --depth 1 https://github.com/Chia-Network/bls-signatures.git
cd bls-signatures/
mkdir build/
cd build/
cmake .. -DBUILD_BLS_PYTHON_BINDINGS=0 -DBUILD_BLS_TESTS=0 -DBUILD_BLS_BENCHMARKS=0
make -j$(nproc)
export CHIA_BLS_LIBBLS_A_PATH=$(realpath libbls.a)
export CHIA_BLS_INCLUDE_PATH=$(realpath ../src/)
export CHIA_BLS_RELIC_INCLUDE_PATH_1=$(realpath _deps/relic-build/include/)
export CHIA_BLS_RELIC_INCLUDE_PATH_2=$(realpath _deps/relic-src/include/)
cd ../../

and then

$CXX $CXXFLAGS -I $CHIA_BLS_INCLUDE_PATH -I $CHIA_BLS_RELIC_INCLUDE_PATH_1 -I $CHIA_BLS_RELIC_INCLUDE_PATH_2 poc.cpp $CHIA_BLS_LIBBLS_A_PATH

CC @dfaranha

hoffmang9 commented

Here is the delta in Relic - relic-toolkit/relic@1885ae...e6209f

dfaranha commented

I tracked down the issue inside RELIC, which accepted integers out of the canonical bounds [0, p-1] by first reducing modulo p. Sorry about that.

I just modified the behavior such that byte arrays must satisfy the bounds (as specified in the encoding standards), while strings are still handled flexibly. The latter is convenient for initializing curve coefficients, for example.

The PoC code now triggers an error with version 1620a03b388e50acd68ed9c88d7cd82ec5490ce4 of RELIC.

hoffmang9 commented

@dfaranha - It looks like your fix broke something additional - #248

dfaranha commented

Oh, will take a look tomorrow morning.

guidovranken commented

Assuming this is fixed, if not we will hear it from OSS-Fuzz and I will reopen the issue.