altsvc: reject bad port numbers

The existing code tried but did not properly reject alternative services
using negative or too large port numbers.

With this fix, the logic now also flushes the old entries immediately
before adding a new one, making a following header with an illegal entry
not flush the already stored entry.

Report from the ongoing source code audit by Trail of Bits.

Adjusted test 356 to verify.

Closes #9607

diff --git a/lib/altsvc.c b/lib/altsvc.c
index bda3bf4e0a6f6ddc8b6aa8c5c87a4d82f6bbd332..7bca840151753192b7fb7ea4ffc86406105b48f1 100644 (file)
--- a/lib/altsvc.c
+++ b/lib/altsvc.c
@@ -462,6 +462,7 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
   struct altsvc *as;
   unsigned short dstport = srcport; /* the same by default */
   CURLcode result = getalnum(&p, alpnbuf, sizeof(alpnbuf));
+  size_t entries = 0;
 #ifdef CURL_DISABLE_VERBOSE_STRINGS
   (void)data;
 #endif
@@ -472,11 +473,10 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
 
   DEBUGASSERT(asi);
 
-  /* Flush all cached alternatives for this source origin, if any */
-  altsvc_flush(asi, srcalpnid, srchost, srcport);
-
   /* "clear" is a magic keyword */
   if(strcasecompare(alpnbuf, "clear")) {
+    /* Flush cached alternatives for this source origin */
+    altsvc_flush(asi, srcalpnid, srchost, srcport);
     return CURLE_OK;
   }
 
@@ -494,6 +494,7 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
         bool quoted = FALSE;
         time_t maxage = 24 * 3600; /* default is 24 hours */
         bool persist = FALSE;
+        bool valid = TRUE;
         p++;
         if(*p != ':') {
           /* host name starts here */
@@ -503,7 +504,7 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
           len = p - hostp;
           if(!len || (len >= MAX_ALTSVC_HOSTLEN)) {
             infof(data, "Excessive alt-svc host name, ignoring.");
-            dstalpnid = ALPN_none;
+            valid = FALSE;
           }
           else {
             memcpy(namebuf, hostp, len);
@@ -520,10 +521,11 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
           unsigned long port = strtoul(++p, &end_ptr, 10);
           if(port > USHRT_MAX || end_ptr == p || *end_ptr != '\"') {
             infof(data, "Unknown alt-svc port number, ignoring.");
-            dstalpnid = ALPN_none;
+            valid = FALSE;
           }
+          else
+            dstport = curlx_ultous(port);
           p = end_ptr;
-          dstport = curlx_ultous(port);
         }
         if(*p++ != '\"')
           break;
@@ -575,7 +577,12 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
               persist = TRUE;
           }
         }
-        if(dstalpnid) {
+        if(dstalpnid && valid) {
+          if(!entries++)
+            /* Flush cached alternatives for this source origin, if any - when
+               this is the first entry of the line. */
+            altsvc_flush(asi, srcalpnid, srchost, srcport);
+
           as = altsvc_createid(srchost, dsthost,
                                srcalpnid, dstalpnid,
                                srcport, dstport);
@@ -589,10 +596,6 @@ CURLcode Curl_altsvc_parse(struct Curl_easy *data,
                   Curl_alpnid2str(dstalpnid));
           }
         }
-        else {
-          infof(data, "Unknown alt-svc protocol \"%s\", skipping.",
-                alpnbuf);
-        }
       }
       else
         break;

diff --git a/tests/data/test356 b/tests/data/test356
index 0f8795899793e16fedd699a5b80a01c6bf5ea1f7..9f6459487c3db78297dfa978875087b1048ae2e7 100644 (file)
--- a/tests/data/test356
+++ b/tests/data/test356
@@ -16,7 +16,9 @@ Content-Length: 6
 Connection: close
 Content-Type: text/html
 Funny-head: yesyes
+Alt-Svc: h1="nowhere.foo:-1"
 Alt-Svc: h1="nowhere.foo:81", un-kno22!wn=":82"
+Alt-Svc: h1="nowhere.foo:70000"
 
 -foo-
 </data>