From 22d5a5a9e1be48fd03feed377096d821935344d0 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 15 Feb 2018 12:57:14 +0000 Subject: [PATCH] core: Ensure that ap_*getline*() return NUL terminated lines on any error. This was done only on buffer full, so be consistent, and fail early if the given buffer can't even hold the NUL bytes (negative or nul size). git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1824303 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 3 ++ server/protocol.c | 76 ++++++++++++++++++++++++++--------------------- 2 files changed, 45 insertions(+), 34 deletions(-) diff --git a/CHANGES b/CHANGES index 379d51f867..91c3208426 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.1 + *) core: For consistency, ensure that read lines are NUL terminated on any + error, not only on buffer full. [Yann Ylavic] + *) logresolve: Fix incorrect behavior or segfault if -c flag is used Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=823259 [Stefan Fritsch] diff --git a/server/protocol.c b/server/protocol.c index d7e73f2339..69634d78bb 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -225,6 +225,11 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, int fold = flags & AP_GETLINE_FOLD; int crlf = flags & AP_GETLINE_CRLF; + if (!n) { + /* Needs room for NUL byte at least */ + return APR_BADARG; + } + /* * Initialize last_char as otherwise a random value will be compared * against APR_ASCII_LF at the end of the loop if bb only contains @@ -238,14 +243,15 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_GETLINE, APR_BLOCK_READ, 0); if (rv != APR_SUCCESS) { - return rv; + goto cleanup; } /* Something horribly wrong happened. Someone didn't block! * (this also happens at the end of each keepalive connection) */ if (APR_BRIGADE_EMPTY(bb)) { - return APR_EGENERAL; + rv = APR_EGENERAL; + goto cleanup; } for (e = APR_BRIGADE_FIRST(bb); @@ -263,7 +269,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ); if (rv != APR_SUCCESS) { - return rv; + goto cleanup; } if (len == 0) { @@ -276,17 +282,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, /* Would this overrun our buffer? If so, we'll die. */ if (n < bytes_handled + len) { - *read = bytes_handled; - if (*s) { - /* ensure this string is NUL terminated */ - if (bytes_handled > 0) { - (*s)[bytes_handled-1] = '\0'; - } - else { - (*s)[0] = '\0'; - } - } - return APR_ENOSPC; + rv = APR_ENOSPC; + goto cleanup; } /* Do we have to handle the allocation ourselves? */ @@ -294,7 +291,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, /* We'll assume the common case where one bucket is enough. */ if (!*s) { current_alloc = len; - *s = apr_palloc(r->pool, current_alloc); + *s = apr_palloc(r->pool, current_alloc + 1); } else if (bytes_handled + len > current_alloc) { /* Increase the buffer size */ @@ -305,7 +302,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, new_size = (bytes_handled + len) * 2; } - new_buffer = apr_palloc(r->pool, new_size); + new_buffer = apr_palloc(r->pool, new_size + 1); /* Copy what we already had. */ memcpy(new_buffer, *s, bytes_handled); @@ -329,19 +326,15 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, } } - if (crlf && (last_char <= *s || last_char[-1] != APR_ASCII_CR)) { - *last_char = '\0'; - bytes_handled = last_char - *s; - *read = bytes_handled; - return APR_EINVAL; - } - - /* Now NUL-terminate the string at the end of the line; + /* Now terminate the string at the end of the line; * if the last-but-one character is a CR, terminate there */ if (last_char > *s && last_char[-1] == APR_ASCII_CR) { last_char--; } - *last_char = '\0'; + else if (crlf) { + rv = APR_EINVAL; + goto cleanup; + } bytes_handled = last_char - *s; /* If we're folding, we have more work to do. @@ -361,7 +354,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_SPECULATIVE, APR_BLOCK_READ, 1); if (rv != APR_SUCCESS) { - return rv; + goto cleanup; } if (APR_BRIGADE_EMPTY(bb)) { @@ -378,7 +371,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ); if (rv != APR_SUCCESS) { apr_brigade_cleanup(bb); - return rv; + goto cleanup; } /* Found one, so call ourselves again to get the next line. @@ -395,10 +388,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, if (c == APR_ASCII_BLANK || c == APR_ASCII_TAB) { /* Do we have enough space? We may be full now. */ if (bytes_handled >= n) { - *read = n; - /* ensure this string is terminated */ - (*s)[n-1] = '\0'; - return APR_ENOSPC; + rv = APR_ENOSPC; + goto cleanup; } else { apr_size_t next_size, next_len; @@ -411,7 +402,6 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, tmp = NULL; } else { - /* We're null terminated. */ tmp = last_char; } @@ -420,7 +410,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, rv = ap_rgetline_core(&tmp, next_size, &next_len, r, 0, bb); if (rv != APR_SUCCESS) { - return rv; + goto cleanup; } if (do_alloc && next_len > 0) { @@ -434,7 +424,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, memcpy(new_buffer, *s, bytes_handled); /* copy the new line, including the trailing null */ - memcpy(new_buffer + bytes_handled, tmp, next_len + 1); + memcpy(new_buffer + bytes_handled, tmp, next_len); *s = new_buffer; } @@ -447,8 +437,21 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n, } } } + +cleanup: + if (bytes_handled >= n) { + bytes_handled = n - 1; + } + if (*s) { + /* ensure the string is NUL terminated */ + (*s)[bytes_handled] = '\0'; + } *read = bytes_handled; + if (rv != APR_SUCCESS) { + return rv; + } + /* PR#43039: We shouldn't accept NULL bytes within the line */ if (strlen(*s) < bytes_handled) { return APR_EINVAL; @@ -487,6 +490,11 @@ AP_DECLARE(int) ap_getline(char *s, int n, request_rec *r, int flags) apr_size_t len; apr_bucket_brigade *tmp_bb; + if (n < 1) { + /* Can't work since we always NUL terminate */ + return -1; + } + tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc); rv = ap_rgetline(&tmp_s, n, &len, r, flags, tmp_bb); apr_brigade_destroy(tmp_bb); -- 2.40.0