Skip to content

Commit

Permalink
Fix #1157, multiple bugs in sending_302 & sending_302_without_prepari…
Browse files Browse the repository at this point in the history 
…ng tests:

1. http_sticky_suite_setup() initializes mock.conn_req as TfwCliConn,
   while it's declared as TfwConn => memory corruption;
2. tfw_connection_send() calls parser on mock.resp, while the actual
   response was allocated in tfw_http_sticky_build_redirect() and
   referenced by mock.req->resp only => bad memory reads;
3. tfw_http_sticky_build_redirect() uses lightweight message allocation
   w/o h_tbl initialization => bad memory reads on parsing;
4. bad error handling - if tfw_http_sticky_build_redirect() fails, then
   mock.req isn't freed and we must not zeroize it.
  • Loading branch information
@krizhanovsky
krizhanovsky committed May 9, 2019
1 parent 5232ac3 commit a648e4f
Showing 1 changed file with 87 additions and 34 deletions.
121 changes: 87 additions & 34 deletions tempesta_fw/t/unit/test_http_sticky.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Tempesta FW
*
* Copyright (C) 2014 NatSys Lab. (info@natsys-lab.com).
* Copyright (C) 2015-2018 Tempesta Technologies, Inc.
* Copyright (C) 2015-2019 Tempesta Technologies, Inc.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -151,7 +151,7 @@ static struct {
char *rmark_val;
TfwHttpReq *req;
TfwHttpResp *resp;
TfwConn conn_req;
TfwCliConn conn_req;
TfwConn conn_resp;
TfwClient client;
struct sock sock;
Expand Down Expand Up @@ -334,7 +334,7 @@ http_sticky_suite_setup(void)
skb_reserve(skb, MAX_TCP_HEADER);
ss_skb_queue_tail(&mock.resp->msg.skb_head, skb);

tfw_connection_init(&mock.conn_req);
tfw_connection_init((TfwConn *)&mock.conn_req);
tfw_connection_init(&mock.conn_resp);
TFW_CONN_TYPE(&mock.conn_req) |= Conn_Clnt;
TFW_CONN_TYPE(&mock.conn_resp) |= Conn_Srv;
Expand All @@ -344,7 +344,7 @@ http_sticky_suite_setup(void)
spin_lock_init(&cli_conn->seq_qlock);
spin_lock_init(&cli_conn->ret_qlock);

tfw_connection_revive(&mock.conn_req);
tfw_connection_revive((TfwConn *)&mock.conn_req);
mock.conn_req.peer = (TfwPeer *)&mock.client;
mock.client.addr.sin6_family = AF_INET6;
/*
Expand All @@ -354,7 +354,7 @@ http_sticky_suite_setup(void)
mock.sock.sk_family = AF_INET6;
mock.conn_req.sk = &mock.sock;

mock.req->conn = &mock.conn_req;
mock.req->conn = (TfwConn *)&mock.conn_req;
mock.resp->conn = &mock.conn_resp;
tfw_http_init_parser_req(mock.req);
tfw_http_init_parser_resp(mock.resp);
Expand All @@ -377,7 +377,8 @@ http_sticky_suite_teardown(void)
mock.req->sess->st_conn.srv_conn = NULL;
tfw_http_msg_free((TfwHttpMsg *)mock.req);
}
tfw_http_msg_free((TfwHttpMsg *)mock.resp);
if (mock.resp)
tfw_http_msg_free((TfwHttpMsg *)mock.resp);

memset(&mock, 0, sizeof(mock));
}
Expand Down Expand Up @@ -414,54 +415,106 @@ TEST(http_sticky, sending_302_without_preparing)
StickyVal sv = {};
TfwConn *c = mock.req->conn;

/*
* The redirect constructor rewrote mock.req->resp, so we have to
* reinitialize mock.resp to be able to parse the response.
* Unpair request and response first to avoid "Response-Request pairing
* is broken!" warning.
* We don't pair response w/ request in http_sticky_suite_setup(),
* so tfw_http_msg_free() doesn't unpair the messages and we have to do
* this manually.
*/
tfw_http_msg_unpair((TfwHttpMsg *)mock.req);
tfw_http_msg_free((TfwHttpMsg *)mock.resp);

/* Cookie is calculated for zero HMAC. */
EXPECT_EQ(tfw_http_sticky_build_redirect(mock.req, &sv, NULL),
TFW_HTTP_SESS_REDIRECT_NEED);
EXPECT_NOT_NULL(mock.req->resp);
if (!mock.req->resp)
goto err;
tfw_http_resp_fwd(mock.req->resp);

mock.resp = mock.req->resp;
mock.resp->conn = &mock.conn_resp;

/*
* tfw_http_sticky_build_redirect() uses quick HTTP message allocator,
* so duplicate part of __tfw_http_msg_alloc() to be able to call
* parser on the response.
*/
mock.resp->h_tbl = (TfwHttpHdrTbl *)tfw_pool_alloc(mock.resp->pool,
TFW_HHTBL_SZ(1));
EXPECT_NOT_NULL(mock.resp->h_tbl);
if (!mock.resp->h_tbl)
goto err;
mock.resp->h_tbl->size = __HHTBL_SZ(1);
mock.resp->h_tbl->off = TFW_HTTP_HDR_RAW;
bzero_fast(mock.resp->h_tbl->tbl, __HHTBL_SZ(1) * sizeof(TfwStr));

tfw_http_resp_fwd(mock.resp);

EXPECT_TRUE(mock.tfw_connection_send_was_called);

/*
* __tfw_http_resp_fwd() calls tfw_http_resp_pair_free(), so both the
* request and response are already freed.
*/
mock.req = NULL;
mock.resp = NULL;
err:
tfw_connection_put(c);
mock.req = NULL; /* already freed */
}

