From: Daniel Stenberg Date: Wed, 21 May 2025 06:24:39 +0000 (+0200) Subject: http_aws_sigv4: avoid risk of overflowed constant X-Git-Tag: curl-8_14_0~39 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8c8186eadcb56ad1f3b51579e13f0ff416bdb58d;p=thirdparty%2Fcurl.git http_aws_sigv4: avoid risk of overflowed constant - Simplify canon_query() a bit. Avoid unconditionally using length -1 where length risks being zero at times. Pointed out by Coverity. - Fix indent errors - narrow some variable scopes - fix keywords in tests Closes #17402 --- diff --git a/lib/http_aws_sigv4.c b/lib/http_aws_sigv4.c index 4d09013851..e8ef2c459b 100644 --- a/lib/http_aws_sigv4.c +++ b/lib/http_aws_sigv4.c @@ -72,15 +72,16 @@ struct pair { static void dyn_array_free(struct dynbuf *db, size_t num_elements); static void pair_array_free(struct pair *pair_array, size_t num_elements); -static CURLcode split_to_dyn_array(const char *source, char split_by, - struct dynbuf db[MAX_QUERY_COMPONENTS], size_t *num_splits); +static CURLcode split_to_dyn_array(const char *source, + struct dynbuf db[MAX_QUERY_COMPONENTS], + size_t *num_splits); static bool is_reserved_char(const char c); static CURLcode uri_encode_path(struct Curl_str *original_path, - struct dynbuf *new_path); + struct dynbuf *new_path); static CURLcode encode_query_component(char *component, size_t len, - struct dynbuf *db); + struct dynbuf *db); static CURLcode http_aws_decode_encode(const char *in, size_t in_len, - struct dynbuf *out); + struct dynbuf *out); static bool should_urlencode(struct Curl_str *service_name); static void sha256_to_hex(char *dst, unsigned char *sha) @@ -577,21 +578,12 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq) size_t num_query_components; size_t counted_query_components = 0; size_t index; - size_t in_key_len; - size_t in_value_len; - size_t query_part_len; - const char *in_key; - char *in_value; - char *offset; - char *key_ptr; - char *value_ptr; - const char *query_part; if(!query) return result; - result = split_to_dyn_array(query, '&', &query_array[0], - &num_query_components); + result = split_to_dyn_array(query, &query_array[0], + &num_query_components); if(result) { goto fail; } @@ -599,11 +591,12 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq) /* Create list of pairs, each pair containing an encoded query * component */ - for(index = 0; index < num_query_components; - index++) { - - query_part_len = curlx_dyn_len(&query_array[index]); - query_part = curlx_dyn_ptr(&query_array[index]); + for(index = 0; index < num_query_components; index++) { + const char *in_key; + size_t in_key_len; + char *offset; + size_t query_part_len = curlx_dyn_len(&query_array[index]); + char *query_part = curlx_dyn_ptr(&query_array[index]); in_key = query_part; @@ -622,17 +615,18 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq) /* Decode/encode the key */ result = http_aws_decode_encode(in_key, in_key_len, - &encoded_query_array[index].key); + &encoded_query_array[index].key); if(result) { goto fail; } /* Decode/encode the value if it exists */ if(offset && offset != (query_part + query_part_len - 1)) { - in_value = offset + 1; + size_t in_value_len; + const char *in_value = offset + 1; in_value_len = query_part + query_part_len - (offset + 1); result = http_aws_decode_encode(in_value, in_value_len, - &encoded_query_array[index].value); + &encoded_query_array[index].value); if(result) { goto fail; } @@ -650,30 +644,33 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq) /* Sort the encoded query components by key and value */ qsort(&encoded_query_array, num_query_components, - sizeof(struct pair), compare_func); + sizeof(struct pair), compare_func); /* Append the query components together to make a full query string */ for(index = 0; index < num_query_components; index++) { - key_ptr = curlx_dyn_ptr(&encoded_query_array[index].key); - value_ptr = curlx_dyn_ptr(&encoded_query_array[index].value); - - if(value_ptr && strlen(value_ptr)) { - result = curlx_dyn_addf(dq, "%s=%s&", key_ptr, value_ptr); - } - else { - /* Empty value is always encoded to key= */ - result = curlx_dyn_addf(dq, "%s=&", key_ptr); - } - if(result) { - goto fail; + if(index) + result = curlx_dyn_addn(dq, "&", 1); + if(!result) { + char *key_ptr = curlx_dyn_ptr(&encoded_query_array[index].key); + char *value_ptr = curlx_dyn_ptr(&encoded_query_array[index].value); + size_t vlen = curlx_dyn_len(&encoded_query_array[index].value); + if(value_ptr && vlen) { + result = curlx_dyn_addf(dq, "%s=%s", key_ptr, value_ptr); + } + else { + /* Empty value is always encoded to key= */ + result = curlx_dyn_addf(dq, "%s=", key_ptr); + } } + if(result) + break; } - /* Remove trailing & */ - result = curlx_dyn_setlen(dq, curlx_dyn_len(dq)-1); fail: - pair_array_free(&encoded_query_array[0], counted_query_components); + if(counted_query_components) + /* the encoded_query_array might not be initialized yet */ + pair_array_free(&encoded_query_array[0], counted_query_components); dyn_array_free(&query_array[0], num_query_components); return result; } @@ -1010,41 +1007,38 @@ static void dyn_array_free(struct dynbuf *db, size_t num_elements) { size_t index; - for(index = 0; index < num_elements; index++) { + for(index = 0; index < num_elements; index++) curlx_dyn_free((&db[index])); - } - } /* -* Splits source string by split_by, and creates an array of dynbuf in db -* db is initialized by this function +* Splits source string by SPLIT_BY, and creates an array of dynbuf in db. +* db is initialized by this function. * Caller is responsible for freeing the array elements with dyn_array_free */ -static CURLcode split_to_dyn_array(const char *source, char split_by, - struct dynbuf db[MAX_QUERY_COMPONENTS], size_t *num_splits_out) -{ +#define SPLIT_BY '&' +static CURLcode split_to_dyn_array(const char *source, + struct dynbuf db[MAX_QUERY_COMPONENTS], + size_t *num_splits_out) +{ CURLcode result = CURLE_OK; - size_t len = strlen(source); - size_t pos = 0; /* Position in result buffer */ size_t start = 0; /* Start of current segment */ size_t segment_length = 0; size_t index = 0; - size_t num_splits; + size_t num_splits = 0; - /* Split source_ptr on split_by and store the segment offsets and - * length in array */ - num_splits = 0; + /* Split source_ptr on SPLIT_BY and store the segment offsets and length in + * array */ for(pos = 0; pos < len; pos++) { - if(source[pos] == split_by) { + if(source[pos] == SPLIT_BY) { if(segment_length) { curlx_dyn_init(&db[index], segment_length + 1); result = curlx_dyn_addn(&db[index], &source[start], - segment_length); + segment_length); if(result) { goto fail; } @@ -1064,14 +1058,14 @@ static CURLcode split_to_dyn_array(const char *source, char split_by, if(segment_length) { curlx_dyn_init(&db[index], segment_length + 1); result = curlx_dyn_addn(&db[index], &source[start], - segment_length); + segment_length); if(result) { goto fail; } if(++num_splits == MAX_QUERY_COMPONENTS) { goto fail; } -} + } fail: *num_splits_out = num_splits; return result; @@ -1084,9 +1078,8 @@ static bool is_reserved_char(const char c) } static CURLcode uri_encode_path(struct Curl_str *original_path, -struct dynbuf *new_path) + struct dynbuf *new_path) { - const char *p = curlx_str(original_path); CURLcode result = CURLE_OK; size_t index; @@ -1113,9 +1106,8 @@ fail: static CURLcode encode_query_component(char *component, size_t len, - struct dynbuf *db) + struct dynbuf *db) { - size_t index; CURLcode result = CURLE_OK; unsigned char this_char; @@ -1149,17 +1141,16 @@ fail: */ static CURLcode http_aws_decode_encode(const char *in, size_t in_len, -struct dynbuf *out) + struct dynbuf *out) { - CURLcode result = CURLE_OK; char *out_s; size_t out_s_len; + CURLcode result = + Curl_urldecode(in, in_len, &out_s, &out_s_len, REJECT_NADA); - result = Curl_urldecode(in, in_len, &out_s, &out_s_len, REJECT_NADA); - - if(result) { + if(result) goto fail; - } + result = encode_query_component(out_s, out_s_len, out); Curl_safefree(out_s); fail: diff --git a/tests/data/test1979 b/tests/data/test1979 index d89b097a07..3cff1314db 100644 --- a/tests/data/test1979 +++ b/tests/data/test1979 @@ -3,6 +3,7 @@ unittest canon_string +aws-sigv4 diff --git a/tests/data/test1980 b/tests/data/test1980 index 6e801fe061..4ac36377f8 100644 --- a/tests/data/test1980 +++ b/tests/data/test1980 @@ -3,6 +3,7 @@ unittest canon_query +aws-sigv4