Apply suggestions from code review

Co-authored-by: Willem Toorop <willem@nlnetlabs.nl>

diff --git a/sldns/str2wire.c b/sldns/str2wire.c
index 275cc7e8c5759dd2f24c3c621508f0bf1ae9457c..abc55a7c1c6ed91796bc3882a0550338b4d90b62 100644 (file)
--- a/sldns/str2wire.c
+++ b/sldns/str2wire.c
@@ -626,7 +626,7 @@ static int sldns_str2wire_svcparam_key_cmp(const void *a, const void *b)
 static int sldns_str2wire_check_svcbparams(uint8_t* rdata, uint16_t rdata_len)
 {
        size_t   nparams = 0, i, j;
-       uint8_t  new_rdata[65536];
+       uint8_t  new_rdata[LDNS_MAX_RDFLEN];
        uint8_t* new_rdata_ptr = new_rdata;
        uint8_t* svcparams[64];
        uint8_t* mandatory = NULL;
@@ -650,7 +650,7 @@ static int sldns_str2wire_check_svcbparams(uint8_t* rdata, uint16_t rdata_len)
                rdata_ptr += svcbparam_len;
 
                nparams += 1;
-               if (nparams > sizeof(svcparams)/8)
+               if (nparams > MAX_NUMBER_OF_SVCPARAMS)
                        return LDNS_WIREPARSE_ERR_SVCB_TOO_MANY_PARAMS;
        }
 
