f serialize s2c_opening as 33 bytes instead of 34

Author: jonasnick

include/secp256k1.h

@@ -477,27 +477,27 @@ SECP256K1_API int secp256k1_ecdsa_signature_serialize_compact(
  *  Args:    ctx: a secp256k1 context object.
  *  Out: opening: pointer to an opening object. If 1 is returned, it is set to a
  *                 parsed version of input. If not, its value is undefined.
- *  In:  input34: pointer to 34-byte array with a serialized opening
+ *  In:  input33: pointer to 33-byte array with a serialized opening
  *
  */
 SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_s2c_opening_parse(
     const secp256k1_context* ctx,
     secp256k1_s2c_opening* opening,
-    const unsigned char *input34
+    const unsigned char *input33
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
 
 /** Serialize a sign-to-contract opening into a byte sequence.
  *
  *  Returns: 1 if the opening was successfully serialized.
  *           0 if the opening was not initializaed.
  *  Args:     ctx: a secp256k1 context object.
- *  Out: output34: pointer to a 34-byte array to place the serialized opening
+ *  Out: output33: pointer to a 33-byte array to place the serialized opening
  *                 in.
  *  In:   opening: a pointer to an initialized `secp256k1_s2c_opening`.
  */
 SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_s2c_opening_serialize(
     const secp256k1_context* ctx,
-    unsigned char *output34,
+    unsigned char *output33,
     const secp256k1_s2c_opening* opening
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
 

src/secp256k1.c

@@ -840,26 +840,47 @@ static int secp256k1_s2c_commit_is_init(const secp256k1_s2c_opening *opening) {
     return opening->magic == s2c_opening_magic;
 }
 
-int secp256k1_s2c_opening_parse(const secp256k1_context* ctx, secp256k1_s2c_opening* opening, const unsigned char *input34) {
+/* s2c_opening is serialized as 33 bytes containing the compressed original pubnonce. In addition to
+ * holding the EVEN or ODD tag, the first byte has the third bit set to 1 if the nonce was negated.
+ * The remaining bits in the first byte are 0. */
+int secp256k1_s2c_opening_parse(const secp256k1_context* ctx, secp256k1_s2c_opening* opening, const unsigned char *input33) {
+    unsigned char pk_ser[33];
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(opening != NULL);
-    ARG_CHECK(input34 != NULL);
+    ARG_CHECK(input33 != NULL);
 
     secp256k1_s2c_opening_init(opening);
-    opening->nonce_is_negated = input34[0];
-    return secp256k1_ec_pubkey_parse(ctx, &opening->original_pubnonce, &input34[1], 33);
+    /* Return 0 if unknown bits are set */
+    if ((input33[0] & ~0x06) != 0) {
+        return 0;
+    }
+    /* Read nonce_is_negated bit */
+    opening->nonce_is_negated = input33[0] & (1 << 2);
+    memcpy(pk_ser, input33, sizeof(pk_ser));
+    /* Unset nonce_is_negated bit to allow parsing the public key */
+    pk_ser[0] &= ~(1 << 2);
+    return secp256k1_ec_pubkey_parse(ctx, &opening->original_pubnonce, &pk_ser[0], 33);
 }
 
-int secp256k1_s2c_opening_serialize(const secp256k1_context* ctx, unsigned char *output34, const secp256k1_s2c_opening* opening) {
+int secp256k1_s2c_opening_serialize(const secp256k1_context* ctx, unsigned char *output33, const secp256k1_s2c_opening* opening) {
     size_t outputlen = 33;
 
     VERIFY_CHECK(ctx != NULL);
-    ARG_CHECK(output34 != NULL);
+    ARG_CHECK(output33 != NULL);
     ARG_CHECK(opening != NULL);
     ARG_CHECK(secp256k1_s2c_commit_is_init(opening));
 
-    output34[0] = opening->nonce_is_negated;
-    return secp256k1_ec_pubkey_serialize(ctx, &output34[1], &outputlen, &opening->original_pubnonce, SECP256K1_EC_COMPRESSED);
+    if (!secp256k1_ec_pubkey_serialize(ctx, &output33[0], &outputlen, &opening->original_pubnonce, SECP256K1_EC_COMPRESSED)) {
+        return 0;
+    }
+    /* Verify that ec_pubkey_serialize only sets the first two bits of the
+     * first byte, otherwise this function doesn't make any sense */
+    VERIFY_CHECK(output33[0] == 0x02 || output33[0] == 0x03);
+    if (opening->nonce_is_negated) {
+        /* Set nonce_is_negated bit */
+        output33[0] |= (1 << 2);
+    }
+    return 1;
 }
 
 #ifdef ENABLE_MODULE_ECDH

src/tests.c

@@ -4183,13 +4183,13 @@ void run_eckey_edge_case_test(void) {
     secp256k1_context_set_illegal_callback(ctx, NULL, NULL);
 }
 
-
 void run_s2c_opening_test(void) {
     int i = 0;
-    unsigned char output[34];
-    unsigned char input[34] = {
-            0x01,
-            0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    unsigned char output[33];
+    /* First byte 0x06 means that nonce_is_negated and EVEN tag for the
+     * following compressed pubkey (which is valid). */
+    unsigned char input[33] = {
+            0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
             0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
             0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
             0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -4210,12 +4210,20 @@ void run_s2c_opening_test(void) {
     /* First parsing, then serializing works */
     CHECK(secp256k1_s2c_opening_parse(ctx, &opening, input) == 1);
     CHECK(secp256k1_s2c_opening_serialize(ctx, output, &opening) == 1);
+    CHECK(secp256k1_s2c_opening_parse(ctx, &opening, input) == 1);
 
     {
         /* Invalid pubkey makes parsing fail */
-        unsigned char input_tmp[34];
+        unsigned char input_tmp[33];
         memcpy(input_tmp, input, sizeof(input_tmp));
-        input_tmp[33] = 0;
+        /* Pubkey oddness tag is invalid */
+        input_tmp[0] = 0;
+        CHECK(secp256k1_s2c_opening_parse(ctx, &opening, input_tmp) == 0);
+        /* nonce_is_negated bit is set but pubkey oddness tag is invalid */
+        input_tmp[0] = 5;
+        CHECK(secp256k1_s2c_opening_parse(ctx, &opening, input_tmp) == 0);
+        /* Unknown bit is set */
+        input_tmp[0] = 8;
         CHECK(secp256k1_s2c_opening_parse(ctx, &opening, input_tmp) == 0);
     }
 
@@ -4225,13 +4233,13 @@ void run_s2c_opening_test(void) {
          * points' x-coordinates are uniformly random */
         if (secp256k1_s2c_opening_parse(ctx, &opening, input) == 1) {
             CHECK(secp256k1_s2c_opening_serialize(ctx, output, &opening) == 1);
-            CHECK(memcmp(output, input, 34) == 0);
+            CHECK(memcmp(output, input, sizeof(output)) == 0);
         }
-        secp256k1_rand256(input);
-        /* nonce_is_negated */
-        input[0] = input[0] & 1;
-        /* oddness */
-        input[1] = (input[1] % 2) + 2;
+        secp256k1_rand256(&input[1]);
+        /* Set pubkey oddness tag to first bit of input[1] */
+        input[0] = (input[1] & 1) + 2;
+        /* Set nonce_is_negated bit to input[1]'s 3rd bit */
+        input[0] |= (input[1] & (1 << 2));
         i++;
     } while(i < count);
 }