Skip to content

Treat TRACE with body as bad request #7905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shinrich merged 3 commits into from
Jun 23, 2021
Merged

Treat TRACE with body as bad request #7905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11fdd77
Treat TRACE with body as bad request
shinrich May 26, 2021
ccd6fcc
Fix assert
shinrich Jun 1, 2021
5c1adf6
Fix tests and ensure client_request values are set before checking
shinrich Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions proxy/http/HttpSM.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,11 +899,17 @@ HttpSM::state_read_client_request_header(int event, void *data)
}
}

if (t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_TRACE ||
(t_state.hdr_info.client_request.get_content_length() == 0 &&
t_state.client_info.transfer_encoding != HttpTransact::CHUNKED_ENCODING)) {
// Call to ensure the content-length and transfer_encoding elements in client_request are filled in
HttpTransact::set_client_request_state(&t_state, &t_state.hdr_info.client_request);

if (t_state.hdr_info.client_request.get_content_length() == 0 &&
t_state.client_info.transfer_encoding != HttpTransact::CHUNKED_ENCODING) {
// Enable further IO to watch for client aborts
ua_entry->read_vio->reenable();
} else if (t_state.hdr_info.client_request.method_get_wksidx() == HTTP_WKSIDX_TRACE) {
// Trace with request body is not allowed
call_transact_and_set_next_state(HttpTransact::BadRequest);
return 0;
} else {
// Disable further I/O on the client since there could
// be body that we are tunneling POST/PUT/CONNECT or
Expand Down
82 changes: 44 additions & 38 deletions proxy/http/HttpTransact.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5346,6 +5346,9 @@ HttpTransact::add_client_ip_to_outgoing_request(State *s, HTTPHdr *request)
HttpTransact::RequestError_t
HttpTransact::check_request_validity(State *s, HTTPHdr *incoming_hdr)
{
// Called also on receiving request. Not sure if we need to call this again in case
// the transfer-encoding and content-length headers changed
set_client_request_state(s, incoming_hdr);
if (incoming_hdr == nullptr) {
return NON_EXISTANT_REQUEST_HEADER;
}
Expand All @@ -5369,44 +5372,6 @@ HttpTransact::check_request_validity(State *s, HTTPHdr *incoming_hdr)
int scheme = incoming_url->scheme_get_wksidx();
int method = incoming_hdr->method_get_wksidx();

// Check for chunked encoding
if (incoming_hdr->presence(MIME_PRESENCE_TRANSFER_ENCODING)) {
MIMEField *field = incoming_hdr->field_find(MIME_FIELD_TRANSFER_ENCODING, MIME_LEN_TRANSFER_ENCODING);
if (field) {
HdrCsvIter enc_val_iter;
int enc_val_len;
const char *enc_value = enc_val_iter.get_first(field, &enc_val_len);

while (enc_value) {
const char *wks_value = hdrtoken_string_to_wks(enc_value, enc_val_len);
if (wks_value == HTTP_VALUE_CHUNKED) {
s->client_info.transfer_encoding = CHUNKED_ENCODING;
break;
}
enc_value = enc_val_iter.get_next(&enc_val_len);
}
}
}

/////////////////////////////////////////////////////
// get request content length //
// To avoid parsing content-length twice, we set //
// s->hdr_info.request_content_length here rather //
// than in initialize_state_variables_from_request //
/////////////////////////////////////////////////////
if (method != HTTP_WKSIDX_TRACE) {
if (incoming_hdr->presence(MIME_PRESENCE_CONTENT_LENGTH)) {
s->hdr_info.request_content_length = incoming_hdr->get_content_length();
} else {
s->hdr_info.request_content_length = HTTP_UNDEFINED_CL; // content length less than zero is invalid
}

TxnDebug("http_trans", "[init_stat_vars_from_req] set req cont length to %" PRId64, s->hdr_info.request_content_length);

} else {
s->hdr_info.request_content_length = 0;
}

if (!((scheme == URL_WKSIDX_HTTP) && (method == HTTP_WKSIDX_GET))) {
if (scheme != URL_WKSIDX_HTTP && scheme != URL_WKSIDX_HTTPS && method != HTTP_WKSIDX_CONNECT &&
!((scheme == URL_WKSIDX_WS || scheme == URL_WKSIDX_WSS) && s->is_websocket)) {
Expand Down Expand Up @@ -5485,6 +5450,47 @@ HttpTransact::check_request_validity(State *s, HTTPHdr *incoming_hdr)
return NO_REQUEST_HEADER_ERROR;
}

void
HttpTransact::set_client_request_state(State *s, HTTPHdr *incoming_hdr)
{
if (incoming_hdr == nullptr) {
return;
}

// Set transfer_encoding value
if (incoming_hdr->presence(MIME_PRESENCE_TRANSFER_ENCODING)) {
MIMEField *field = incoming_hdr->field_find(MIME_FIELD_TRANSFER_ENCODING, MIME_LEN_TRANSFER_ENCODING);
if (field) {
HdrCsvIter enc_val_iter;
int enc_val_len;
const char *enc_value = enc_val_iter.get_first(field, &enc_val_len);

while (enc_value) {
const char *wks_value = hdrtoken_string_to_wks(enc_value, enc_val_len);
if (wks_value == HTTP_VALUE_CHUNKED) {
s->client_info.transfer_encoding = CHUNKED_ENCODING;
break;
}
enc_value = enc_val_iter.get_next(&enc_val_len);
}
}
}

/////////////////////////////////////////////////////
// get request content length //
// To avoid parsing content-length twice, we set //
// s->hdr_info.request_content_length here rather //
// than in initialize_state_variables_from_request //
/////////////////////////////////////////////////////
if (incoming_hdr->presence(MIME_PRESENCE_CONTENT_LENGTH)) {
s->hdr_info.request_content_length = incoming_hdr->get_content_length();
} else {
s->hdr_info.request_content_length = HTTP_UNDEFINED_CL; // content length less than zero is invalid
}

TxnDebug("http_trans", "[set_client_request_state] set req cont length to %" PRId64, s->hdr_info.request_content_length);
}

HttpTransact::ResponseError_t
HttpTransact::check_response_validity(State *s, HTTPHdr *incoming_hdr)
{
Expand Down
1 change: 1 addition & 0 deletions proxy/http/HttpTransact.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,7 @@ class HttpTransact
static void add_client_ip_to_outgoing_request(State *s, HTTPHdr *request);
static RequestError_t check_request_validity(State *s, HTTPHdr *incoming_hdr);
static ResponseError_t check_response_validity(State *s, HTTPHdr *incoming_hdr);
static void set_client_request_state(State *s, HTTPHdr *incoming_hdr);
static bool delete_all_document_alternates_and_return(State *s, bool cache_hit);
static bool does_client_request_permit_cached_response(const OverridableHttpConfigParams *p, CacheControlResult *c, HTTPHdr *h,
char *via_string);
Expand Down
30 changes: 8 additions & 22 deletions tests/gold_tests/headers/gold/bad_good_request.gold
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,9 @@
HTTP/1.1 400 Invalid HTTP Request
Date: ``
Connection: close
Server: ATS/``
Cache-Control: no-store
Content-Type: text/html
Content-Language: en
Content-Length: 219

<HTML>
<HEAD>
``HTTP/1.1 400 Invalid HTTP Request
``Connection: close
``Server: ATS/``
``Content-Length: 219
``
<TITLE>Bad Request</TITLE>
</HEAD>

<BODY BGCOLOR="white" FGCOLOR="black">
<H1>Bad Request</H1>
<HR>

<FONT FACE="Helvetica,Arial"><B>
Description: Could not process this request.
</B></FONT>
<HR>
</BODY>
``<H1>Bad Request</H1>
``Description: Could not process this request.
``
27 changes: 27 additions & 0 deletions tests/gold_tests/headers/good_request_after_bad.test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
Test.Summary = '''
Verify that request following a ill-formed request is not processed
'''
Test.ContinueOnFail = True
ts = Test.MakeATSProcess("ts", enable_cache=True)

ts.Disk.records_config.update({'proxy.config.diags.debug.tags': 'http',
Expand Down Expand Up @@ -64,3 +65,29 @@
ts.Variables.port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.stdout = 'gold/bad_te_value.gold'

# TRACE request with a body
tr = Test.AddTestRun("Trace request with a body")
tr.Processes.Default.Command = 'printf "TRACE /foo HTTP/1.1\r\nHost: bob\r\nContent-length:2\r\n\r\nokGET / HTTP/1.1\r\nHost: boa\r\n\r\n" | nc 127.0.0.1 {}'.format(
ts.Variables.port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.stdout = 'gold/bad_good_request.gold'

tr = Test.AddTestRun("Trace request with a chunked body")
tr.Processes.Default.Command = 'printf "TRACE /foo HTTP/1.1\r\nHost: bob\r\ntransfer-encoding: chunked\r\n\r\n2\r\nokGGET / HTTP/1.1\r\nHost: boa\r\n\r\n" | nc 127.0.0.1 {}'.format(
ts.Variables.port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.stdout = 'gold/bad_good_request.gold'

tr = Test.AddTestRun("Trace request with a chunked body via curl")
tr.Processes.Default.Command = 'curl -v --http1.1 --header "Transfer-Encoding: chunked" -d aaa -X TRACE -k http://127.0.0.1:{}/foo'.format(
ts.Variables.port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.All = 'gold/bad_good_request.gold'

tr = Test.AddTestRun("Trace request via curl")
tr.Processes.Default.Command = 'curl -v --http1.1 -X TRACE -k http://127.0.0.1:{}/bar'.format(ts.Variables.port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.All = Testers.ContainsExpression(
r"HTTP/1.1 501 Unsupported method \('TRACE'\)",
"microserver does not support TRACE")