Re: [Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-13 Thread Stefan Hajnoczi 
On Thu, Jun 13, 2013 at 4:13 AM, Fam Zheng f...@redhat.com wrote:
 On Tue, 06/11 09:40, Stefan Hajnoczi wrote:
 On Tue, Jun 11, 2013 at 11:15:15AM +0800, Fam Zheng wrote:
  On Mon, Jun 10, 2013 at 5:21 PM, Stefan Hajnoczi stefa...@gmail.com 
  wrote:
   On Sun, Jun 09, 2013 at 10:34:54AM +0800, Fam Zheng wrote:
   @@ -110,14 +111,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
   +s-accept_range = true;
}
  
   This still assumes ptr is NUL-terminated.  You need to pass size * nmemb
   instead of strlen(accept_line).
  
  OK, the case is so corner, only when :
  - realsize  strlen(accept_line) and
  - ptr is the first part of  accept_line, without NUL-termination
  strncpm will possibly access no more than (strlen(accept_line) -
  realsize) bytes after ptr buffer.
 
  I'll need to check if realsize = strlen(accept_line), not passing 
  realsize.

 You can just pass size * nmemb because strncmp() does check for NUL in
 both strings.  Therefore strlen(accept_line) is not needed - you know
 accept_line is NUL-terminated.


 No, e.g. size * nmemb is 5, and *ptr is Conte, passing size * nmemb to
 strncmp gets zero. We need to:
 * Ensure size * nmemb is no less than needed

That's true, it would match Accept-.  The libcurl docs do say that
only complete headers are provided but the server could return junk so
we need to be careful.

Re: [Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-12 Thread Fam Zheng 
On Tue, 06/11 09:40, Stefan Hajnoczi wrote:
 On Tue, Jun 11, 2013 at 11:15:15AM +0800, Fam Zheng wrote:
  On Mon, Jun 10, 2013 at 5:21 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
   On Sun, Jun 09, 2013 at 10:34:54AM +0800, Fam Zheng wrote:
   @@ -110,14 +111,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
   +s-accept_range = true;
}
  
   This still assumes ptr is NUL-terminated.  You need to pass size * nmemb
   instead of strlen(accept_line).
  
  OK, the case is so corner, only when :
  - realsize  strlen(accept_line) and
  - ptr is the first part of  accept_line, without NUL-termination
  strncpm will possibly access no more than (strlen(accept_line) -
  realsize) bytes after ptr buffer.
  
  I'll need to check if realsize = strlen(accept_line), not passing realsize.
 
 You can just pass size * nmemb because strncmp() does check for NUL in
 both strings.  Therefore strlen(accept_line) is not needed - you know
 accept_line is NUL-terminated.
 

No, e.g. size * nmemb is 5, and *ptr is Conte, passing size * nmemb to
strncmp gets zero. We need to:
* Ensure size * nmemb is no less than needed
* Only compare needed, not whole (first strlen(accept_line) bytes).

-- 
Fam

Re: [Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-11 Thread Stefan Hajnoczi 
On Tue, Jun 11, 2013 at 11:15:15AM +0800, Fam Zheng wrote:
 On Mon, Jun 10, 2013 at 5:21 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
  On Sun, Jun 09, 2013 at 10:34:54AM +0800, Fam Zheng wrote:
  @@ -110,14 +111,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
  +s-accept_range = true;
   }
 
  This still assumes ptr is NUL-terminated.  You need to pass size * nmemb
  instead of strlen(accept_line).
 
 OK, the case is so corner, only when :
 - realsize  strlen(accept_line) and
 - ptr is the first part of  accept_line, without NUL-termination
 strncpm will possibly access no more than (strlen(accept_line) -
 realsize) bytes after ptr buffer.
 
 I'll need to check if realsize = strlen(accept_line), not passing realsize.

You can just pass size * nmemb because strncmp() does check for NUL in
both strings.  Therefore strlen(accept_line) is not needed - you know
accept_line is NUL-terminated.

Stefan

Re: [Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-11 Thread Kevin Wolf 
Am 11.06.2013 um 09:40 hat Stefan Hajnoczi geschrieben:
 On Tue, Jun 11, 2013 at 11:15:15AM +0800, Fam Zheng wrote:
  On Mon, Jun 10, 2013 at 5:21 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
   On Sun, Jun 09, 2013 at 10:34:54AM +0800, Fam Zheng wrote:
   @@ -110,14 +111,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
   +s-accept_range = true;
}
  
   This still assumes ptr is NUL-terminated.  You need to pass size * nmemb
   instead of strlen(accept_line).
  
  OK, the case is so corner, only when :
  - realsize  strlen(accept_line) and
  - ptr is the first part of  accept_line, without NUL-termination
  strncpm will possibly access no more than (strlen(accept_line) -
  realsize) bytes after ptr buffer.
  
  I'll need to check if realsize = strlen(accept_line), not passing realsize.
 
 You can just pass size * nmemb because strncmp() does check for NUL in
 both strings.  Therefore strlen(accept_line) is not needed - you know
 accept_line is NUL-terminated.

Don't you just want to check that ptr _starts_ with accept_line rather
than requiring an exact match?

Kevin

Re: [Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-10 Thread Stefan Hajnoczi 
On Sun, Jun 09, 2013 at 10:34:54AM +0800, Fam Zheng wrote:
 @@ -110,14 +111,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
 +s-accept_range = true;
  }

This still assumes ptr is NUL-terminated.  You need to pass size * nmemb
instead of strlen(accept_line).

Re: [Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-10 Thread Fam Zheng 
On Mon, Jun 10, 2013 at 5:21 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
 On Sun, Jun 09, 2013 at 10:34:54AM +0800, Fam Zheng wrote:
 @@ -110,14 +111,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
 +s-accept_range = true;
  }

 This still assumes ptr is NUL-terminated.  You need to pass size * nmemb
 instead of strlen(accept_line).

OK, the case is so corner, only when :
- realsize  strlen(accept_line) and
- ptr is the first part of  accept_line, without NUL-termination
strncpm will possibly access no more than (strlen(accept_line) -
realsize) bytes after ptr buffer.

I'll need to check if realsize = strlen(accept_line), not passing realsize.

[Qemu-devel] [PATCH] curl: refuse to open URL from HTTP server without range support

2013-06-08 Thread Fam Zheng 
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 rjo...@redhat.com
Signed-off-by: Fam Zheng f...@redhat.com
---
 block/curl.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index b8935fd..b46de69 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,14 @@ 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 (strncmp((char *)ptr, accept_line, strlen(accept_line)) == 0) {
+s-accept_range = true;
 }
 
 return realsize;
@@ -441,8 +442,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 +456,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);
-- 
1.8.3