Fix corner cases in http parsing

This patch adds checks for malloc / strdup, and free all memory
when such call fails.

diff --git a/libfreerdp/core/gateway/http.c b/libfreerdp/core/gateway/http.c
index d35a2c8..b71683d 100644 (file)
--- a/libfreerdp/core/gateway/http.c
+++ b/libfreerdp/core/gateway/http.c
@@ -30,14 +30,7 @@
 
 HttpContext* http_context_new()
 {
-       HttpContext* http_context = (HttpContext*) malloc(sizeof(HttpContext));
-
-       if (http_context != NULL)
-       {
-               ZeroMemory(http_context, sizeof(HttpContext));
-       }
-
-       return http_context;
+       return (HttpContext *)calloc(1, sizeof(HttpContext));
 }
 
 void http_context_set_method(HttpContext* http_context, char* method)
@@ -46,6 +39,7 @@ void http_context_set_method(HttpContext* http_context, char* method)
                free(http_context->Method);
 
        http_context->Method = _strdup(method);
+       // TODO: check result
 }
 
 void http_context_set_uri(HttpContext* http_context, char* uri)
@@ -54,6 +48,7 @@ void http_context_set_uri(HttpContext* http_context, char* uri)
                free(http_context->URI);
 
        http_context->URI = _strdup(uri);
+       // TODO: check result
 }
 
 void http_context_set_user_agent(HttpContext* http_context, char* user_agent)
@@ -62,6 +57,7 @@ void http_context_set_user_agent(HttpContext* http_context, char* user_agent)
                free(http_context->UserAgent);
 
        http_context->UserAgent = _strdup(user_agent);
+       // TODO: check result
 }
 
 void http_context_set_host(HttpContext* http_context, char* host)
@@ -70,6 +66,7 @@ void http_context_set_host(HttpContext* http_context, char* host)
                free(http_context->Host);
 
        http_context->Host = _strdup(host);
+       // TODO: check result
 }
 
 void http_context_set_accept(HttpContext* http_context, char* accept)
@@ -78,6 +75,7 @@ void http_context_set_accept(HttpContext* http_context, char* accept)
                free(http_context->Accept);
 
        http_context->Accept = _strdup(accept);
+       // TODO: check result
 }
 
 void http_context_set_cache_control(HttpContext* http_context, char* cache_control)
@@ -86,6 +84,7 @@ void http_context_set_cache_control(HttpContext* http_context, char* cache_contr
                free(http_context->CacheControl);
 
        http_context->CacheControl = _strdup(cache_control);
+       // TODO: check result
 }
 
 void http_context_set_connection(HttpContext* http_context, char* connection)
@@ -94,6 +93,7 @@ void http_context_set_connection(HttpContext* http_context, char* connection)
                free(http_context->Connection);
 
        http_context->Connection = _strdup(connection);
+       // TODO: check result
 }
 
 void http_context_set_pragma(HttpContext* http_context, char* pragma)
@@ -102,6 +102,7 @@ void http_context_set_pragma(HttpContext* http_context, char* pragma)
                free(http_context->Pragma);
 
        http_context->Pragma = _strdup(pragma);
+       // TODO: check result
 }
 
 void http_context_free(HttpContext* http_context)
@@ -126,6 +127,7 @@ void http_request_set_method(HttpRequest* http_request, char* method)
                free(http_request->Method);
 
        http_request->Method = _strdup(method);
+       // TODO: check result
 }
 
 void http_request_set_uri(HttpRequest* http_request, char* uri)
@@ -134,6 +136,7 @@ void http_request_set_uri(HttpRequest* http_request, char* uri)
                free(http_request->URI);
 
        http_request->URI = _strdup(uri);
+       // TODO: check result
 }
 
 void http_request_set_auth_scheme(HttpRequest* http_request, char* auth_scheme)
@@ -142,6 +145,7 @@ void http_request_set_auth_scheme(HttpRequest* http_request, char* auth_scheme)
                free(http_request->AuthScheme);
 
        http_request->AuthScheme = _strdup(auth_scheme);
+       // TODO: check result
 }
 
 void http_request_set_auth_param(HttpRequest* http_request, char* auth_param)
@@ -150,6 +154,7 @@ void http_request_set_auth_param(HttpRequest* http_request, char* auth_param)
                free(http_request->AuthParam);
 
        http_request->AuthParam = _strdup(auth_param);
