winhttp: Escape untrusted URL paths.

dlls/winhttp/request.c

@@ -2517,7 +2517,7 @@ static WCHAR *get_redirect_url( request_t *request, DWORD *len )
     query_headers( request, WINHTTP_QUERY_LOCATION, NULL, NULL, &size, NULL );
     if (get_last_error() != ERROR_INSUFFICIENT_BUFFER) return FALSE;
     if (!(ret = heap_alloc( size ))) return NULL;
-    *len = size / sizeof(WCHAR);
+    *len = size / sizeof(WCHAR) - 1;
     if (query_headers( request, WINHTTP_QUERY_LOCATION, NULL, ret, &size, NULL )) return ret;
     heap_free( ret );
     return NULL;
@@ -2526,42 +2526,42 @@ static WCHAR *get_redirect_url( request_t *request, DWORD *len )
 static BOOL handle_redirect( request_t *request, DWORD status )
 {
     BOOL ret = FALSE;
-    DWORD len, len_url;
+    DWORD len, len_loc;
     URL_COMPONENTS uc;
     connect_t *connect = request->connect;
     INTERNET_PORT port;
     WCHAR *hostname = NULL, *location;
     int index;
 
-    if (!(location = get_redirect_url( request, &len_url ))) return FALSE;
+    if (!(location = get_redirect_url( request, &len_loc ))) return FALSE;
 
     memset( &uc, 0, sizeof(uc) );
     uc.dwStructSize = sizeof(uc);
     uc.dwSchemeLength = uc.dwHostNameLength = uc.dwUrlPathLength = uc.dwExtraInfoLength = ~0u;
 
-    if (!WinHttpCrackUrl( location, len_url, 0, &uc )) /* assume relative redirect */
+    if (!WinHttpCrackUrl( location, len_loc, 0, &uc )) /* assume relative redirect */
     {
         WCHAR *path, *p;
 
         if (location[0] == '/')
         {
-            len = strlenW( location );
+            len = escape_string( NULL, location, len_loc );
             if (!(path = heap_alloc( (len + 1) * sizeof(WCHAR) ))) goto end;
-            strcpyW( path, location );
+            escape_string( path, location, len_loc );
         }
         else
         {
             if ((p = strrchrW( request->path, '/' ))) *p = 0;
-            len = strlenW( request->path ) + 1 + strlenW( location );
+            len = strlenW( request->path ) + 1 + escape_string( NULL, location, len_loc );
             if (!(path = heap_alloc( (len + 1) * sizeof(WCHAR) ))) goto end;
             strcpyW( path, request->path );
             strcatW( path, slashW );
-            strcatW( path, location );
+            escape_string( path + strlenW(path), location, len_loc );
         }
         heap_free( request->path );
         request->path = path;
 
-        send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_REDIRECT, location, len_url + 1 );
+        send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_REDIRECT, location, len_loc + 1 );
     }
     else
     {
@@ -2577,7 +2577,7 @@ static BOOL handle_redirect( request_t *request, DWORD status )
             request->hdr.flags |= WINHTTP_FLAG_SECURE;
         }
 
-        send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_REDIRECT, location, len_url + 1 );
+        send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_REDIRECT, location, len_loc + 1 );
 
         len = uc.dwHostNameLength;
         if (!(hostname = heap_alloc( (len + 1) * sizeof(WCHAR) ))) goto end;
@@ -2607,9 +2607,9 @@ static BOOL handle_redirect( request_t *request, DWORD status )
         request->path = NULL;
         if (uc.dwUrlPathLength)
         {
-            len = uc.dwUrlPathLength + uc.dwExtraInfoLength;
+            len = escape_string( NULL, uc.lpszUrlPath, uc.dwUrlPathLength + uc.dwExtraInfoLength );
             if (!(request->path = heap_alloc( (len + 1) * sizeof(WCHAR) ))) goto end;
-            strcpyW( request->path, uc.lpszUrlPath );
+            escape_string( request->path, uc.lpszUrlPath, uc.dwUrlPathLength + uc.dwExtraInfoLength );
         }
         else request->path = strdupW( slashW );
     }

dlls/winhttp/session.c

