[v2] curl: refuse to open URL from HTTP server without range support
diff mbox

Message ID 1371463763-25628-1-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng June 17, 2013, 10:09 a.m. UTC
CURL driver requests partial data from server on guest IO req. For HTTP
and HTTPS, it uses "Range: ***" in requests, and this will not work if
server not accepting range. This patch does this check when open.

 * Removed curl_size_cb, which is not used: On one hand it's registered to
   libcurl as CURLOPT_WRITEFUNCTION, instead of CURLOPT_HEADERFUNCTION,
   which will get called with *data*, not *header*. On the other hand the
   s->len is assigned unconditionally later.

   In this gone function, the sscanf for "Content-Length: %zd", on
   (void *)ptr, which is not guaranteed to be zero-terminated, is
   potentially a security bug. So this patch fixes it as a side-effect. The
   bug is reported as: https://bugs.launchpad.net/qemu/+bug/1188943
   (Note the bug is marked "private" so you might not be able to see it)

 * Introduced curl_header_cb, which is used to parse header and mark the
   server as accepting range if "Accept-Ranges: bytes" line is seen from
   response header. If protocol is HTTP or HTTPS, but server response has
   no not this support, refuse to open this URL.

Note that python builtin module SimpleHTTPServer is an example of not
supporting range, if you need to test this driver, get a better server
or use internet URLs.

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---

v2:
    Fix the case that header data is shorter than accept_line length. [stefan]

---
 block/curl.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Comments

Richard W.M. Jones June 17, 2013, 10:54 a.m. UTC | #1
On Mon, Jun 17, 2013 at 06:09:23PM +0800, Fam Zheng wrote:
> +    if (real_size >= strlen(accept_len)

What's accept_len?  This patch gives me a couple of compile errors:

block/curl.c: In function ‘curl_header_cb’:
block/curl.c:120:9: error: ‘real_size’ undeclared (first use in this function)
block/curl.c:120:9: note: each undeclared identifier is reported only once for each function it appears in
block/curl.c:120:29: error: ‘accept_len’ undeclared (first use in this function)
make: *** [block/curl.o] Error 1

Rich.
Fam Zheng June 17, 2013, 11:02 a.m. UTC | #2
On Mon, 06/17 11:54, Richard W.M. Jones wrote:
> On Mon, Jun 17, 2013 at 06:09:23PM +0800, Fam Zheng wrote:
> > +    if (real_size >= strlen(accept_len)
> 
> What's accept_len?  This patch gives me a couple of compile errors:
> 
Oops, a copy&paste mistake. I meant this:

+    if (realsize >= strlen(accept_line)
Richard W.M. Jones June 17, 2013, 12:19 p.m. UTC | #3
On Mon, Jun 17, 2013 at 07:02:00PM +0800, Fam Zheng wrote:
> On Mon, 06/17 11:54, Richard W.M. Jones wrote:
> > On Mon, Jun 17, 2013 at 06:09:23PM +0800, Fam Zheng wrote:
> > > +    if (real_size >= strlen(accept_len)
> > 
> > What's accept_len?  This patch gives me a couple of compile errors:
> > 
> Oops, a copy&paste mistake. I meant this:
> 
> +    if (realsize >= strlen(accept_line)

I added this fix and tested it against a Python HTTP server (which
does not support ranges):

$ cd /tmp
$ python -m SimpleHTTPServer

[in another window ...]

$ http_proxy= ./qemu-io -r http://127.0.0.1:8000/test1.img -c 'read -v 0 512'
CURL: Error opening file: Server not supporting range.
qemu-io: can't open device http://127.0.0.1:8000/test1.img
no file open, try 'help open'

[against another server which does support ranges ...]

$ LD_LIBRARY_PATH=~/d/curl/lib/.libs http_proxy= ./qemu-io -r http://libguestfs.org/index.html -c 'read -v 0 512'
00000000:  3c 21 44 4f 43 54 59 50 45 20 68 74 6d 6c 20 50  ..DOCTYPE.html.P
[etc]

By the way I think it would be better if the error message said
"Server does not support 'range' (byte ranges)."

Rich.

Patch
diff mbox

diff --git a/block/curl.c b/block/curl.c
index b8935fd..7d8644a 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -81,6 +81,7 @@  typedef struct BDRVCURLState {
     CURLState states[CURL_NUM_STATES];
     char *url;
     size_t readahead_size;
+    bool accept_range;
 } BDRVCURLState;
 
 static void curl_clean_state(CURLState *s);
@@ -110,14 +111,15 @@  static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     return 0;
 }
 
-static size_t curl_size_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
+static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
 {
-    CURLState *s = ((CURLState*)opaque);
+    BDRVCURLState *s = opaque;
     size_t realsize = size * nmemb;
-    size_t fsize;
+    const char *accept_line = "Accept-Ranges: bytes";
 
-    if(sscanf(ptr, "Content-Length: %zd", &fsize) == 1) {
-        s->s->len = fsize;
+    if (real_size >= strlen(accept_len)
+        && strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
+        s->accept_range = true;
     }
 
     return realsize;
@@ -441,8 +443,11 @@  static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 
     // Get file size
 
+    s->accept_range = false;
     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 1);
-    curl_easy_setopt(state->curl, CURLOPT_WRITEFUNCTION, (void *)curl_size_cb);
+    curl_easy_setopt(state->curl, CURLOPT_HEADERFUNCTION,
+                     curl_header_cb);
+    curl_easy_setopt(state->curl, CURLOPT_HEADERDATA, s);
     if (curl_easy_perform(state->curl))
         goto out;
     curl_easy_getinfo(state->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &d);
@@ -452,6 +457,12 @@  static int curl_open(BlockDriverState *bs, QDict *options, int flags)
         s->len = (size_t)d;
     else if(!s->len)
         goto out;
+    if ((!strncasecmp(s->url, "http://", strlen("http://"))
+        || !strncasecmp(s->url, "https://", strlen("https://")))
+        && !s->accept_range) {
+        pstrcpy(state->errmsg, CURL_ERROR_SIZE, "Server not supporting range.");
+        goto out;
+    }
     DPRINTF("CURL: Size = %zd\n", s->len);
 
     curl_clean_state(state);