From a809ac484f8ae768f62d88bdfa67931eca967197 Mon Sep 17 00:00:00 2001
From: Jonas Nick <jonasd.nick@gmail.com>
Date: Tue, 27 Jun 2017 12:14:29 +0200
Subject: [PATCH] Fix checks of whitelist serialize/parse arguments

---
 include/secp256k1_whitelist.h      |  2 +-
 src/modules/whitelist/main_impl.h  |  5 +++
 src/modules/whitelist/tests_impl.h | 53 +++++++++++++++++++++++-------
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/include/secp256k1_whitelist.h b/include/secp256k1_whitelist.h
index 19412883..e1e17022 100644
--- a/include/secp256k1_whitelist.h
+++ b/include/secp256k1_whitelist.h
@@ -88,7 +88,7 @@ SECP256K1_API int secp256k1_whitelist_signature_serialize(
     unsigned char *output,
     size_t *output_len,
     const secp256k1_whitelist_signature *sig
-) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
 
 /** Compute a whitelist signature
  * Returns 1: signature was successfully created
diff --git a/src/modules/whitelist/main_impl.h b/src/modules/whitelist/main_impl.h
index 7445d6e3..8ac93ded 100644
--- a/src/modules/whitelist/main_impl.h
+++ b/src/modules/whitelist/main_impl.h
@@ -141,6 +141,10 @@ int secp256k1_whitelist_signature_parse(const secp256k1_context* ctx, secp256k1_
     ARG_CHECK(sig != NULL);
     ARG_CHECK(input != NULL);
 
+    if (input_len == 0) {
+        return 0;
+    }
+
     sig->n_keys = input[0];
     if (sig->n_keys >= MAX_KEYS || input_len != 1 + 32 * (sig->n_keys + 1)) {
         return 0;
@@ -153,6 +157,7 @@ int secp256k1_whitelist_signature_parse(const secp256k1_context* ctx, secp256k1_
 int secp256k1_whitelist_signature_serialize(const secp256k1_context* ctx, unsigned char *output, size_t *output_len, const secp256k1_whitelist_signature *sig) {
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(output != NULL);
+    ARG_CHECK(output_len != NULL);
     ARG_CHECK(sig != NULL);
 
     if (*output_len < 1 + 32 * (sig->n_keys + 1)) {
diff --git a/src/modules/whitelist/tests_impl.h b/src/modules/whitelist/tests_impl.h
index dcaf3baa..647e237b 100644
--- a/src/modules/whitelist/tests_impl.h
+++ b/src/modules/whitelist/tests_impl.h
@@ -63,6 +63,7 @@ void test_whitelist_end_to_end(const size_t n_keys) {
         CHECK(secp256k1_whitelist_verify(ctx, &sig, offline_pubkeys, online_pubkeys, &sub_pubkey) != 1);
         /* Serialization round trip */
         CHECK(secp256k1_whitelist_signature_serialize(ctx, serialized, &slen, &sig) == 1);
+        CHECK(slen == 33 + 32 * n_keys);
         CHECK(secp256k1_whitelist_signature_parse(ctx, &sig1, serialized, slen) == 1);
         /* (Check various bad-length conditions) */
         CHECK(secp256k1_whitelist_signature_parse(ctx, &sig1, serialized, slen + 32) == 0);
@@ -87,23 +88,53 @@ void test_whitelist_end_to_end(const size_t n_keys) {
 }
 
 void test_whitelist_bad_parse(void) {
-    const unsigned char serialized[] = {
-        /* Hash */
-        0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
-        0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
-        0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
-        0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
-        /* Length in excess of maximum */
-        0x00, 0x00, 0x01, 0x00
-        /* No room for s-values; parse should be rejected before reading past length */
-    };
     secp256k1_whitelist_signature sig;
 
-    CHECK(secp256k1_whitelist_signature_parse(ctx, &sig, serialized, sizeof(serialized)) == 0);
+    const unsigned char serialized0[] = { 1+32*(0+1) };
+    const unsigned char serialized1[] = {
+        0x00,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
+    };
+    const unsigned char serialized2[] = {
+        0x01,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07
+    };
+
+    /* Empty input */
+    CHECK(secp256k1_whitelist_signature_parse(ctx, &sig, serialized0, 0) == 0);
+    /* Misses one byte of e0 */
+    CHECK(secp256k1_whitelist_signature_parse(ctx, &sig, serialized1, sizeof(serialized1)) == 0);
+    /* Enough bytes for e0, but there is no s value */
+    CHECK(secp256k1_whitelist_signature_parse(ctx, &sig, serialized2, sizeof(serialized2)) == 0);
+}
+
+void test_whitelist_bad_serialize(void) {
+    unsigned char serialized[] = {
+        0x00,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07
+    };
+    size_t serialized_len;
+    secp256k1_whitelist_signature sig;
+
+    CHECK(secp256k1_whitelist_signature_parse(ctx, &sig, serialized, sizeof(serialized)) == 1);
+    serialized_len = sizeof(serialized) - 1;
+    /* Output buffer is one byte too short */
+    CHECK(secp256k1_whitelist_signature_serialize(ctx, serialized, &serialized_len, &sig) == 0);
 }
 
 void run_whitelist_tests(void) {
     int i;
+    test_whitelist_bad_parse();
+    test_whitelist_bad_serialize();
     for (i = 0; i < count; i++) {
         test_whitelist_end_to_end(1);
         test_whitelist_end_to_end(10);