@@ -1142,14 +1142,14 @@ HINTERNET WINAPI WinHttpOpenRequest( HINTERNET hconnect, LPCWSTR verb, LPCWSTR o
     if (object)
     {
         WCHAR *path, *p;
-        unsigned int len;
+        unsigned int len, len_object = strlenW(object);
 
-        len = strlenW( object ) + 1;
+        len = escape_string( NULL, object, len_object );
         if (object[0] != '/') len++;
-        if (!(p = path = heap_alloc( len * sizeof(WCHAR) ))) goto end;
+        if (!(p = path = heap_alloc( (len + 1) * sizeof(WCHAR) ))) goto end;
 
         if (object[0] != '/') *p++ = '/';
-        strcpyW( p, object );
+        escape_string( p, object, len_object );
         request->path = path;
     }
     else if (!(request->path = strdupW( slashW ))) goto end;

dlls/winhttp/tests/url.c

@@ -667,6 +667,7 @@ static void WinHttpCrackUrl_test( void )
     ok( ret, "WinHttpCrackUrl failed le=%u\n", GetLastError() );
     ok( !lstrcmpW( uc.lpszHostName, hostnameW ), "unexpected host name\n" );
     ok( !lstrcmpW( uc.lpszUrlPath, pathW ), "unexpected path\n" );
+    ok( uc.dwUrlPathLength == lstrlenW(pathW), "got %u\n", uc.dwUrlPathLength );
 
     uc.dwStructSize = sizeof(uc);
     uc.lpszScheme = NULL;

dlls/winhttp/tests/winhttp.c

@@ -2274,6 +2274,11 @@ static DWORD CALLBACK server_thread(LPVOID param)
             if (!strstr(buffer, "Cookie: name=value\r\n")) send(c, cookiemsg, sizeof(cookiemsg) - 1, 0);
             else send(c, notokmsg, sizeof(notokmsg) - 1, 0);
         }
+        else if (strstr(buffer, "GET /escape"))
+        {
+            if (strstr(buffer, "GET /escape?one%20two%0D%0A HTTP/1.1")) send(c, okmsg, sizeof(okmsg) - 1, 0);
+            else send(c, notokmsg, sizeof(notokmsg) - 1, 0);
+        }
         if (strstr(buffer, "GET /quit"))
         {
             send(c, okmsg, sizeof okmsg - 1, 0);
@@ -3229,6 +3234,40 @@ static void test_cookies( int port )
     WinHttpCloseHandle( ses );
 }
 
+static void test_request_path_escapes( int port )
+{
+    static const WCHAR objW[] =
+        {'/','e','s','c','a','p','e','?','o','n','e',' ','t','w','o','\r','\n',0};
+    HINTERNET ses, con, req;
+    DWORD status, size;
+    BOOL ret;
+
+    ses = WinHttpOpen( test_useragent, WINHTTP_ACCESS_TYPE_NO_PROXY, NULL, NULL, 0 );
+    ok( ses != NULL, "failed to open session %u\n", GetLastError() );
+
+    con = WinHttpConnect( ses, localhostW, port, 0 );
+    ok( con != NULL, "failed to open a connection %u\n", GetLastError() );
+
+    req = WinHttpOpenRequest( con, NULL, objW, NULL, NULL, NULL, 0 );
+    ok( req != NULL, "failed to open a request %u\n", GetLastError() );
+
+    ret = WinHttpSendRequest( req, NULL, 0, NULL, 0, 0, 0 );
+    ok( ret, "failed to send request %u\n", GetLastError() );
+
+    ret = WinHttpReceiveResponse( req, NULL );
+    ok( ret, "failed to receive response %u\n", GetLastError() );
+
+    status = 0xdeadbeef;
+    size = sizeof(status);
+    ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_STATUS_CODE|WINHTTP_QUERY_FLAG_NUMBER, NULL, &status, &size, NULL );
+    ok( ret, "failed to query status code %u\n", GetLastError() );
+    ok( status == HTTP_STATUS_OK, "request failed unexpectedly %u\n", status );
+
+    WinHttpCloseHandle( req );
+    WinHttpCloseHandle( con );
+    WinHttpCloseHandle( ses );
+}
+
 static void test_connection_info( int port )
 {
     static const WCHAR basicW[] = {'/','b','a','s','i','c',0};
@@ -4555,6 +4594,7 @@ START_TEST (winhttp)
     test_bad_header(si.port);
     test_multiple_reads(si.port);
     test_cookies(si.port);
+    test_request_path_escapes(si.port);
 
     /* send the basic request again to shutdown the server thread */
     test_basic_request(si.port, NULL, quitW);

dlls/winhttp/url.c

@@ -118,7 +118,7 @@ static BOOL need_escape( WCHAR c )
     }
 }
 
