Frame forwarded h2 message to never overflow max frame size on receive side #1400

vankoven · 2020-04-03T11:33:10Z

#1378

krizhanovsky

The PR looks incomplete and there are several issues.

Also there are some traces #1378 (comment) which must be analyzed and fixed or reported as a separate issue.

tempesta_fw/http_limits.c

tempesta_fw/cache.c

tempesta_fw/http.c

tempesta_fw/cache.c

tempesta_fw/http.c

…e side

…ects, all error responses

tempesta_fw/http.c

tempesta_fw/http_msg.c

tempesta_fw/cache.c

tempesta_fw/http.c

Never happens since we don't copy anything in the last page with body, but make the code safe it will happen once. Also fix warning on incomplete record case

The combined buffer was sent to http/2 client as a single body fragment and client receives extra garbage at the beginning of the body not allowing to parse it. This was an optimization for http/1, since the header is always written when body is sent. In http/2 the header is not required and native http/2 framing is used. Since http/1 is now compatibility mode, drop the optimization.

Two issues are fixed here. First, frang is called as early as possible now and sticky cookie module is not called for untrusted requests. So attacker cant stress cookie module from now on. But I can tell that about http tables, they are still vulnerable. Second, a null-dereference happened in frang if a request was already replied by sticky module.

krizhanovsky

LGTM, only minor changes are required.

krizhanovsky · 2020-05-26T20:26:28Z

tempesta_fw/cache.c

+		 * no data in the next record part.
+		 */
+		if (WARN_ON_ONCE(!trec->len))
+			return -EINVAL;
 		p = trec->data;

 		if (it->frag == MAX_SKB_FRAGS
 		    && (r = tfw_msg_iter_append_skb(it)))


It seems we should here also set SKBTX_SHARED_FRAG flag as we do this above if sh_frag

No, we don't need to. tfw_msg_iter_append_skb() not only add a new empty skb to the list, but also sets the same tx_flags last skb had.

krizhanovsky · 2020-05-27T16:41:52Z

tempesta_fw/http.c

+
+/**
+ * Split response body stored locally. Allocate a new skb and put body there
+ * by fragments. Every skb fragment is has size of single page and has frame


Suggested change

* by fragments. Every skb fragment is has size of single page and has frame

* by fragments. Every skb fragment has size of single page and has frame

Thanks! Fixed.

krizhanovsky · 2020-05-27T17:26:26Z

tempesta_fw/http.c

+ * processing thus no chunked body allowed, only plain TfwStr is accepted there.
+ */
+static int
+tfw_h2_append_predefined_body(TfwHttpResp *resp, unsigned int stream_id,


[Offtopic] That's pity to generate static message body, but it seems we don't have much opportunities in the wonderful HTTP/2 world, so I close #163 (comment)

krizhanovsky · 2020-05-27T17:44:26Z

tempesta_fw/http.c

+	 * Run frang checks first before any processing happen. Can't start
+	 * the checks earlier, since vhost and specific client is required
+	 * for frang checks.
+	 * TDB: if a request was received in a single skb, the only frang check


Suggested change

* TDB: if a request was received in a single skb, the only frang check

* TBD: if a request was received in a single skb, the only frang check

Thanks! Fixed.

krizhanovsky · 2020-05-27T17:46:47Z

tempesta_fw/http.c

+	 * for frang checks.
+	 * TDB: if a request was received in a single skb, the only frang check
+	 * happens here. Looks like http tables are not protected with
+	 * anti-DDoS limits and attacker may stress http tables as long as he


Suggested change

* anti-DDoS limits and attacker may stress http tables as long as he

* anti-DDoS limits and attacker may stress http tables as long as they

Here and at the below: ladies also can launch attacks as well ;) "They" is usually used in modern English even in singular form

I thought about 'attacker' as a gender-neutral term. Ok, fixed it.

krizhanovsky · 2020-05-27T17:49:39Z

tempesta_fw/http.c

+	 * to close the connection instead on sending 403 errors, but such
+	 * behaviour is not browser friendly and even may increase request
+	 * rate from browsers during "referer" attacks, since browser usually
+	 * retry failed and unreplied requests.