@@ -684,7 +684,7 @@ static int sldns_str2wire_check_svcbparams(uint8_t* rdata, uint16_t rdata_len)
        if (mandatory) {
 
                /* Divide by sizeof(uint16_t)*/
-               uint16_t mandatory_len = sldns_read_uint16(mandatory + 2) >> 1;
+               uint16_t mandatory_len = sldns_read_uint16(mandatory + 2) / sizeof(uint16_t);
 
                /* Guaranteed by sldns_str2wire_svcparam_key_value */
                assert(mandatory_len > 0);
@@ -1360,6 +1360,8 @@ sldns_str2wire_svcbparam_ech_value(const char* val, uint8_t* rd, size_t* rd_len)
 
        /* single 0 represents empty buffer */
        if(strcmp(val, "0") == 0) {
+               if (*rd_len < 4)
+                       return LDNS_WIREPARSE_ERR_BUFFER_TOO_SMALL
                sldns_write_uint16(rd, SVCB_KEY_ECH);
                sldns_write_uint16(rd + 2, 0);
 
@@ -1399,6 +1401,9 @@ sldns_str2wire_svcbparam_parse_next_unescaped_comma(const char *val)
        return NULL;
 }
 
+/* The source is already properly unescaped, this double unescaping is purely to allow for
+ * comma's in comma seperated alpn lists.
+ */
 static size_t
 sldns_str2wire_svcbparam_parse_copy_unescaped(uint8_t *dst,
        const char *src, size_t len)
@@ -1422,12 +1427,12 @@ static int
 sldns_str2wire_svcbparam_alpn_value(const char* val,
        uint8_t* rd, size_t* rd_len)
 {
-       uint8_t     unescaped_dst[65536];
+       uint8_t     unescaped_dst[LDNS_MAX_RDFLEN];
        uint8_t    *dst = unescaped_dst;
        const char *next_str;
        size_t      str_len;
        size_t      dst_len;
-       size_t          val_len;
+       size_t      val_len;
        
        val_len = strlen(val);
 
@@ -1456,7 +1461,8 @@ sldns_str2wire_svcbparam_alpn_value(const char* val,
                val = next_str + 1;
        }
        dst_len = dst - unescaped_dst;
-
+       if (*rd_len < 4 + dst_len)
+               return LDNS_WIREPARSE_ERR_BUFFER_TOO_SMALL;
        sldns_write_uint16(rd, SVCB_KEY_ALPN);
        sldns_write_uint16(rd + 2, dst_len);
        memcpy(rd + 4, unescaped_dst, dst_len);
@@ -1486,6 +1492,8 @@ sldns_str2wire_svcparam_value(const char *key, size_t key_len,
                case SVCB_KEY_IPV6HINT:
                        return LDNS_WIREPARSE_ERR_SVCB_MISSING_PARAM;
                default:
+                       if (*rd_len < 4)
+                               return LDNS_WIREPARSE_ERR_BUFFER_TOO_SMALL;
                        sldns_write_uint16(rd, svcparamkey);
                        sldns_write_uint16(rd + 2, 0);
                        *rd_len = 4;
@@ -1512,6 +1520,8 @@ sldns_str2wire_svcparam_value(const char *key, size_t key_len,
                return sldns_str2wire_svcbparam_alpn_value(val, rd, rd_len);
        default:
                str_len = strlen(val);
+               if (*rd_len < 4 + str_len)
+                       return LDNS_WIREPARSE_ERR_BUFFER_TOO_SMALL;
                sldns_write_uint16(rd, svcparamkey);
                sldns_write_uint16(rd + 2, str_len);
                memcpy(rd + 4, val, str_len);
@@ -1527,7 +1537,7 @@ sldns_str2wire_svcparam_value(const char *key, size_t key_len,
 int sldns_str2wire_svcparam_buf(const char* str, uint8_t* rd, size_t* rd_len)
 {
        const char* eq_pos;
-       char unescaped_val[65536];
+       char unescaped_val[LDNS_MAX_RDFLEN];
        char* val_out = unescaped_val;
        const char* val_in;
 
@@ -1540,11 +1550,14 @@ int sldns_str2wire_svcparam_buf(const char* str, uint8_t* rd, size_t* rd_len)
                /* unescape characters and "" blocks */
                if (*val_in == '"') {
                        val_in++;
-                       while (*val_in != '"' && sldns_parse_char( (uint8_t*) val_out, &val_in)) {
+                       while (*val_in != '"'
+                       && val_out - unescaped_val < sizeof(unescaped_val) - 1
+                       && sldns_parse_char( (uint8_t*) val_out, &val_in)) {
                                val_out++;
                        }
                } else {
-                       while ( sldns_parse_char( (uint8_t*) val_out, &val_in)) {
+                       while (val_out - unescaped_val < sizeof(unescaped_val) - 1
+                       && sldns_parse_char( (uint8_t*) val_out, &val_in)) {
                                val_out++;
                        }
                }
@@ -1561,8 +1574,6 @@ int sldns_str2wire_svcparam_buf(const char* str, uint8_t* rd, size_t* rd_len)
        else {
                return sldns_str2wire_svcparam_value(str, strlen(str), NULL, rd, rd_len);
        }
-
-       return LDNS_WIREPARSE_ERR_OK;
 }
 
 int sldns_str2wire_rdf_buf(const char* str, uint8_t* rd, size_t* len,

diff --git a/sldns/str2wire.h b/sldns/str2wire.h
index 5fc096b3ec3e60669abf58886760ae480c6a39a3..60dab77ae4f4039bc98d3a914e32d5876c3d5f5b 100644 (file)
--- a/sldns/str2wire.h
+++ b/sldns/str2wire.h
@@ -36,7 +36,7 @@ struct sldns_struct_lookup_table;
 #define SVCB_KEY_NO_DEFAULT_ALPN       2
 #define SVCB_KEY_PORT                  3
 #define SVCB_KEY_IPV4HINT              4
-#define SVCB_KEY_ECH           5
+#define SVCB_KEY_ECH                   5
 #define SVCB_KEY_IPV6HINT              6
 #define SVCPARAMKEY_COUNT 7
 

diff --git a/sldns/wire2str.c b/sldns/wire2str.c
index 99ce5574ec759d39456b03f528cc72e6548724ed..6ed94760a0be3f8a529fbb8236254d459cc55bf8 100644 (file)
--- a/sldns/wire2str.c
+++ b/sldns/wire2str.c
@@ -1003,19 +1003,20 @@ static int sldns_wire2str_svcparam_ipv4hint2str(char** s,
 
        if ((data_len % LDNS_IP4ADDRLEN) == 0) {
                if (inet_ntop(AF_INET, data, ip_str, sizeof(ip_str)) == NULL)
-                       return 0; /* wireformat error, incorrect size or inet family */
+                       return -1; /* wireformat error, incorrect size or inet family */
 
                w += sldns_str_print(s, slen, "=%s", ip_str);
                data += LDNS_IP4ADDRLEN;
 
                while ((data_len -= LDNS_IP4ADDRLEN) > 0) {
                        if (inet_ntop(AF_INET, data, ip_str, sizeof(ip_str)) == NULL)
-                               return 0; /* wireformat error, incorrect size or inet family */
+                               return -1; /* wireformat error, incorrect size or inet family */
 
                        w += sldns_str_print(s, slen, ",%s", ip_str);
                        data += LDNS_IP4ADDRLEN;
                }
-       }
+       } else
+               return -1;
 
        return w;
 }
@@ -1031,19 +1032,20 @@ static int sldns_wire2str_svcparam_ipv6hint2str(char** s,
 
        if ((data_len % LDNS_IP6ADDRLEN) == 0) {
                if (inet_ntop(AF_INET6, data, ip_str, sizeof(ip_str)) == NULL)
-                       return 0; /* wireformat error, incorrect size or inet family */
+                       return -1; /* wireformat error, incorrect size or inet family */
 
                w += sldns_str_print(s, slen, "=%s", ip_str);
                data += LDNS_IP6ADDRLEN;
 
                while ((data_len -= LDNS_IP6ADDRLEN) > 0) {
                        if (inet_ntop(AF_INET6, data, ip_str, sizeof(ip_str)) == NULL)
-                               return 0; /* wireformat error, incorrect size or inet family */
+                               return -1; /* wireformat error, incorrect size or inet family */
 
                        w += sldns_str_print(s, slen, ",%s", ip_str);
                        data += LDNS_IP6ADDRLEN;
                }
-       }
+       } else
+               return -1;
 
        return w;
 }
@@ -1055,8 +1057,8 @@ static int sldns_wire2str_svcparam_mandatory2str(char** s,
 
        assert(data_len > 0);
 
-       // if (data_len % sizeof(uint16_t))
-       //      return 0; // wireformat error, data_len must be multiple of shorts
+       if (data_len % sizeof(uint16_t))
+               return -1; // wireformat error, data_len must be multiple of shorts
        w += sldns_str_print(s, slen, "=");
        w += sldns_print_svcparamkey(s, slen, sldns_read_uint16(data));
        data += 2;
@@ -1076,14 +1078,15 @@ static int sldns_wire2str_svcparam_alpn2str(char** s,
        uint8_t *dp = (void *)data;
        int w = 0;
 
-       assert(data_len > 0); /* Guaranteed by rdata_svcparam_to_string */
+       assert(data_len > 0); /* Guaranteed by sldns_wire2str_svcparam_scan */
 
        w += sldns_str_print(s, slen, "=\"");
        while (data_len) {
+               /* alpn is list of length byte (str_len) followed by a string of that size */
                uint8_t i, str_len = *dp++;
 
                if (str_len > --data_len)
-                       return 0;
+                       return -1;
 
                for (i = 0; i < str_len; i++) {
                        if (dp[i] == '"' || dp[i] == '\\')
@@ -1113,22 +1116,18 @@ static int sldns_wire2str_svcparam_ech2str(char** s,
        int size;
        int w = 0;
 
-       assert(data_len > 0); /* Guaranteed by rdata_svcparam_to_string */
+       assert(data_len > 0); /* Guaranteed by sldns_wire2str_svcparam_scan */
 
        w += sldns_str_print(s, slen, "=\"");
 
-       size = sldns_b64_ntop(data, data_len, *s, *slen);
+       if ((size = sldns_b64_ntop(data, data_len, *s, *slen)) < 0)
+               return -1;
 
        (*s) += size;
        (*slen) -= size;
 
        w += sldns_str_print(s, slen, "\"");    
 
-       // @TODO fix check
-       // if(size > *slen) {
-       //      buffer_skip(output, size);
-       // }
-
        return w + size;
 }
 
@@ -1162,9 +1161,9 @@ int sldns_wire2str_svcparam_scan(uint8_t** d, size_t* dlen, char** s, size_t* sl
                case SVCB_KEY_IPV4HINT:
                case SVCB_KEY_IPV6HINT:
                case SVCB_KEY_MANDATORY:
-                       return LDNS_WIREPARSE_ERR_SYNTAX_MISSING_VALUE;
+                       return -1;
                default:
-                       return LDNS_WIREPARSE_ERR_OK;
+                       return written_chars;
                }
        }
 
@@ -1205,7 +1204,7 @@ int sldns_wire2str_svcparam_scan(uint8_t** d, size_t* dlen, char** s, size_t* sl
                                r += sldns_str_print(s, slen, "%c", ch);
 
                }
-               r += sldns_str_print(s, slen, "%c", '"');
+               r += sldns_str_print(s, slen, "\"");
                break;
        }
        if (r <= 0)

diff --git a/sldns/wire2str.h b/sldns/wire2str.h
index 3c777367c3b4e7888958dd8a0aac6f131eacff32..0167fe7c1e2d2fdc23b67a38fe54c29e2183a638 100644 (file)
--- a/sldns/wire2str.h
+++ b/sldns/wire2str.h
@@ -41,9 +41,6 @@ extern struct sldns_struct_lookup_table* sldns_wireparse_errors;
 /** tsig errors are the rcodes with extra (higher) values */
 extern struct sldns_struct_lookup_table* sldns_tsig_errors;
 
-/* draft-ietf-dnsop-svcb-https-04: 6. Initial SvcParamKeys */
-extern const char *svcparamkey_strs[];
-
 /**
  * Convert wireformat packet to a string representation
  * @param data: wireformat packet data (starting at ID bytes).

diff --git a/testcode/readzone.c b/testcode/readzone.c
index 927d55f539b63f0810580babd949664c106a871e..3854465eb84d115ed4daf534a945965dd5b90d54 100644 (file)
--- a/testcode/readzone.c
+++ b/testcode/readzone.c
@@ -72,7 +72,7 @@ int main(int argc, char *const *argv)
 
                s = sldns_fp2wire_rr_buf(in, rr, &rr_len, &dname_len, &state);
                if (s) {
-                       fprintf( stderr, "parse error %d:%d: %s"
+                       fprintf( stderr, "parse error %d:%d: %s\n"
                               , state.lineno, LDNS_WIREPARSE_OFFSET(s)
                               , sldns_get_errorstr_parse(s));
                        break;
@@ -103,5 +103,6 @@ int main(int argc, char *const *argv)
        }
        if (in)
                fclose(in);
+       free(str);
        return !in || s ? EXIT_FAILURE : EXIT_SUCCESS;
 }