From: Brian Neradt Date: Sat, 21 May 2022 19:14:28 +0000 (+0100) Subject: CVE-2020-17509 X-Git-Tag: archive/raspbian/8.0.2+ds-1+rpi1+deb10u6^2~8 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=c9cf133966f2d4648827d7e311a223f484b1fdcc;p=trafficserver.git CVE-2020-17509 Origin: backport Applied-upstream: https://github.com/apache/trafficserver/pull/7359 Last-Update: 2020-06-25 Last-Update: 2020-06-25 Gbp-Pq: Name 0018-CVE-2020-17509.patch --- diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst index 769e7c04..8449d52e 100644 --- a/doc/admin-guide/files/records.config.en.rst +++ b/doc/admin-guide/files/records.config.en.rst @@ -1482,11 +1482,9 @@ Negative Response Caching ====================== ===================================================== ``204`` No Content ``305`` Use Proxy - ``400`` Bad Request ``403`` Forbidden ``404`` Not Found ``414`` URI Too Long - ``405`` Method Not Allowed ``500`` Internal Server Error ``501`` Not Implemented ``502`` Bad Gateway @@ -1504,7 +1502,7 @@ Negative Response Caching How long (in seconds) Traffic Server keeps the negative responses valid in cache. This value only affects negative responses that do NOT have explicit ``Expires:`` or ``Cache-Control:`` lifetimes set by the server. -.. ts:cv:: CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504 +.. ts:cv:: CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 414 500 501 502 503 504 :reloadable: The HTTP status code for negative caching. Default values are mentioned above. The unwanted status codes can be diff --git a/doc/admin-guide/performance/index.en.rst b/doc/admin-guide/performance/index.en.rst index 18f05dc6..87e999cb 100644 --- a/doc/admin-guide/performance/index.en.rst +++ b/doc/admin-guide/performance/index.en.rst @@ -493,7 +493,7 @@ Error responses from origins are conistent and costly If error responses are costly for your origin server to generate, you may elect to have |TS| cache these responses for a period of time. The default behavior is to consider all of these responses to be uncacheable, which will lead to every -client request to result in an origin request. +client request resulting in an origin request. This behavior is controlled by both enabling the feature via :ts:cv:`proxy.config.http.negative_caching_enabled` and setting the cache time @@ -502,7 +502,7 @@ status code for negative caching can be set with :ts:cv:`proxy.config.http.negat CONFIG proxy.config.http.negative_caching_enabled INT 1 CONFIG proxy.config.http.negative_caching_lifetime INT 10 - CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504 + CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 414 500 501 502 503 504 SSL-Specific Options ~~~~~~~~~~~~~~~~~~~~ diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc index 2fb8572e..41cab3b3 100644 --- a/mgmt/RecordsConfig.cc +++ b/mgmt/RecordsConfig.cc @@ -499,7 +499,7 @@ static const RecordElement RecordsConfig[] = , {RECT_CONFIG, "proxy.config.http.negative_caching_lifetime", RECD_INT, "1800", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , - {RECT_CONFIG, "proxy.config.http.negative_caching_list", RECD_STRING, "204 305 403 404 405 414 500 501 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} + {RECT_CONFIG, "proxy.config.http.negative_caching_list", RECD_STRING, "204 305 403 404 414 500 501 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , // ######################### diff --git a/proxy/http/HttpConfig.cc b/proxy/http/HttpConfig.cc index 39a5a71b..1a0c36d1 100644 --- a/proxy/http/HttpConfig.cc +++ b/proxy/http/HttpConfig.cc @@ -910,7 +910,7 @@ set_negative_caching_list(const char *name, RecDataT dtype, RecData data, HttpCo HttpStatusBitset set; // values from proxy.config.http.negative_caching_list if (0 == strcasecmp("proxy.config.http.negative_caching_list", name) && RECD_STRING == dtype && data.rec_string) { - // parse the list of status code + // parse the list of status codes ts::TextView status_list(data.rec_string, strlen(data.rec_string)); auto is_sep{[](char c) { return isspace(c) || ',' == c || ';' == c; }}; while (!status_list.ltrim_if(is_sep).empty()) { diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index a0a48e8a..e9b35de9 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -2922,7 +2922,7 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p) // the reason string being written to the client and a bad CL when reading from cache. // I didn't find anywhere this appended reason is being used, so commenting it out. /* - if (t_state.negative_caching && p->bytes_read == 0) { + if (t_state.is_cacheable_and_negative_caching_is_enabled && p->bytes_read == 0) { int reason_len; const char *reason = t_state.hdr_info.server_response.reason_get(&reason_len); if (reason == NULL) @@ -2977,8 +2977,8 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p) } // turn off negative caching in case there are multiple server contacts - if (t_state.negative_caching) { - t_state.negative_caching = false; + if (t_state.is_cacheable_and_negative_caching_is_enabled) { + t_state.is_cacheable_and_negative_caching_is_enabled = false; } // If we had a ground fill, check update our status @@ -6531,7 +6531,8 @@ HttpSM::setup_server_transfer() nbytes = server_transfer_init(buf, hdr_size); - if (t_state.negative_caching && t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NO_CONTENT) { + if (t_state.is_cacheable_and_negative_caching_is_enabled && + t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NO_CONTENT) { int s = sizeof("No Content") - 1; buf->write("No Content", s); nbytes += s; diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index be6a6e0e..ea879a7b 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -4150,7 +4150,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s) client_response_code = server_response_code; base_response = &s->hdr_info.server_response; - s->negative_caching = is_negative_caching_appropriate(s) && cacheable; + s->is_cacheable_and_negative_caching_is_enabled = cacheable && s->txn_conf->negative_caching_enabled; // determine the correct cache action given the original cache action, // cacheability of server response, and request method @@ -4185,7 +4185,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s) } } else if (s->cache_info.action == CACHE_DO_WRITE) { - if (!cacheable && !s->negative_caching) { + if (!cacheable) { s->cache_info.action = CACHE_DO_NO_ACTION; } else if (s->method == HTTP_WKSIDX_HEAD) { s->cache_info.action = CACHE_DO_NO_ACTION; @@ -4212,7 +4212,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s) // before issuing a 304 if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION || s->cache_info.action == CACHE_DO_REPLACE) { - if (s->negative_caching) { + if (s->is_cacheable_and_negative_caching_is_enabled) { HTTPHdr *resp; s->cache_info.object_store.create(); s->cache_info.object_store.request_set(&s->hdr_info.client_request); @@ -4248,8 +4248,8 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s) SET_VIA_STRING(VIA_PROXY_RESULT, VIA_PROXY_SERVER_REVALIDATED); } } - } else if (s->negative_caching) { - s->negative_caching = false; + } else if (s->is_cacheable_and_negative_caching_is_enabled) { + s->is_cacheable_and_negative_caching_is_enabled = false; } break; @@ -4659,7 +4659,7 @@ HttpTransact::set_headers_for_cache_write(State *s, HTTPInfo *cache_info, HTTPHd sites yields no insight. So the assert is removed and we keep the behavior that if the response in @a cache_info is already set, we don't override it. */ - if (!s->negative_caching || !cache_info->response_get()->valid()) { + if (!s->is_cacheable_and_negative_caching_is_enabled || !cache_info->response_get()->valid()) { cache_info->response_set(response); } @@ -6111,24 +6111,24 @@ HttpTransact::is_response_cacheable(State *s, HTTPHdr *request, HTTPHdr *respons } } - // default cacheability - if (!s->txn_conf->negative_caching_enabled) { - if ((response_code == HTTP_STATUS_OK) || (response_code == HTTP_STATUS_NOT_MODIFIED) || - (response_code == HTTP_STATUS_NON_AUTHORITATIVE_INFORMATION) || (response_code == HTTP_STATUS_MOVED_PERMANENTLY) || - (response_code == HTTP_STATUS_MULTIPLE_CHOICES) || (response_code == HTTP_STATUS_GONE)) { - TxnDebug("http_trans", "[is_response_cacheable] YES by default "); - return true; - } else { - TxnDebug("http_trans", "[is_response_cacheable] NO by default"); - return false; - } + if ((response_code == HTTP_STATUS_OK) || (response_code == HTTP_STATUS_NOT_MODIFIED) || + (response_code == HTTP_STATUS_NON_AUTHORITATIVE_INFORMATION) || (response_code == HTTP_STATUS_MOVED_PERMANENTLY) || + (response_code == HTTP_STATUS_MULTIPLE_CHOICES) || (response_code == HTTP_STATUS_GONE)) { + TxnDebug("http_trans", "[is_response_cacheable] YES response code seems fine"); + return true; } + // Notice that the following are not overridable by negative caching. if (response_code == HTTP_STATUS_SEE_OTHER || response_code == HTTP_STATUS_UNAUTHORIZED || response_code == HTTP_STATUS_PROXY_AUTHENTICATION_REQUIRED) { return false; } - // let is_negative_caching_approriate decide what to do - return true; + // The response code does not look appropriate for caching. Check, however, + // whether the user has specified it should be cached via negative response + // caching configuration. + if (is_negative_caching_appropriate(s)) { + return true; + } + return false; /* Since we weren't caching response obtained with Authorization (the cache control stuff was commented out previously) I've moved this check to is_request_cache_lookupable(). diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h index e3ee61b2..29b23f29 100644 --- a/proxy/http/HttpTransact.h +++ b/proxy/http/HttpTransact.h @@ -761,8 +761,23 @@ public: bool client_connection_enabled = true; bool acl_filtering_performed = false; - // for negative caching - bool negative_caching = false; + /// True if negative caching is enabled and the response is cacheable. + /// + /// Note carefully that this being true does not necessarily imply that the + /// response code was negative. It means that (a) the response was + /// cacheable apart from response code considerations, and (b) concerning + /// the response code one of the following was true: + /// + /// * The response was a negative response code configured cacheable + /// by the user via negative response caching configuration, or ... + /// + /// * The response code was an otherwise cacheable positive repsonse + /// value (such as a 200 response, for example). + /// + /// TODO: We should consider refactoring this variable and its use. For now + /// I'm giving it an awkwardly long name to make sure the meaning of it is + /// clear in its various contexts. + bool is_cacheable_and_negative_caching_is_enabled = false; // for authenticated content caching CacheAuth_t www_auth_content = CACHE_AUTH_NONE;