+       // TODO: check result
 }
 
 char* http_encode_body_line(char* param, char* value)
@@ -159,6 +164,8 @@ char* http_encode_body_line(char* param, char* value)
 
        length = strlen(param) + strlen(value) + 2;
        line = (char*) malloc(length + 1);
+       if (!line)
+               return NULL;
        sprintf_s(line, length + 1, "%s: %s", param, value);
 
        return line;
@@ -172,7 +179,9 @@ char* http_encode_content_length_line(int ContentLength)
 
        _itoa_s(ContentLength, str, sizeof(str), 10);
        length = strlen("Content-Length") + strlen(str) + 2;
-       line = (char*) malloc(length + 1);
+       line = (char *)malloc(length + 1);
+       if (!line)
+               return NULL;
        sprintf_s(line, length + 1, "Content-Length: %s", str);
 
        return line;
@@ -184,7 +193,9 @@ char* http_encode_header_line(char* Method, char* URI)
        int length;
 
        length = strlen("HTTP/1.1") + strlen(Method) + strlen(URI) + 2;
-       line = (char*) malloc(length + 1);
+       line = (char *)malloc(length + 1);
+       if (!line)
+               return NULL;
        sprintf_s(line, length + 1, "%s %s HTTP/1.1", Method, URI);
 
        return line;
@@ -197,6 +208,8 @@ char* http_encode_authorization_line(char* AuthScheme, char* AuthParam)
 
        length = strlen("Authorization") + strlen(AuthScheme) + strlen(AuthParam) + 3;
        line = (char*) malloc(length + 1);
+       if (!line)
+               return NULL;
        sprintf_s(line, length + 1, "Authorization: %s %s", AuthScheme, AuthParam);
 
        return line;
@@ -204,81 +217,101 @@ char* http_encode_authorization_line(char* AuthScheme, char* AuthParam)
 
 wStream* http_request_write(HttpContext* http_context, HttpRequest* http_request)
 {
-       int i;
+       int i, count;
+       char **lines;
        wStream* s;
        int length = 0;
 
-       http_request->count = 9;
-       http_request->lines = (char**) malloc(sizeof(char*) * http_request->count);
-
-       http_request->lines[0] = http_encode_header_line(http_request->Method, http_request->URI);
-       http_request->lines[1] = http_encode_body_line("Cache-Control", http_context->CacheControl);
-       http_request->lines[2] = http_encode_body_line("Connection", http_context->Connection);
-       http_request->lines[3] = http_encode_body_line("Pragma", http_context->Pragma);
-       http_request->lines[4] = http_encode_body_line("Accept", http_context->Accept);
-       http_request->lines[5] = http_encode_body_line("User-Agent", http_context->UserAgent);
-       http_request->lines[6] = http_encode_content_length_line(http_request->ContentLength);
-       http_request->lines[7] = http_encode_body_line("Host", http_context->Host);
+       count = 9;
+       lines = (char **)calloc(count, sizeof(char *));
+       if (!lines)
+               return NULL;
+
+       lines[0] = http_encode_header_line(http_request->Method, http_request->URI);
+       lines[1] = http_encode_body_line("Cache-Control", http_context->CacheControl);
+       lines[2] = http_encode_body_line("Connection", http_context->Connection);
+       lines[3] = http_encode_body_line("Pragma", http_context->Pragma);
+       lines[4] = http_encode_body_line("Accept", http_context->Accept);
+       lines[5] = http_encode_body_line("User-Agent", http_context->UserAgent);
+       lines[6] = http_encode_content_length_line(http_request->ContentLength);
+       lines[7] = http_encode_body_line("Host", http_context->Host);
+
+       /* check that everything went well */
+       for (i = 0; i < 8; i++)
+       {
+               if (!lines[i])
+                       goto out_free;
+       }
 
        if (http_request->Authorization != NULL)
        {
-               http_request->lines[8] = http_encode_body_line("Authorization", http_request->Authorization);
+               lines[8] = http_encode_body_line("Authorization", http_request->Authorization);
+               if (!lines[8])
+                       goto out_free;
        }
        else if ((http_request->AuthScheme != NULL) && (http_request->AuthParam != NULL))
        {
-               http_request->lines[8] = http_encode_authorization_line(http_request->AuthScheme, http_request->AuthParam);
+               lines[8] = http_encode_authorization_line(http_request->AuthScheme, http_request->AuthParam);
+               if (!lines[8])
+                       goto out_free;
        }
 
-       for (i = 0; i < http_request->count; i++)
+       for (i = 0; i < count; i++)
        {
-               length += (strlen(http_request->lines[i]) + 2); /* add +2 for each '\r\n' character */
+               length += (strlen(lines[i]) + 2); /* add +2 for each '\r\n' character */
        }
        length += 2; /* empty line "\r\n" at end of header */
        length += 1; /* null terminator */
 
        s = Stream_New(NULL, length);
+       if (!s)
+               goto out_free;
 
-       for (i = 0; i < http_request->count; i++)
+       for (i = 0; i < count; i++)
        {
-               Stream_Write(s, http_request->lines[i], strlen(http_request->lines[i]));
+               Stream_Write(s, lines[i], strlen(lines[i]));
                Stream_Write(s, "\r\n", 2);
-               free(http_request->lines[i]);
+               free(lines[i]);
        }
        Stream_Write(s, "\r\n", 2);
 
-       free(http_request->lines);
+       free(lines);
 
        Stream_Write(s, "\0", 1); /* append null terminator */
        Stream_Rewind(s, 1); /* don't include null terminator in length */
        Stream_Length(s) = Stream_GetPosition(s);
-
        return s;
-}
-
-HttpRequest* http_request_new()
-{
-       HttpRequest* http_request = (HttpRequest*) malloc(sizeof(HttpRequest));
 
-       if (http_request != NULL)
+out_free:
+       for (i = 0; i < 9; i++)
        {
-               ZeroMemory(http_request, sizeof(HttpRequest));
+               if (lines[i])
+                       free(lines[i]);
        }
+       free(lines);
+       return NULL;
+}
 