I believe we're good with the change and HTTP tables in general. There are HTTP rate limits now before them and also there are nftables rate limit and we're planning to use XDP (#1048) to block volumetric attacks early.

Yeah, you right here I was to furious and ddin't notice the fact we close connection every time we face block action in http tables. Attackers then need to reopen connection every time to stress us, but now we filter them by new connection rate limit, which is normally much more strict than request rate. So we're good with the scenario (Also checked it on practice). Comment was updated.

krizhanovsky · 2020-05-27T18:13:54Z

tempesta_fw/http.c

 	size_t b_off;

-	return __tfw_http_msg_body_dup(filename, &c_len_hdr, len, &b_off);
+	return __tfw_http_msg_body_dup(filename, NULL, len, &b_off);


__tfw_http_msg_body_dup() is called only in http.c, so make it static

vankoven added the ready for review label Apr 9, 2020

krizhanovsky self-requested a review April 16, 2020 12:51

krizhanovsky self-assigned this Apr 16, 2020

krizhanovsky mentioned this pull request Apr 16, 2020

Fragmentation for h2 messages #1378

Closed

4 tasks

krizhanovsky requested changes Apr 17, 2020

View reviewed changes

vankoven removed the ready for review label Apr 28, 2020

vankoven added 5 commits May 1, 2020 11:11

Frame forwarded h2 message to never overflow max frame size on receiv…

34223bb

…e side

Frame h2 messages served from cache

9b16789

Frame h2 responses fully created by Tempesta: 304 response, 302 redir…

11c2986

…ects, all error responses

frang: suppress crash when sticky module is used for h2 protocol

29a7343

remove outdated #1378 todos: all the required changes are already here

2396e30

vankoven force-pushed the ik-frame-h2-messages branch from 1c041ea to 2396e30 Compare May 1, 2020 13:58

krizhanovsky assigned aleksostapenko May 5, 2020

vankoven force-pushed the ik-frame-h2-messages branch from 3bcfbb9 to 4b17404 Compare May 9, 2020 13:23

vankoven added the ready for review label May 9, 2020

aleksostapenko suggested changes May 10, 2020

View reviewed changes

tempesta_fw/http.c Outdated Show resolved Hide resolved

tempesta_fw/http_msg.c Outdated Show resolved Hide resolved

tempesta_fw/cache.c Outdated Show resolved Hide resolved

tempesta_fw/cache.c Outdated Show resolved Hide resolved

tempesta_fw/cache.c Outdated Show resolved Hide resolved

vankoven added 2 commits May 12, 2020 15:44

Cache: avoid extra skb fragmentation for h2 messages

c2680c1

Bump copyrights

9e9d02f

vankoven force-pushed the ik-frame-h2-messages branch from f008f09 to 9e9d02f Compare May 12, 2020 10:44

krizhanovsky reviewed May 13, 2020

View reviewed changes

tempesta_fw/cache.c Show resolved Hide resolved

krizhanovsky reviewed May 13, 2020

View reviewed changes

tempesta_fw/cache.c Outdated Show resolved Hide resolved

aleksostapenko reviewed May 21, 2020

View reviewed changes

tempesta_fw/http.c Outdated Show resolved Hide resolved

vankoven added 8 commits May 22, 2020 14:29

Update comments and documentation to mirror last changes.

1a68a18

cache: don't try to add data behind body end

6401862

Never happens since we don't copy anything in the last page with body, but make the code safe it will happen once. Also fix warning on incomplete record case

Remove double assugnment

cf9d328

h2: Fix framing locally generated responses

97db233

Add response_body notice.

c1f1dfe

Don't unconditionally set SKBTX_SHARED_FRAG

c979137

krizhanovsky approved these changes May 27, 2020

View reviewed changes

Address code review comments:

c24cda4

vankoven force-pushed the ik-frame-h2-messages branch from d31d7b3 to c24cda4 Compare May 28, 2020 11:54

Http tables are not vulnerable for rate attacks, fix comment

f167c8a

vankoven merged commit de15830 into master May 28, 2020

vankoven unassigned aleksostapenko May 28, 2020

vankoven deleted the ik-frame-h2-messages branch May 28, 2020 12:37

vankoven mentioned this pull request Oct 17, 2020

Fix framing on long msg #1457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frame forwarded h2 message to never overflow max frame size on receive side #1400

Frame forwarded h2 message to never overflow max frame size on receive side #1400

vankoven commented Apr 3, 2020

krizhanovsky left a comment •

edited

Loading

krizhanovsky left a comment

krizhanovsky May 26, 2020

vankoven May 28, 2020

krizhanovsky May 27, 2020

vankoven May 28, 2020

krizhanovsky May 27, 2020

krizhanovsky May 27, 2020

vankoven May 28, 2020

krizhanovsky May 27, 2020

vankoven May 28, 2020

krizhanovsky May 27, 2020

vankoven May 28, 2020

krizhanovsky May 27, 2020

vankoven May 28, 2020

	* by fragments. Every skb fragment is has size of single page and has frame
	* by fragments. Every skb fragment has size of single page and has frame

	* TDB: if a request was received in a single skb, the only frang check
	* TBD: if a request was received in a single skb, the only frang check

	* anti-DDoS limits and attacker may stress http tables as long as he
	* anti-DDoS limits and attacker may stress http tables as long as they

Frame forwarded h2 message to never overflow max frame size on receive side #1400

Frame forwarded h2 message to never overflow max frame size on receive side #1400

Conversation

vankoven commented Apr 3, 2020

krizhanovsky left a comment • edited Loading

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky left a comment •

edited

Loading