fix(1346): fix control frames flood #2108

kingluo · 2024-04-24T10:31:48Z

part of #1346

EvgeniiMekhanik · 2024-05-03T15:44:35Z

fw/http_frame.c

@@ -280,6 +280,17 @@ __tfw_h2_send_frame(TfwH2Ctx *ctx, TfwFrameHdr *hdr, TfwStr *data,
 	unsigned char buf[FRAME_HEADER_SIZE];
 	TfwStr *hdr_str = TFW_STR_CHUNK(data, 0);
 	TfwH2Conn *conn = container_of(ctx, TfwH2Conn, h2);
+	bool is_control_frame = !hdr->stream_id || hdr->type == HTTP2_RST_STREAM;
+
+	// If the peer is causing us to generate a lot of control frames,


We use another type of comments like:
/*
*
*/

EvgeniiMekhanik · 2024-05-03T15:46:15Z

I think that it is better to implement new counter in frang and check max count of control frames as we do for all other limits in frang.

kingluo · 2024-05-06T08:41:37Z

I think that it is better to implement new counter in frang and check max count of control frames as we do for all other limits in frang.

Maybe global configuration is better than frang? Just like "http_max_header_list_size", it should apply to all HTTP/2 connections regardless of virtual host, and it is not a request response related parameter.

const-t · 2024-05-09T13:29:55Z

I think that it is better to implement new counter in frang and check max count of control frames as we do for all other limits in frang.

I don't see much sense to move it to frang. 1. There is no appropriate frang handler. 2. We don't need history. 3. We have http_max_header_list_size that is not belong to frang as well.

const-t

LGTM

const-t · 2024-05-09T13:17:51Z

fw/http_frame.c

+	 */
+	if (is_control_frame &&
+	    atomic_read(&ctx->queued_control_frames)
+	                > max_queued_control_frames) {


Please, move brace to next line.

const-t · 2024-05-09T13:19:19Z

fw/http_frame.c

+	if (is_control_frame &&
+	    atomic_read(&ctx->queued_control_frames)
+	                > max_queued_control_frames) {
+		T_ERR("Too many control frames in send queue, closing connection");


Usually we use T_WARN in such cases. frang as example.

EvgeniiMekhanik · 2024-05-09T14:02:29Z

fw/http_frame.c

+	 * run out of memory.
+	 */
+	if (is_control_frame &&
+	    atomic_read(&ctx->queued_control_frames)


I think we don't need atomic here. Both functions where we read/inc/dec queued_control_frames are called under the socket lock.

const-t · 2024-05-10T11:02:04Z

I think that it is better to implement new counter in frang and check max count of control frames as we do for all other limits in frang.

I don't see much sense to move it to frang. 1. There is no appropriate frang handler. 2. We don't need history. 3. We have http_max_header_list_size that is not belong to frang as well.

However also there is one disadvantage of using it outside the frang, we can't configure ip_block for ping flooding. And this could be the deciding factor for moving it to frang.

krizhanovsky

I can't approve the PR since I don't understand what do we fix and why in this way.

Please, in this PR further development and other pull requests, write good commit messages what the commit does and why. In this case exact enumeration of CVEs and description of the approach to close the vulnerabilities is required. In #1346 for now we just have a bunch of CVE references, so it's not helpful in the review.

krizhanovsky · 2024-06-24T21:16:52Z

fw/http_frame.h

+ * SETTINGS, PING and RST_STREAM that will be queued for writing before
+ * the connection is closed to prevent memory exhaustion attacks.
+ */
+#define MAX_QUEUED_CONTROL_FRAMES	10000


The constant isn't used anywhere, and only a string default value "10000" is used in http.c configuration.

krizhanovsky · 2024-06-24T21:17:23Z

fw/sock_clnt.c

@@ -460,6 +460,9 @@ tfw_sk_write_xmit(struct sock *sk, struct sk_buff *skb, unsigned int mss_now,
 	if (h2_mode) {
 		h2 = tfw_h2_context(conn);
 		tbl = &h2->hpack.enc_tbl;
+		if (flags & SS_F_HTTT2_FRAME_CONTROL) {


Suggested change

if (flags & SS_F_HTTT2_FRAME_CONTROL) {

if (unlikely(flags & SS_F_HTTT2_FRAME_CONTROL)) {

krizhanovsky · 2024-06-24T21:32:05Z

fw/http_frame.h

@@ -209,6 +223,7 @@ typedef struct {
 */
 typedef struct tfw_h2_ctx_t {
 	spinlock_t	lock;
+	unsigned int	queued_control_frames;


Probably there is a better place for the 4 byte field in the data structure to avoid alignment holes

krizhanovsky · 2024-06-24T21:43:35Z

fw/http_frame.c

+		T_WARN("Too many control frames in send queue, closing connection\n");
+		r = SS_BLOCK_WITH_RST;
+		goto err;
+	}


Why do we limit egress control frames, not ingress from clients?

It is said in the comment above that control frames are PING, SETTINGS or RESET_STREAM, so what's the point to limit them on _egress_path? In general, (D)DoS attack implies some asymmetric conditions, when an attacker spends less resources than a victim server. It seems all the control frames sent from our side imply a some frame from a client. RESET even terminates a stream. E.g. we already have request_rate, which blocks HTTP/2 Rapid Reset - doesn't it solve the problem. Do we have a test using the rate limit and exhibiting a (D)DoS attack? @RomanBelozerov FYI

P.S. even with this comment tempesta-tech/tempesta-test#612 (comment) it seems request_rate will still do its job to block/mitigate the attack.

P.P.S. Is this a HTTP/2 slow read prevention? If so, then why don't we account the number and size of header and data frames?

According to the h2 spec, we should ack some control frames, e.g. PING, SETTINGS, RESET_STREAM, and there is no limit for the number of incoming frames in the spec, while it's necessary to limit them according to the responsive capability of the server, that is, when the server is under pressure or due to malicious client (zero TCP window), we should not ack the client in case of OOM or DDOS. If we limit the number of ingress paths instead, we cannot provision a reasonable threshold because the runtime changes always.

request_rate is only used to limit the HTTP/1 or HTTP/2 requests, not control frames. So unfortunately it's useless for this purpose.

for generic slow read prevention, there are other two PRs for it: test OOM by header leak tempesta-test#616, test OOM by slow read attack tempesta-test#618. All slow-read-related issues are located in http2: tests for CVE-2019-9512/9517 tempesta-test#612.

In this case exact enumeration of CVEs and description of the approach to close the vulnerabilities is required. In #1346 for now we just have a bunch of CVE references, so it's not helpful in the review.

Yes. But 1346 refers to 612.

And Roman lists the exact CVEs we need to fix in 1346, and this PR, as part of PRs of 1346, it solves:

GHSA-hgr8-6h9x-f7q9 “Ping Flood”
GHSA-9259-5376-vjcj “Settings Flood”

@krizhanovsky The inspiration of this fix comes from Nginx and golang's built-in http2 server.

Yes. But 1346 refers to 612.

And Roman #1346 (comment) the exact CVEs we need to fix in 1346, and this PR, as part of PRs of 1346, it solves:

GHSA-hgr8-6h9x-f7q9 “Ping Flood”
GHSA-9259-5376-vjcj “Settings Flood”

I know. The idea is that we should have a good commit message history, so anyone should be able to quickly understand the changes history only from git, without digging on github: this is faster and sometimes it's not trivial to find required task and pull request on github and reducing github (as 3rd-party product and company) dependency is also good.

Why can't we solve the problem with

rate limit for ingress h2 frames

limit the number of pending bytes (aka TCP send buffer)

Both the limits are easy to understand by a user and they follow the drop early strategy. With the current #2108 we accept control frames, process them and only after that terminate the connection.If we configure that a client connection can have not more than N bytes (we actually can just get the value of the current sysctl send buffer minus data in flight - we know everything about TCP state) for transmission to a client, we already have N pending bytes to the client and receive a control frame, which must be replied, we know upfront that we can't send the reply and can just reset send TCP zero window.This is part of #498 and seems not so hard to do. Definitely more work than #2108, but I'd prefer to make a good iteration on the important task (scheduled for the next milestone) now rather than pull tasks from 1.0

I'm not sure, but looks like we will broke #1973 with send buffer. The main goal of 1973 is to have send buffer with max size = CONG_WND

If we have non-zero congestion window, then we don't have the problem with slow read. We should be on the maximum transfer size as large as the congestion window, but do not keep more than send buffer size of pending data (waiting to be pushed down to the stack by TCP). I.e. 'send buffer' limit the the size of data queued for transmission to the TCP client connectio. This is plus to the data, which is transmitted in #1973

With #1973 we push for TCP transmission up to congestion window size bytes (h2 encode, ecrypt and send to the IP layer). But we also have some (maybe a lot) data for transmission (in the TCP send queue).

With #2108 we aiming to catch slow http2 attack, but each control frame sent by us is actually triggered by some frame received from a client. We also aim to drop malicious data early, so we should drop the malicious client frames early (this is where Nginx and Go http do bad job). Another problem with 2108, also inherited from Nginx, is why 10,000? The slow read attack targets memory exhaust, not "frames number" exhaust, so we should account memory, not frames. Is 10K too much for the default 100 streams? Is it to small if a user defines 1000 streams?

My proposal is to send zero receive TCP window to a client if our send buffer is full for the client, i.e. do no accept new data from a client and notify them that we can't accept data if we can't reply to the data.

receive a control frame (do not send TCP ACK!)

determine if we have a send buffer space for a reply for the frame

if yes - send ACK, if not - send zero tcp window and drop the tcp data

This is partial version of #498. We do not implement backpresure at least. Probably a lot of more logic isn't in this scope.

@kingluo After discussion with @krizhanovsky we decided to implement rate limit for PING, SETTINGS, PRIORITY frames. This limit must be simple as possible, just reset connection when receive frames too fast.

@kingluo @const-t @EvgeniiMekhanik we also agreed that the task should be the part of #498, i.e. also limit the memory consumption for slow read attacks. I.e. we should have the rate limiting as @const-t mentioned above and memory consumption by each client, just like Nginx provides 2-layer protection

EvgeniiMekhanik · 2024-06-25T16:12:12Z

fw/sock_clnt.c

@@ -460,6 +460,9 @@ tfw_sk_write_xmit(struct sock *sk, struct sk_buff *skb, unsigned int mss_now,
 	if (h2_mode) {
 		h2 = tfw_h2_context(conn);
 		tbl = &h2->hpack.enc_tbl;
+		if (flags & SS_F_HTTT2_FRAME_CONTROL) {
+			--h2->queued_control_frames;


I found that we can't counted control frames here. Several skbs can be aggregated in one tls record in this case we don't decrement count of control frames.

part of #1346

kingluo mentioned this pull request Apr 24, 2024

test OOM by control frames attack tempesta-tech/tempesta-test#615

Open

kingluo marked this pull request as ready for review April 24, 2024 13:23

kingluo requested review from const-t and EvgeniiMekhanik April 24, 2024 13:39

EvgeniiMekhanik reviewed May 3, 2024

View reviewed changes

kingluo force-pushed the jinhua/fix-1346-flood branch from 1effa31 to 9153825 Compare May 6, 2024 16:05

const-t approved these changes May 9, 2024

View reviewed changes

EvgeniiMekhanik reviewed May 9, 2024

View reviewed changes

RomanBelozerov mentioned this pull request May 28, 2024

sometimes max_queued_control_frames does not send a warning to dmesg #2126

Closed

EvgeniiMekhanik approved these changes Jun 5, 2024

View reviewed changes

This was referenced Jun 13, 2024

Mekhanik evgenii/1196 tempesta-tech/linux-5.10.35-tfw#17

Merged

Tls errors under ping flood #2117

Closed

krizhanovsky mentioned this pull request Jun 25, 2024

HTTP message buffering and streaming #498

Open

6 tasks

krizhanovsky requested changes Jun 25, 2024

View reviewed changes

EvgeniiMekhanik reviewed Jun 25, 2024

View reviewed changes

EvgeniiMekhanik self-requested a review June 25, 2024 16:12

kingluo added 6 commits July 10, 2024 19:30

fix(1346): fix control frames flood

7010cf7

part of #1346

move MAX_QUEUED_CONTROL_FRAMES to configuration file

a106b7e

fix PR

685b84b

fix T_WARN

c3f311c

rework based on tfw_tls_encrypt and skb->cb

1195e1e

rebase fix

902025a

kingluo force-pushed the jinhua/fix-1346-flood branch from c3d466b to 902025a Compare July 10, 2024 11:48

store flags in tcp_skb_cb->unused

6f5b30d

EvgeniiMekhanik requested a review from const-t July 11, 2024 10:18

kingluo linked an issue Aug 12, 2024 that may be closed by this pull request

HTTTP/2 (D)DoS prevention #1346

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(1346): fix control frames flood #2108

fix(1346): fix control frames flood #2108

kingluo commented Apr 24, 2024

EvgeniiMekhanik May 3, 2024

EvgeniiMekhanik commented May 3, 2024

kingluo commented May 6, 2024

const-t commented May 9, 2024

const-t left a comment

const-t May 9, 2024

const-t May 9, 2024

EvgeniiMekhanik May 9, 2024

const-t commented May 10, 2024

krizhanovsky left a comment

krizhanovsky Jun 24, 2024

krizhanovsky Jun 24, 2024

krizhanovsky Jun 24, 2024

krizhanovsky Jun 24, 2024

kingluo Jun 25, 2024

kingluo Jun 25, 2024 •

edited

Loading

kingluo Jun 25, 2024

krizhanovsky Jun 25, 2024

krizhanovsky Jun 25, 2024 •

edited

Loading

const-t Jul 15, 2024

krizhanovsky Jul 16, 2024

EvgeniiMekhanik Jun 25, 2024

	if (flags & SS_F_HTTT2_FRAME_CONTROL) {
	if (unlikely(flags & SS_F_HTTT2_FRAME_CONTROL)) {

fix(1346): fix control frames flood #2108

Are you sure you want to change the base?

fix(1346): fix control frames flood #2108

Conversation

kingluo commented Apr 24, 2024

Choose a reason for hiding this comment

EvgeniiMekhanik commented May 3, 2024

kingluo commented May 6, 2024

const-t commented May 9, 2024

const-t 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

const-t commented May 10, 2024

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

kingluo Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky Jun 25, 2024 • edited Loading

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

kingluo Jun 25, 2024 •

edited

Loading

krizhanovsky Jun 25, 2024 •

edited

Loading