-       return http_request;
+HttpRequest* http_request_new()
+{
+       return (HttpRequest*) calloc(1, sizeof(HttpRequest));
 }
 
 void http_request_free(HttpRequest* http_request)
 {
-       if (http_request != NULL)
-       {
+       if (!http_request)
+               return;
+
+       if (http_request->AuthParam)
                free(http_request->AuthParam);
+       if (http_request->AuthScheme)
                free(http_request->AuthScheme);
+       if (http_request->Authorization)
                free(http_request->Authorization);
-               free(http_request->Content);
-               free(http_request->Method);
-               free(http_request->URI);
-               free(http_request);
-       }
+       free(http_request->Content);
+       free(http_request->Method);
+       free(http_request->URI);
+       free(http_request);
 }
 
 BOOL http_response_parse_header_status_line(HttpResponse* http_response, char* status_line)
@@ -300,11 +333,13 @@ BOOL http_response_parse_header_status_line(HttpResponse* http_response, char* s
        *separator = '\0';
        http_response->StatusCode = atoi(status_code);
        http_response->ReasonPhrase = _strdup(reason_phrase);
+       if (!http_response->ReasonPhrase)
+               return FALSE;
        *separator = ' ';
        return TRUE;
 }
 
-void http_response_parse_header_field(HttpResponse* http_response, char* name, char* value)
+BOOL http_response_parse_header_field(HttpResponse* http_response, char* name, char* value)
 {
        if (_stricmp(name, "Content-Length") == 0)
        {
@@ -315,6 +350,8 @@ void http_response_parse_header_field(HttpResponse* http_response, char* name, c
                char* separator;
 
                http_response->Authorization = _strdup(value);
+               if (!http_response->Authorization)
+                       return FALSE;
 
                separator = strchr(value, ' ');
 
@@ -323,6 +360,8 @@ void http_response_parse_header_field(HttpResponse* http_response, char* name, c
                        *separator = '\0';
                        http_response->AuthScheme = _strdup(value);
                        http_response->AuthParam = _strdup(separator + 1);
+                       if (!http_response->AuthScheme || !http_response->AuthParam)
+                               return FALSE;
                        *separator = ' ';
                }
        }
@@ -335,7 +374,7 @@ void http_response_parse_header_field(HttpResponse* http_response, char* name, c
                if (separator != NULL)
                {
                        /* WWW-Authenticate: parameter with spaces="value" */
-                       return;
+                       return FALSE;
                }
 
                separator = strchr(value, ' ');
@@ -349,9 +388,10 @@ void http_response_parse_header_field(HttpResponse* http_response, char* name, c
                        http_response->AuthParam = _strdup(separator + 1);
                        *separator = ' ';
 
-                       return;
+                       return TRUE;
                }
        }
+       return TRUE;
 }
 
 BOOL http_response_parse_header(HttpResponse* http_response)
@@ -412,7 +452,8 @@ BOOL http_response_parse_header(HttpResponse* http_response)
                                break;
                }
 
