diff --git a/network/cloud_sync/s3.c b/network/cloud_sync/s3.c index f456e8441cba..22a22c0bf337 100644 --- a/network/cloud_sync/s3.c +++ b/network/cloud_sync/s3.c @@ -444,21 +444,91 @@ static char* s3_url_encode(const char *input) return output; } -/* Canonicalize query string parameters */ +/* qsort comparator: SigV4 sorts query parameters by URL-encoded name, + * then by URL-encoded value, in byte order. The names are compared on + * their own: on the whole "name=value" string, a name that extends + * another ("a1" after "a") would meet the '=' and sort first, since + * digits, '-', '.' and '%' all sort below '='. */ +static int s3_query_param_cmp(const void *a, const void *b) +{ + const char *x = *(const char* const*)a; + const char *y = *(const char* const*)b; + size_t xn = strcspn(x, "="); + size_t yn = strcspn(y, "="); + int r = strncmp(x, y, xn < yn ? xn : yn); + + if (r) + return r; + if (xn != yn) + return xn < yn ? -1 : 1; + return strcmp(x + xn, y + yn); +} + +/* Canonicalize query string parameters per SigV4: split on '&', sort + * with s3_query_param_cmp, re-join. The caller encodes each name and + * value with s3_url_encode_query_value, and writes every parameter as + * name=value, even with an empty value ("uploads="). Returns NULL only + * when out of memory; an empty query gives "". */ static char* s3_canonicalize_query_string(const char *query_string) { + size_t i; + size_t query_len; + size_t count = 1; + size_t pos = 0; + char *copy; + char *out; + char **params; + if (!query_string || !*query_string) return strdup(""); - /* For now, return the query string as-is since most S3 operations don't use - * query parameters - * A full implementation would: - * 1. Split parameters by '&' - * 2. URL encode each parameter name and value - * 3. Sort parameters alphabetically by name - * 4. Join with '&' - */ - return strdup(query_string); + if (!(copy = strdup(query_string))) + return NULL; + + query_len = strlen(copy); + for (i = 0; i < query_len; i++) + if (copy[i] == '&') + count++; + + params = (char**)malloc(count * sizeof(char*)); + /* Sorting moves the parameters but adds nothing, so the result is + * the input's length. */ + out = (char*)malloc(query_len + 1); + + if (!params || !out) + { + free(params); + free(out); + free(copy); + return NULL; + } + + /* Cut the copy into one string per parameter. */ + params[0] = copy; + for (i = 0, count = 1; i < query_len; i++) + { + if (copy[i] == '&') + { + copy[i] = '\0'; + params[count++] = copy + i + 1; + } + } + + qsort(params, count, sizeof(char*), s3_query_param_cmp); + + for (i = 0; i < count; i++) + { + size_t len = strlen(params[i]); + if (i > 0) + out[pos++] = '&'; + memcpy(out + pos, params[i], len); + pos += len; + } + out[pos] = '\0'; + + free(params); + free(copy); + return out; } /* Calculate HMAC-SHA256 and return binary result */ @@ -693,19 +763,20 @@ static char* s3_calculate_signature_v4_with_time(const char *method, const char * x-amz-date */ size_t canonical_len = 512 + (copy_source ? strlen(copy_source) : 0); - canonical_headers = malloc(canonical_len); - if (canonical_headers) - { - if (copy_source) - snprintf(canonical_headers, canonical_len, - "host:%s\nx-amz-content-sha256:%s\nx-amz-copy-source:%s\nx-amz-date:%s\n", - host, payload_hash, copy_source, datetime); - else - snprintf(canonical_headers, canonical_len, - "host:%s\nx-amz-content-sha256:%s\nx-amz-date:%s\n", - host, payload_hash, datetime); - } + /* Out of memory, the request is not signed at all: a signature + * made without the headers or the query only shows up later, as + * a 403. */ + if (!(canonical_headers = malloc(canonical_len))) + goto cleanup; + if (copy_source) + snprintf(canonical_headers, canonical_len, + "host:%s\nx-amz-content-sha256:%s\nx-amz-copy-source:%s\nx-amz-date:%s\n", + host, payload_hash, copy_source, datetime); + else + snprintf(canonical_headers, canonical_len, + "host:%s\nx-amz-content-sha256:%s\nx-amz-date:%s\n", + host, payload_hash, datetime); } /* Build signed headers list */ @@ -714,8 +785,10 @@ static char* s3_calculate_signature_v4_with_time(const char *method, const char : "host;x-amz-content-sha256;x-amz-date", sizeof(signed_headers)); /* Build canonical query string */ - if (query_string) - canonical_query_string = s3_canonicalize_query_string(query_string); + if (query_string + && !(canonical_query_string = + s3_canonicalize_query_string(query_string))) + goto cleanup; /* Build canonical request */ snprintf(canonical_request, sizeof(canonical_request), diff --git a/samples/network/s3_backup/s3_backup_test.c b/samples/network/s3_backup/s3_backup_test.c index 06951d1afd09..235b3b246463 100644 --- a/samples/network/s3_backup/s3_backup_test.c +++ b/samples/network/s3_backup/s3_backup_test.c @@ -20,6 +20,10 @@ * CopyObject can answer - stops the delete or upload, reported once; * - destructive sync and the server manifest skip the copy. * + * It also calls the signer directly: query parameters sort as SigV4 + * requires, and out of memory the signer returns no signature rather + * than a wrong one. + * * The signature itself was cross-checked against botocore's SigV4 for * the same request when this was written; here the test checks the * header set that goes into it. @@ -28,6 +32,7 @@ */ #include +#include #include #include #include @@ -35,6 +40,28 @@ #include #include +/* Set to N to fail the driver's Nth allocation from here on, counting + * malloc and strdup together. Zero fails none. */ +static unsigned fail_alloc = 0; + +static bool alloc_fails(void) +{ + return fail_alloc && --fail_alloc == 0; +} + +static void *test_malloc(size_t size) +{ + return alloc_fails() ? NULL : malloc(size); +} + +static char *test_strdup(const char *s) +{ + return alloc_fails() ? NULL : strdup(s); +} + +#define malloc test_malloc +#define strdup test_strdup + #include "../../../network/cloud_sync/s3.c" #include "stubs_retroarch.h" @@ -313,6 +340,73 @@ static void test_no_backup_wanted(const char *tmp) "the server manifest must upload without a copy"); } +static char *sign_failing_alloc(unsigned nth) +{ + char *sig; + + fail_alloc = nth; + sig = s3_calculate_signature_v4_with_time("PUT", "/key", "uploadId=x", + "host;x-amz-content-sha256;x-amz-date", S3_EMPTY_PAYLOAD_SHA256, + "us-east-1", S3_SERVICE, "access", "secret", "example.com", + NULL, 0); + fail_alloc = 0; + return sig; +} + +/* The signer allocates the canonical headers first, then the canonical + * query. Losing either must give no signature: one made without them + * only shows up later, as a 403 from S3. */ +static void test_signer_out_of_memory(void) +{ + char *sig = sign_failing_alloc(0); + CHECK(sig, "the signer must sign when memory is available"); + free(sig); + + sig = sign_failing_alloc(1); + CHECK(!sig, "no memory for the headers, but it signed: %s", sig); + free(sig); + + sig = sign_failing_alloc(2); + CHECK(!sig, "no memory for the query, but it signed: %s", sig); + free(sig); +} + +/* SigV4 sorts query parameters by name, then by value. Sorting the + * whole "name=value" string would put "a1=2" before "a=1", because '1' + * sorts below '='. */ +static void test_query_order(void) +{ + static const char *cases[][2] = { + { "a1=2&a=1", "a=1&a1=2" }, + { "a-b=1&a=2", "a=2&a-b=1" }, + { "uploadId=x&partNumber=3", "partNumber=3&uploadId=x" }, + { "k=2&k=1", "k=1&k=2" }, + { "", "" } + }; + size_t i; + + for (i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) + { + char *got = s3_canonicalize_query_string(cases[i][0]); + CHECK(got && !strcmp(got, cases[i][1]), + "query \"%s\": sorted to \"%s\", SigV4 wants \"%s\"", + cases[i][0], got ? got : "(null)", cases[i][1]); + free(got); + } + + /* Each of its three allocations can fail, and none may leak. */ + for (i = 1; i <= 3; i++) + { + char *got; + + fail_alloc = (unsigned)i; + got = s3_canonicalize_query_string("b=2&a=1"); + fail_alloc = 0; + CHECK(!got, "allocation %u failed, but it returned: %s", (unsigned)i, got); + free(got); + } +} + int main(void) { char tmp[64]; @@ -331,6 +425,8 @@ int main(void) test_nothing_to_keep(tmp); test_copy_fails(tmp); test_no_backup_wanted(tmp); + test_signer_out_of_memory(); + test_query_order(); remove(tmp);