PR #24366 opened by Niklas Haas (haasn) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24366 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24366.patch
Avoids a number of UB cases still reachable in the current code. Tested using a slop-coded synthetic malicious test server. >From 6c4f37eea09f30bc4e796973eb29875f88ac2b61 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Fri, 4 Sep 2026 15:06:31 +0200 Subject: [PATCH 1/3] avformat/libcurl: add extra Content-Range sanity checks libcurl does essentially no checking on these metadata fields and passes them to the user unfiltered. Instead of blindly trusting the server values, we should verify that the values are sane. This implicitly guards arithmetic downstream of these values. This commit also slightly changes the semantics of when and how c->request_end is set. It's now always set to the correct content end, even if the stream is not seekable. The only current use site already checks c->seekable as a separate precondition. In addition, we also only consult the header at all on 206 responses. The HTTP spec clearly states that the content-range header has no meaning outside of the defined responses (i.e. 206, 416). Finally, we also sanity check the expected content size against the claimed content end. Signed-off-by: Niklas Haas <[email protected]> --- libavformat/libcurl.c | 74 +++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 20 deletions(-) diff --git a/libavformat/libcurl.c b/libavformat/libcurl.c index a2f7e6201e..7b8f2a8bdb 100644 --- a/libavformat/libcurl.c +++ b/libavformat/libcurl.c @@ -138,6 +138,7 @@ struct CurlContext { int64_t hdr_content_start; /* inclusive start, or -1 */ int64_t hdr_content_end; /* inclusive end, or -1 */ int64_t hdr_content_total; /* if known, or -1 */ + int64_t hdr_content_length; /* Probe result. Set by the loop thread, read by url_open() once probed. */ int probed; @@ -255,6 +256,47 @@ static void parse_content_range(CurlContext *c, const char *v) c->hdr_content_total = parse_offset(slash + 1); } +static int verify_content_range(CurlContext *c, int64_t start, int64_t end) +{ + /* The reply must start at the offset we requested: for follow-up + * requests always, for the initial one when an explicit nonzero + * offset was requested. */ + const int expected_offset = c->probed ? c->seekable : c->off > 0; + if (start < 0 || (expected_offset && start != c->request_start)) { + av_log(c->h, AV_LOG_ERROR, "Server sent back unexpected reply " + "with offset %"PRId64" (expected %"PRId64")\n", + start, c->request_start); + return 0; + } + + if (end >= 0 && end < start) { + av_log(c->h, AV_LOG_ERROR, "Server sent back backwards content range " + "%"PRId64"-%"PRId64"\n", start, end); + return 0; + } + + if (c->hdr_content_total >= 0 && + (start >= c->hdr_content_total || end >= c->hdr_content_total)) + { + av_log(c->h, AV_LOG_ERROR, "Server sent back content range " + "%"PRId64"-%"PRId64" that exceeds the total size " + "%"PRId64"\n", start, end, c->hdr_content_total); + return 0; + } + + if (c->hdr_content_length >= 0 && end >= 0 && + (c->hdr_content_length - 1 > INT64_MAX - start || + start + (c->hdr_content_length - 1) != end)) + { + av_log(c->h, AV_LOG_ERROR, "Server sent back content range " + "%"PRId64"-%"PRId64" that doesn't match the content length " + "%"PRId64"\n", start, end, c->hdr_content_length); + return 0; + } + + return 1; +} + static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userdata) { CurlContext *c = userdata; @@ -268,6 +310,7 @@ static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userd c->hdr_content_start = -1; c->hdr_content_end = -1; c->hdr_content_total = -1; + c->hdr_content_length = -1; return len; } if (av_strncasecmp(ptr, "Accept-Ranges:", 14) == 0) { @@ -296,17 +339,17 @@ static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userd if (status < 200 || (status >= 300 && status < 400)) return len; + curl_off_t cl = -1; + if (curl_easy_getinfo(c->easy, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, &cl) == CURLE_OK) + c->hdr_content_length = cl >= 0 ? cl : -1; + pthread_mutex_lock(&c->mutex); if (status >= 200 && status < 300) { int64_t content_start = status == 206 ? c->hdr_content_start : 0; - /* The reply must start at the offset we requested: for follow-up - * requests always, for the initial one when an explicit nonzero - * offset was requested. */ - if ((c->probed ? c->seekable : c->off > 0) && - content_start != c->request_start) { - av_log(c->h, AV_LOG_ERROR, "Server sent back unexpected reply " - "with offset %"PRId64" (expected %"PRId64")\n", - content_start, c->request_start); + int64_t content_end = status == 206 ? c->hdr_content_end : -1; + if (content_end < 0 && !c->hdr_compressed) + content_end = c->content_size - 1; + if (!verify_content_range(c, content_start, content_end)) { c->loop->num_errors++; c->stream_ok = 0; if (!c->status) @@ -317,6 +360,7 @@ static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userd } c->stream_ok = 1; + c->request_end = content_end; /* Capture the post-redirect URL, this is exposed as "location" AVOption * for compatibility with http.c. */ if (!c->probed) { @@ -338,22 +382,12 @@ static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userd (status == 206 || c->hdr_accept_ranges); if (!c->hdr_compressed) { int64_t total = c->hdr_content_total; - if (total < 0 && status != 206) { - curl_off_t cl = -1; - if (curl_easy_getinfo(c->easy, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, - &cl) == CURLE_OK && cl >= 0) - total = cl; - } + if (total < 0 && status != 206) + total = c->hdr_content_length; /* Don't unlearn a known size when a reply omits it. */ if (total >= 0) c->content_size = total; } - if (c->seekable) { - if (c->hdr_content_end >= 0) - c->request_end = c->hdr_content_end; - else - c->request_end = c->content_size > 0 ? c->content_size - 1 : -1; - } /* Apply the user override on every reply so re-evaluation of a * follow-up reply doesn't clobber it. */ if (c->seekable_opt >= 0) -- 2.52.0 >From c98d20be0bbeee0ddd3319b30db74771f238626a Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Fri, 4 Sep 2026 17:31:52 +0200 Subject: [PATCH 2/3] avformat/libcurl: harden Content-Range parsing Reject out-of-range values, non-numeric leading characters (e.g. extra whitespace), and extra unexpected bytes; which the previous code implicitly skipped over by way of using strchr() to find delimiters. Signed-off-by: Niklas Haas <[email protected]> --- libavformat/libcurl.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/libavformat/libcurl.c b/libavformat/libcurl.c index 7b8f2a8bdb..ea8fcf6d0d 100644 --- a/libavformat/libcurl.c +++ b/libavformat/libcurl.c @@ -22,6 +22,7 @@ #include "config_components.h" #include <curl/curl.h> +#include <errno.h> #include <inttypes.h> #include <limits.h> #include <stdlib.h> @@ -229,10 +230,19 @@ static size_t write_callback(char *ptr, size_t size, size_t nmemb, void *userdat return bytes; } -static int64_t parse_offset(const char *s) +static int64_t parse_offset(const char **s) { - int64_t v = strtoll(s, NULL, 10); - return v < 0 ? -1 : v; + if (!av_isdigit(**s)) + return -1; + + errno = 0; + char *end; + int64_t v = strtoll(*s, &end, 10); + if (errno == ERANGE) + return -1; + + *s = end; + return v; } /* "bytes $from-$to/$document_size" */ @@ -244,16 +254,22 @@ static void parse_content_range(CurlContext *c, const char *v) if (av_strncasecmp(v, "bytes ", 6)) return; - const char *range = v + 6, *end; - if (range[0] != '*') { - c->hdr_content_start = parse_offset(range); - if ((end = strchr(range, '-'))) - c->hdr_content_end = parse_offset(end + 1); + v += 6; + if (v[0] == '*') { + v++; + } else { + c->hdr_content_start = parse_offset(&v); + if (v[0] == '-') { + v++; + c->hdr_content_end = parse_offset(&v); + } } - const char *slash = strchr(range, '/'); - if (slash && slash[1] != '*') - c->hdr_content_total = parse_offset(slash + 1); + if (v[0] == '/') { + v++; + if (v[0] != '*') + c->hdr_content_total = parse_offset(&v); + } } static int verify_content_range(CurlContext *c, int64_t start, int64_t end) -- 2.52.0 >From b6c7c8419613eb5f9e9a38597645f885392e6782 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Fri, 4 Sep 2026 17:45:21 +0200 Subject: [PATCH 3/3] avformat/libcurl: guard against overflow from undelimited responses verify_content_range() already clamps down on the valid byte range for 206 replies (or 200 replies with a Content-Length), but an undelimited response represents an escape path that can still trigger overflow here. Signed-off-by: Niklas Haas <[email protected]> --- libavformat/libcurl.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/libavformat/libcurl.c b/libavformat/libcurl.c index ea8fcf6d0d..7218f312d5 100644 --- a/libavformat/libcurl.c +++ b/libavformat/libcurl.c @@ -208,6 +208,17 @@ static size_t write_callback(char *ptr, size_t size, size_t nmemb, void *userdat pthread_mutex_lock(&c->mutex); + /* Prevent overflow / non-addressable byte ranges */ + if (bytes > INT64_MAX - c->request_start - c->request_received) { + av_log(c->h, AV_LOG_ERROR, "Server sent back more data than addressable " + "at offset %"PRId64"\n", c->request_start); + c->loop->num_errors++; + c->stream_ok = 0; + if (!c->status) + c->status = AVERROR(EIO); + pthread_cond_broadcast(&c->cond); + } + if (c->aborted || !c->stream_ok) { pthread_mutex_unlock(&c->mutex); return CURL_WRITEFUNC_ERROR; -- 2.52.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