-               http_response_parse_header_field(http_response, name, value);
+               if (!http_response_parse_header_field(http_response, name, value))
+                       return FALSE;
 
                *end_of_header = end_of_header_char;
        }
@@ -445,7 +486,12 @@ HttpResponse* http_response_recv(rdpTls* tls)
        length = 10000;
        content = NULL;
        buffer = malloc(length);
+       if (!buffer)
+               return NULL;
+
        http_response = http_response_new();
+       if (!http_response)
+               goto out_free;
 
        p = buffer;
        http_response->ContentLength = 0;
@@ -456,20 +502,14 @@ HttpResponse* http_response_recv(rdpTls* tls)
                {
                        status = tls_read(tls, p, length - nbytes);
             
-                       if (status > 0)
-                       {
-                               nbytes += status;
-                               p = (BYTE*) &buffer[nbytes];
-                       }
-                       else if (status == 0)
-                       {
+                       if (status < 0)
+                               goto out_error;
+
+                       if (!status)
                                continue;
-                       }
-                       else
-                       {
-                               http_response_free(http_response);
-                               return NULL;
-                       }
+
+                       nbytes += status;
+                       p = (BYTE*) &buffer[nbytes];
                }
 
                header_end = strstr((char*) buffer, "\r\n\r\n");
@@ -478,8 +518,7 @@ HttpResponse* http_response_recv(rdpTls* tls)
                {
                        fprintf(stderr, "http_response_recv: invalid response:\n");
                        winpr_HexDump(buffer, status);
-                       http_response_free(http_response);
-                       return NULL;
+                       goto out_error;
                }
 
                header_end += 2;
@@ -505,8 +544,9 @@ HttpResponse* http_response_recv(rdpTls* tls)
                        http_response->count = count;
                        if (count)
                        {
-                               http_response->lines = (char**) malloc(sizeof(char*) * http_response->count);
-                               ZeroMemory(http_response->lines, sizeof(char*) * http_response->count);
+                               http_response->lines = (char **)calloc(http_response->count, sizeof(char *));
+                               if (!http_response->lines)
+                                       goto out_error;
                        }
 
                        count = 0;
@@ -515,19 +555,21 @@ HttpResponse* http_response_recv(rdpTls* tls)
                        while (line != NULL)
                        {
                                http_response->lines[count] = _strdup(line);
+                               if (!http_response->lines[count])
+                                       goto out_error;
+
                                line = strtok(NULL, "\r\n");
                                count++;
                        }
 
                        if (!http_response_parse_header(http_response))
-                       {
-                               http_response_free(http_response);
-                               return NULL;
-                       }
+                               goto out_error;
 
                        if (http_response->ContentLength > 0)
                        {
                                http_response->Content = _strdup(content);
+                               if (!http_response->Content)
+                                       goto out_error;
                        }
 
                        break;
@@ -544,20 +586,17 @@ HttpResponse* http_response_recv(rdpTls* tls)
        free(buffer);
 
        return http_response;
+
+out_error:
+       http_response_free(http_response);
+out_free:
+       free(buffer);
+       return NULL;
 }
 
 HttpResponse* http_response_new()
 {
-       HttpResponse* http_response;
-
-       http_response = (HttpResponse*) malloc(sizeof(HttpResponse));
-
-       if (http_response != NULL)
-       {
-               ZeroMemory(http_response, sizeof(HttpResponse));
-       }
-
-       return http_response;
+       return (HttpResponse *)calloc(1, sizeof(HttpResponse));
 }
 
 void http_response_free(HttpResponse* http_response)

diff --git a/libfreerdp/core/gateway/http.h b/libfreerdp/core/gateway/http.h
index 3869795..ac223f1 100644 (file)
--- a/libfreerdp/core/gateway/http.h
+++ b/libfreerdp/core/gateway/http.h
@@ -55,9 +55,6 @@ void http_context_free(HttpContext* http_context);
 
 struct _http_request
 {
-       int count;
-       char** lines;
-
        char* Method;
        char* URI;
        char* AuthScheme;