Skip to content
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

split skb as soon http_parser returned TFW_PASS #1134

Merged
merged 3 commits into from
Dec 28, 2018
+104 −71
Merged

split skb as soon http_parser returned TFW_PASS #1134

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b024333
split skb as soon http_parser returned TFW_PASS
vankoven Dec 13, 2018
8d60fd8
adress code review comments
vankoven Dec 21, 2018
72ba622
Fix skb leakage if client connection is to be closed after current re…
vankoven Dec 27, 2018
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
175 changes: 104 additions & 71 deletions tempesta_fw/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1890,44 +1890,21 @@ tfw_http_conn_send(TfwConn *conn, TfwMsg *msg)
}

/**
* Create a sibling for @msg message.
* Create a sibling for @hm message.
* Siblings in HTTP are pipelined HTTP messages that share the same SKB.
*/
static TfwHttpMsg *
tfw_http_msg_create_sibling(TfwHttpMsg *hm, struct sk_buff **skb,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like function comment is outdated: @msg -> @hm.

unsigned int split_offset)
tfw_http_msg_create_sibling(TfwHttpMsg *hm, struct sk_buff *skb)
{
TfwHttpMsg *shm;
struct sk_buff *nskb;

TFW_DBG2("Create sibling message: conn %p, skb %p\n", hm->conn, skb);
TFW_DBG2("Create sibling message: conn %p, msg: %p, skb %p\n",
hm->conn, hm, skb);

/* The sibling message belongs to the same connection. */
shm = (TfwHttpMsg *)tfw_http_conn_msg_alloc(hm->conn);
if (unlikely(!shm)) {
/*
* Split skb to proceed with current message @hm, or rest of
* data in this connection will be attached to the message.
* Don't care about client connection, it must be closed and
* request @hm is to be destroyed and won't be forwarded to
* backend server.
*/
if (TFW_CONN_TYPE(hm->conn) & Conn_Srv)
*skb = ss_skb_split(*skb, split_offset);
if (unlikely(!shm))
return NULL;
}

/*
* The sibling message is set up to start with a new SKB.
* The new SKB is split off from the original SKB and has
* the first part of the new message. The original SKB is
* shrunk to have just data from the original message.
*/
nskb = ss_skb_split(*skb, split_offset);
if (!nskb) {
tfw_http_conn_msg_free(shm);
return NULL;
}

/*
* New message created, so it should be in whitelist if
Expand All @@ -1936,11 +1913,10 @@ tfw_http_msg_create_sibling(TfwHttpMsg *hm, struct sk_buff **skb,
*/
if (TFW_CONN_TYPE(hm->conn) & Conn_Clnt) {
shm->flags |= hm->flags & TFW_HTTP_F_WHITELIST;
nskb->mark = (*skb)->mark;
skb->mark = hm->msg.skb_head->mark;
}

ss_skb_queue_tail(&shm->msg.skb_head, nskb);
*skb = nskb;
ss_skb_queue_tail(&shm->msg.skb_head, skb);

return shm;
}
Expand Down Expand Up @@ -2362,6 +2338,13 @@ tfw_http_req_mark_error(TfwHttpReq *req, int status)
tfw_http_error_resp_switch(req, status);
}

static void
tfw_http_conn_error_log(TfwConn *conn, const char *msg)
{
if (!(tfw_blk_flags & TFW_BLK_ERR_NOLOG))
TFW_WARN_ADDR(msg, &conn->peer->addr, TFW_WITH_PORT);
}