-static DWORD copy_escape( WCHAR *dst, const WCHAR *src, DWORD len )
+DWORD escape_string( WCHAR *dst, const WCHAR *src, DWORD len )
 {
     static const WCHAR hex[] = {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};
     DWORD ret = len;
@@ -129,38 +129,45 @@ static DWORD copy_escape( WCHAR *dst, const WCHAR *src, DWORD len )
     {
         if (need_escape( src[i] ))
         {
-            p[0] = '%';
-            p[1] = hex[(src[i] >> 4) & 0xf];
-            p[2] = hex[src[i] & 0xf];
+            if (dst)
+            {
+                p[0] = '%';
+                p[1] = hex[(src[i] >> 4) & 0xf];
+                p[2] = hex[src[i] & 0xf];
+                p += 2;
+            }
             ret += 2;
-            p += 2;
         }
-        else *p = src[i];
+        else if (dst) *p = src[i];
     }
-    dst[ret] = 0;
+    if (dst) dst[ret] = 0;
     return ret;
 }
 
-static WCHAR *escape_url( LPCWSTR url, DWORD *len )
+static WCHAR *escape_url( const WCHAR *url, DWORD *len )
 {
     WCHAR *ret;
-    const WCHAR *p, *q;
+    const WCHAR *p;
+    DWORD len_base, len_path;
 
-    if ((p = q = strrchrW( url, '/' )))
+    if ((p = strrchrW( url, '/' )))
     {
-        while (*q)
-        {
-            if (need_escape( *q )) *len += 2;
-            q++;
-        }
+        len_base = p - url;
+        len_path = escape_string( NULL, p, *len - len_base );
     }
-    if (!(ret = heap_alloc( (*len + 1) * sizeof(WCHAR) ))) return NULL;
-    if (!p) strcpyW( ret, url );
     else
     {
-        memcpy( ret, url, (p - url) * sizeof(WCHAR) );
-        copy_escape( ret + (p - url), p, q - p );
+        len_base = *len;
+        len_path = 0;
     }
+
+    if (!(ret = heap_alloc( (len_base + len_path + 1) * sizeof(WCHAR) ))) return NULL;
+    memcpy( ret, url, len_base * sizeof(WCHAR) );
+
+    if (p) escape_string( ret + len_base, p, *len - (p - url) );
+    ret[len_base + len_path] = 0;
+
+    *len = len_base + len_path;
     return ret;
 }
 
@@ -516,7 +523,7 @@ BOOL WINAPI WinHttpCreateUrl( LPURL_COMPONENTS uc, DWORD flags, LPWSTR url, LPDW
     if (uc->lpszUrlPath)
     {
         len = comp_length( uc->dwUrlPathLength, 0, uc->lpszUrlPath );
-        if (flags & ICU_ESCAPE) url += copy_escape( url, uc->lpszUrlPath, len );
+        if (flags & ICU_ESCAPE) url += escape_string( url, uc->lpszUrlPath, len );
         else
         {
             memcpy( url, uc->lpszUrlPath, len * sizeof(WCHAR) );
@@ -526,7 +533,7 @@ BOOL WINAPI WinHttpCreateUrl( LPURL_COMPONENTS uc, DWORD flags, LPWSTR url, LPDW
     if (uc->lpszExtraInfo)
     {
         len = comp_length( uc->dwExtraInfoLength, 0, uc->lpszExtraInfo );
-        if (flags & ICU_ESCAPE) url += copy_escape( url, uc->lpszExtraInfo, len );
+        if (flags & ICU_ESCAPE) url += escape_string( url, uc->lpszExtraInfo, len );
         else
         {
             memcpy( url, uc->lpszExtraInfo, len * sizeof(WCHAR) );

dlls/winhttp/winhttp_private.h

@@ -322,6 +322,7 @@ BOOL set_server_for_hostname( connect_t *, LPCWSTR, INTERNET_PORT ) DECLSPEC_HID
 void destroy_authinfo( struct authinfo * ) DECLSPEC_HIDDEN;
 
 void release_host( hostdata_t *host ) DECLSPEC_HIDDEN;
+DWORD escape_string( WCHAR *, const WCHAR *, DWORD ) DECLSPEC_HIDDEN;
 
 extern HRESULT WinHttpRequest_create( void ** ) DECLSPEC_HIDDEN;
 void release_typelib( void ) DECLSPEC_HIDDEN;