s3: sign nothing when out of memory, and sort query parameters

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),

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,13 +32,36 @@
  */
 
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
 #include <unistd.h>
 
 #include <boolean.h>
 #include <streams/file_stream.h>
 
+/* 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);