/**
* Functions define logging and response behaviour during detection of
* malformed or malicious messages. Mark client connection in special
Expand Down Expand Up @@ -2687,20 +2670,19 @@ tfw_http_req_process(TfwConn *conn, const TfwFsmData *data)
struct sk_buff *skb = data->skb;
unsigned int off = data->off;
unsigned int data_off = off;
unsigned int skb_len = skb->len;
TfwFsmData data_up;

BUG_ON(!conn->msg);
BUG_ON(data_off >= skb_len);
BUG_ON(data_off >= skb->len);

TFW_DBG2("Received %u client data bytes on conn=%p msg=%p\n",
skb_len - off, conn, conn->msg);
skb->len - off, conn, conn->msg);

/*
* Process pipelined requests in a loop
* until all data in the SKB is processed.
*/
while (data_off < skb_len) {
while (skb) {
int req_conn_close;
bool block = false;
TfwHttpMsg *hmsib = NULL;
Expand All @@ -2724,7 +2706,7 @@ tfw_http_req_process(TfwConn *conn, const TfwFsmData *data)

TFW_DBG2("Request parsed: len=%u parsed=%d msg_len=%lu"
" ver=%d res=%d\n",
skb_len - off, data_off - off, req->msg.len,
skb->len - off, data_off - off, req->msg.len,
req->version, r);

/*
Expand Down Expand Up @@ -2774,6 +2756,27 @@ tfw_http_req_process(TfwConn *conn, const TfwFsmData *data)
&& (req->content_length != req->body.len));
}

/*
* The message is fully parsed, the rest of the data in the
* stream may represent another request or its part.
* If skb splitting has failed, the request can't be forwarded
* to backend server or request-response sequence can be broken.
* @skb is replaced with pointer to a new SKB.
*/
if (data_off < skb->len) {
skb = ss_skb_split(skb, data_off);
if (unlikely(!skb)) {
TFW_INC_STAT_BH(clnt.msgs_otherr);
tfw_client_block(req, 500,
"Can't split pipelined "
"requests");
return TFW_BLOCK;
}
}
else {
skb = NULL;
}

/* Assign the right virtual host for current request. */
if ((req->vhost = tfw_http_tbl_vhost((TfwMsg *)req, &block)))
req->location = tfw_location_match(req->vhost,
Expand Down Expand Up @@ -2841,27 +2844,32 @@ tfw_http_req_process(TfwConn *conn, const TfwFsmData *data)
* and, at the same time, to stop passing data for processing
* from the lower layer.
*/
if((req_conn_close = req->flags & TFW_HTTP_F_CONN_CLOSE))
if ((req_conn_close = req->flags & TFW_HTTP_F_CONN_CLOSE)) {
TFW_CONN_TYPE(req->conn) |= Conn_Stop;
if (unlikely(skb)) {
__kfree_skb(skb);
skb = NULL;
}
}

if (!req_conn_close && (data_off < skb_len)) {
/*
* Pipelined requests: create a new sibling message.
* @skb is replaced with pointer to a new SKB.
*/
/*
* Pipelined requests: create a new sibling message.
* If pipelined message can't be created, it still possible to
* process current one. But @skb must be freed then, since it's
* not owned by any message.
*/
if (!req_conn_close && skb) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If req_conn_close and skb != NULL (i.e. there are pipelined messages), then the skb leaks - nobody frees it.

hmsib = tfw_http_msg_create_sibling((TfwHttpMsg *)req,
&skb, data_off);
skb);
if (unlikely(!hmsib)) {
/*
* Unfortunately, there's no recourse. The
* caller expects that data is processed in
* full, and can't deal with partially
* processed data.
*/
TFW_INC_STAT_BH(clnt.msgs_otherr);
tfw_client_drop(req, 500, "cannot create"
" sibling request");
return TFW_BLOCK;
req->flags |= TFW_HTTP_F_CONN_CLOSE;
TFW_CONN_TYPE(conn) |= Conn_Stop;
tfw_http_conn_error_log(conn,
"Can't create pipelined"
" request");
__kfree_skb(skb);
skb = NULL;
}
}

Expand Down Expand Up @@ -2913,7 +2921,7 @@ tfw_http_req_process(TfwConn *conn, const TfwFsmData *data)
* Note: This connection's @conn must not be dereferenced
* from this point on.
*/
if (req_conn_close)
if (unlikely(req_conn_close))
break;

if (hmsib) {
Expand All @@ -2922,7 +2930,6 @@ tfw_http_req_process(TfwConn *conn, const TfwFsmData *data)
* Data processing will continue with the new SKB.
*/
data_off = 0;
skb_len = skb->len;
conn->msg = (TfwMsg *)hmsib;
}
}
Expand Down Expand Up @@ -3150,24 +3157,24 @@ tfw_http_resp_process(TfwConn *conn, const TfwFsmData *data)
struct sk_buff *skb = data->skb;
unsigned int off = data->off;
unsigned int data_off = off;
unsigned int skb_len = skb->len;
TfwHttpReq *bad_req;
TfwHttpMsg *hmresp;
TfwFsmData data_up;
bool filtout = false;

BUG_ON(!conn->msg);
BUG_ON(data_off >= skb_len);
BUG_ON(data_off >= skb->len);

TFW_DBG2("received %u server data bytes on conn=%p msg=%p\n",
skb->len - off, conn, conn->msg);
/*
* Process pipelined requests in a loop
* until all data in the SKB is processed.
*/
while (data_off < skb_len) {
while (skb) {
TfwHttpMsg *hmsib = NULL;
TfwHttpParser *parser;
bool conn_stop = false;

hmresp = (TfwHttpMsg *)conn->msg;
parser = &hmresp->parser;
Expand All @@ -3189,7 +3196,7 @@ tfw_http_resp_process(TfwConn *conn, const TfwFsmData *data)

TFW_DBG2("Response parsed: len=%u parsed=%d msg_len=%lu"
" ver=%d res=%d\n",
skb_len - off, data_off - off, hmresp->msg.len,
skb->len - off, data_off - off, hmresp->msg.len,
hmresp->version, r);

/*
Expand Down Expand Up @@ -3247,6 +3254,26 @@ tfw_http_resp_process(TfwConn *conn, const TfwFsmData *data)
goto bad_msg;
}

/*
* The message is fully parsed, the rest of the data in the
* stream may represent another response or its part.
* If skb splitting has failed, the response cant be forwarded
* to client or request-response sequence on client side can be
* broken and client may receive sensitive data from other
* clients can be also sent there.
* @skb is replaced with pointer to a new SKB.
*/
if (data_off < skb->len) {
skb = ss_skb_split(skb, data_off);
if (unlikely(!skb)) {
TFW_INC_STAT_BH(serv.msgs_otherr);
goto bad_msg;
}
}
else {
skb = NULL;
}

/*
* Verify response in context of http health monitor,
* and mark server as disabled/enabled.
Expand All @@ -3272,25 +3299,22 @@ tfw_http_resp_process(TfwConn *conn, const TfwFsmData *data)
/*
* If @skb's data has not been processed in full, then
* we have pipelined responses. Create a sibling message.
* @skb is replaced with a pointer to a new SKB.
*/
if (data_off < skb_len) {
hmsib = tfw_http_msg_create_sibling(hmresp, &skb,
data_off);
if (skb) {
hmsib = tfw_http_msg_create_sibling(hmresp, skb);
/*
* In case of an error there's no recourse. The
* caller expects that data is processed in full,
* and can't deal with partially processed data.
*/
if (unlikely(!hmsib)) {
TFW_INC_STAT_BH(serv.msgs_otherr);
/*
* Unable to create a sibling message.
* Send the parsed response to the client
* and close the server connection.
*/
tfw_http_resp_cache(hmresp);
return TFW_BLOCK;
tfw_http_conn_error_log(conn,
"Can't create pipelined"
" response");
__kfree_skb(skb);
skb = NULL;
conn_stop = true;
}
}

Expand All @@ -3306,19 +3330,28 @@ tfw_http_resp_process(TfwConn *conn, const TfwFsmData *data)
/*
* Pass the response to cache for further processing.
* In the end, the response is sent on to the client.
* @hmsib is not attached to the connection yet.
*/
if (tfw_http_resp_cache(hmresp))
if (tfw_http_resp_cache(hmresp)) {
tfw_http_conn_msg_free(hmsib);
return TFW_BLOCK;
}
next_resp:
if (hmsib) {
/*
* Switch the connection to the sibling message.
* Data processing will continue with the new SKB.
*/
data_off = 0;
skb_len = skb->len;
conn->msg = (TfwMsg *)hmsib;
}
else if (unlikely(conn_stop)) {
/*
* Creation of sibling response has failed, close
* the connection to recover.
*/
return TFW_BLOCK;
}
}

return r;
Expand Down