EC_POINT_point2oct/EC_POINT_oct2point asymmetry (fixed in LibreSSL)

[guidovranken]

My fuzzer found an issue where for some points, EC_POINT_point2oct() succeeds, but the reverse operation using EC_POINT_oct2point() fails.

Fixed in LibreSSL: https://github.com/openbsd/src/commit/eec146bd45a3fe2bbd65928cb2797588727ad017

[slontis]

@romen Do you have any thoughts on this?

[romen]

It would be nice to have a reproducer.. The way I see it the patch should be changing only what error code is going on the stack: field_div should be able to recognize a div by zero and return an error, but in that case we skip to add EC_R_INVALID_ENCODING at the top of the error stack.

@guidovranken can you provide a snippet that triggers the issue? Also your description seems at odds with the linked patch: oct2point still fails for malformed hybrid encodings where X is zero, it just fails earlier. What is worrying in your description is that it suggests point2oct succeeds but delivers a malformed encoding, but that is not addressed by the linked patch.

I am not against applying the patch they used in LibreSSL which also has the merit of adding useful annotations, and makes things slightly more consistent.

@slontis How does that work for licensing and CLA?

[slontis]

@slontis How does that work for licensing and CLA?

No idea :)

[mattcaswell]

How does that work for licensing and CLA?

All authors must have a CLA on file.

[guidovranken]

It would be nice to have a reproducer.. The way I see it the patch should be changing only what error code is going on the stack: field_div should be able to recognize a div by zero and return an error, but in that case we skip to add EC_R_INVALID_ENCODING at the top of the error stack.

@guidovranken can you provide a snippet that triggers the issue? Also your description seems at odds with the linked patch: oct2point still fails for malformed hybrid encodings where X is zero, it just fails earlier. What is worrying in your description is that it suggests point2oct succeeds but delivers a malformed encoding, but that is not addressed by the linked patch.

I am not against applying the patch they used in LibreSSL which also has the merit of adding useful annotations, and makes things slightly more consistent.

@slontis How does that work for licensing and CLA?

Here's an example snippet

#include <openssl/ec.h>
#include <openssl/obj_mac.h>

static EC_POINT* copy(EC_GROUP* group, EC_POINT* pub, int form)
{
    size_t outSize;
    BN_CTX* ctx = BN_CTX_new();

    outSize = EC_POINT_point2oct(
            group,
            pub,
            form,
            NULL,
            0,
            ctx);

    uint8_t* out = malloc(outSize);
    if ( EC_POINT_point2oct(
                group,
                pub,
                form,
                out,
                outSize,
                ctx) != 0 ) {
        EC_POINT* tmpPoint = EC_POINT_new(group);
        if ( tmpPoint != NULL ) {
            if ( EC_POINT_oct2point(
                        group,
                        tmpPoint,
                        out,
                        outSize,
                        ctx) != 1 ) {
                printf("Conversion back from oct failed\n");
            }
            EC_POINT_free(pub);
            pub = tmpPoint;
        }
    }

    return pub;
}

int main(void)
{
    BIGNUM* x = BN_new();
    BIGNUM* y = BN_new();
    BN_dec2bn(&x, "0");
    BN_dec2bn(&y, "1");
    EC_KEY* key = EC_KEY_new();
    EC_GROUP* group = EC_GROUP_new_by_curve_name(NID_secp256k1);
    EC_KEY_set_group(key, group);
    EC_POINT* pub = EC_POINT_new(group);
    EC_POINT_set_affine_coordinates(group, pub, x, y, NULL);
    pub = copy(group, pub, POINT_CONVERSION_HYBRID);
    return 0;
}

Ping @botovq from LibreSSL

[botovq]

@guidovranken I think your example snippet has a different problem. The point (0, 1) is not on the curve secp256k1, so here EC_POINT_set_affine_coordinates fails and leaves a bogus point in pub. Consequently, point2oct produces nonsense that oct2point rightfully refuses to decode. A point-on-curve check in point2oct might be worth considering, but that's a different question.

Using NID_sect571k1 instead of NID_secp256k1 would give a reproducer for the problem I fixed.

@romen The corner case that is treated incorrectly is valid hybrid encodings of points with x = 0 on binary curves. This happens for the point (0, 1) on sect571k1, for example. The hybrid encoding of (0, y) must have y_bit unset in its point conversion form. point2oct handles this correctly, but oct2point fails because it tries to check y_bit against the last bit of (y / x) which of course errors for zero x:

https://github.com/openssl/openssl/blob/39da32729401110572da1782c80bef39c6f3f64b/crypto/ec/ec2_oct.c#L339-L342

I have not signed the CLA, but I can do that and send a PR your way.

[romen]

Thanks @botovq for the comment, I missed the point (pun intended) of the patch!

If you could sign the CLA and throw a patch our way it would be most welcome: actually this is a bug both in master and in 1.1.1 (the master patch won't cherry pick), so if you feel generous you could throw 2 PRs at us for the different branches (or I can take it upon myself to backport from master if you don't have the time for 2 PRs).

[botovq]

@romen I have created a PR against master. It should be trivial to backport to 1.1.1. I'll happily do that once that PR is reviewed and the CLA issue is settled (might take a week or two as I don't have easy access to a printer and scanner).

[romen]

• 15108 is now merged to master

• 15112 (the 1.1.1 backport) is waiting for a second review