TEST(http_sticky, sending_302)
{
TfwStr *hdr1;
StickyVal sv = { .ts = 1 };
TfwConn *c = mock.req->conn;

/*
* Need host header.
* It must be compound as a special header.
*/
create_str_pool();
hdr1 = make_compound_str2("Host: ", "localhost");

{
StickyVal sv = { .ts = 1 };
TfwConn *c = mock.req->conn;
/* See test sending_302_without_preparing. */
tfw_http_msg_unpair((TfwHttpMsg *)mock.req);
tfw_http_msg_free((TfwHttpMsg *)mock.resp);

/*
* Need host header.
* It must be compound as a special header.
*/
TFW_STR2(hdr1, "Host: ", "localhost");
mock.req->h_tbl->tbl[TFW_HTTP_HDR_HOST] = *hdr1;

mock.req->h_tbl->tbl[TFW_HTTP_HDR_HOST] = *hdr1;
EXPECT_EQ(__sticky_calc(mock.req, &sv), 0);
EXPECT_EQ(tfw_http_sticky_build_redirect(mock.req, &sv, NULL),
TFW_HTTP_SESS_REDIRECT_NEED);
EXPECT_NOT_NULL(mock.req->resp);
if (!mock.req->resp)
goto err;

EXPECT_EQ(__sticky_calc(mock.req, &sv), 0);
EXPECT_EQ(tfw_http_sticky_build_redirect(mock.req, &sv, NULL),
TFW_HTTP_SESS_REDIRECT_NEED);
EXPECT_NOT_NULL(mock.req->resp);
if (!mock.req->resp)
goto err;
tfw_http_resp_fwd(mock.req->resp);
/* See test sending_302_without_preparing. */
mock.resp = mock.req->resp;
mock.resp->conn = &mock.conn_resp;
mock.resp->h_tbl = (TfwHttpHdrTbl *)tfw_pool_alloc(mock.resp->pool,
TFW_HHTBL_SZ(1));
EXPECT_NOT_NULL(mock.resp->h_tbl);
if (!mock.resp->h_tbl)
goto err;
mock.resp->h_tbl->size = __HHTBL_SZ(1);
mock.resp->h_tbl->off = TFW_HTTP_HDR_RAW;
bzero_fast(mock.resp->h_tbl->tbl, __HHTBL_SZ(1) * sizeof(TfwStr));

EXPECT_TRUE(mock.tfw_connection_send_was_called);
EXPECT_TRUE(mock.cookie.seen_set_header);
EXPECT_TRUE(mock.cookie.seen);
EXPECT_EQ(mock.http_status, 302);
tfw_http_resp_fwd(mock.req->resp);

EXPECT_TRUE(mock.tfw_connection_send_was_called);
EXPECT_TRUE(mock.cookie.seen_set_header);
EXPECT_TRUE(mock.cookie.seen);
EXPECT_EQ(mock.http_status, 302);

/* See test sending_302_without_preparing. */
mock.req = NULL;
mock.resp = NULL;
err:
tfw_connection_put(c);
mock.req = NULL; /* already freed */
}
tfw_connection_put(c);

free_all_str();
}
Expand Down Expand Up @@ -633,7 +686,7 @@ TEST(http_sticky, req_no_cookie)

/* since response was modified, we need to parse it again */
EXPECT_EQ(http_parse_resp_helper(), 0);
tfw_connection_send(&mock.conn_req, &mock.resp->msg);
tfw_connection_send((TfwConn *)&mock.conn_req, &mock.resp->msg);

EXPECT_TRUE(mock.tfw_connection_send_was_called);
EXPECT_TRUE(mock.cookie.seen_set_header);
Expand Down Expand Up @@ -664,7 +717,7 @@ TEST(http_sticky, req_have_cookie)

/* since response could be modified, we need to parse it again */
EXPECT_EQ(http_parse_resp_helper(), 0);
tfw_connection_send(&mock.conn_req, &mock.resp->msg);
tfw_connection_send((TfwConn *)&mock.conn_req, &mock.resp->msg);

/* no Set-Cookie headers are expected */
EXPECT_FALSE(mock.cookie.seen_set_header);
Expand Down Expand Up @@ -721,7 +774,7 @@ TEST(http_sticky, req_have_cookie_enforce)

/* since response could be modified, we need to parse it again */
EXPECT_EQ(http_parse_resp_helper(), 0);
tfw_connection_send(&mock.conn_req, &mock.resp->msg);
tfw_connection_send((TfwConn *)&mock.conn_req, &mock.resp->msg);

/* no Set-Cookie headers are expected */
EXPECT_FALSE(mock.cookie.seen_set_header);
Expand Down

0 comments on commit a648e4f

Please sign in to comment.