From 45eaaef44d441838520540c90504e90fb4dadda1 Mon Sep 17 00:00:00 2001 From: Anthony Schemel Date: Sat, 31 Oct 2026 10:04:01 +0200 Subject: [PATCH 1/2] s3: sign nothing when out of memory s3_calculate_signature_v4_with_time carried on when an allocation for the request's canonical form failed: - No memory for the canonical headers: it signed with empty headers. - No memory for the canonical query: it signed with an empty query, while the URL still carried the real one. S3 can only reject such a signature, so the failure shows up as a 403 that says nothing about memory. Both now return no signature. s3_build_auth_header_ex passes that on as NULL, and every caller already treats a NULL header as a failure. Test: s3_backup_test gains test_signer_out_of_memory, which fails the driver's Nth allocation through macros in the test, the way http_init_failure_test does. Red before: each case returned a signature. Green after. A control call with memory available must still sign. write-code: s3_backup_test under ASan+UBSan and TSan, as upstream's workflow runs it, red on master, green here; full RetroArch build not run. All nine callers' NULL checks were read. Co-Authored-By: Claude Opus 5.5 --- network/cloud_sync/s3.c | 31 ++++++------ samples/network/s3_backup/s3_backup_test.c | 58 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-) diff --git a/network/cloud_sync/s3.c b/network/cloud_sync/s3.c index f456e8441cba..7ba7f983aaf1 100644 --- a/network/cloud_sync/s3.c +++ b/network/cloud_sync/s3.c @@ -693,19 +693,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 +715,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..d6e94ebd2961 100644 --- a/samples/network/s3_backup/s3_backup_test.c +++ b/samples/network/s3_backup/s3_backup_test.c @@ -20,6 +20,9 @@ * 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: 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 +31,7 @@ */ #include +#include #include #include #include @@ -35,6 +39,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 +339,37 @@ 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); +} + int main(void) { char tmp[64]; @@ -331,6 +388,7 @@ int main(void) test_nothing_to_keep(tmp); test_copy_fails(tmp); test_no_backup_wanted(tmp); + test_signer_out_of_memory(); remove(tmp); From 15f1b5f6b79887859cf25bc5bcc291b46cde93f9 Mon Sep 17 00:00:00 2001 From: Anthony Schemel Date: Sat, 31 Oct 2026 10:04:38 +0200 Subject: [PATCH 2/2] s3: sort query parameters as SigV4 requires s3_canonicalize_query_string returned the query as given, with a comment listing what a full implementation would do. Every request the driver signs today already has its parameters in order, so this changes no request: partNumber sorts before uploadId, and the rest have one parameter. A new request with two parameters out of order would get a signature S3 rejects, with only a 403 to show for it. It now splits on '&', sorts and re-joins. The comparator takes the names on their own: on the whole "name=value" string, "a1=2" would sort before "a=1", since '1' is below '='. URL encoding stays with the callers, as they already do it; the comment names the encoder to use. Test: s3_backup_test gains test_query_order. Red before: every out-of-order case kept its order. Green after. Mutation, red: comparing whole strings fails the "a1" and "a-b" cases. Failing each of the three allocations returns NULL with no leak under ASan. write-code: s3_backup_test under ASan+UBSan and TSan, red on the previous commit, green here, mutation above; full RetroArch build not run. Co-Authored-By: Claude Opus 5.5 --- network/cloud_sync/s3.c | 90 +++++++++++++++++++--- samples/network/s3_backup/s3_backup_test.c | 42 +++++++++- 2 files changed, 120 insertions(+), 12 deletions(-) diff --git a/network/cloud_sync/s3.c b/network/cloud_sync/s3.c index 7ba7f983aaf1..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 */ diff --git a/samples/network/s3_backup/s3_backup_test.c b/samples/network/s3_backup/s3_backup_test.c index d6e94ebd2961..235b3b246463 100644 --- a/samples/network/s3_backup/s3_backup_test.c +++ b/samples/network/s3_backup/s3_backup_test.c @@ -20,8 +20,9 @@ * 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: out of memory, the signer returns - * no signature rather than a wrong one. + * 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 @@ -370,6 +371,42 @@ static void test_signer_out_of_memory(void) 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]; @@ -389,6 +426,7 @@ int main(void) test_copy_fails(tmp); test_no_backup_wanted(tmp); test_signer_out_of_memory(); + test_query_order(); remove(tmp);