Do not drop skb to have ability to processs WINDOW_UPDATE #2274

EvgeniiMekhanik · 2024-10-31T17:18:55Z

Should be merged only after #2272

ai-tmpst

LGTM

const-t · 2024-11-08T16:17:40Z

fw/tls.c

 	case T_POSTPONE:
 		/* No complete TLS record seen yet. */
 		spin_unlock(&tls->lock);
 		/*
 		 * save_error_code is T_OK or T_BAD if error occurs
 		 * on one of the previous steps.
 		 */
-		return save_err_code;
+		return T_OK;


Please fix the comment above.

const-t · 2024-11-08T16:19:42Z

lib/log.h

@@ -64,6 +64,22 @@ enum {
 	T_OK		 = 0,
 };

+static inline int
+tfw_handle_error(int r, int *save_err_code, bool was_stopped)


lib/log.g is definitely not appropriate place for tfw_handle_error() I would propose to move it to connection.h or another place.

Yes implement special file to this case

const-t · 2024-11-08T16:39:02Z

fw/connection.c

+			} else {
+				r = tfw_http_msg_process(conn, skb, &split,
+							 save_err_code);
+			}


Braces are not necessary here

const-t · 2024-11-08T17:06:07Z

fw/sock.c

-			goto out; /* connection must be dropped */
+		r = SS_CALL(connection_recv, conn, skb, &tmp_save_err_code);
+		WARN_ON(r && tmp_save_err_code);
+		r = tfw_handle_error(r, &save_err_code, was_stopped);


We do tfw_handle_error() on tls layer and http layer why do we need it here? In my understanding, tls and http layer are responsible for error handling and here we should have only result from upper layers.

In this function we drop unprocessed skb at the end of the function. We should not do it case of T_BAD. So we call this function here also.

const-t · 2024-11-08T17:22:15Z

fw/sock.c

@@ -820,6 +820,9 @@ ss_tcp_process_skb(struct sock *sk, struct sk_buff *skb, int *processed)
 	void *conn;
 	struct sk_buff *skb_head = NULL;
 	struct tcp_sock *tp = tcp_sk(sk);
+	int save_err_code = SS_OK, tmp_save_err_code;


I must notice that save_err_code has life time the same ss_tcp_process_skb() it means, client can send multiple bad packets with some interval, but save_err_code will be SS_OK.

In this case we use Conn_Stop bit. If error occurs on previous call of this function Conn_Stop bit was set.

const-t · 2024-11-08T17:30:52Z

fw/tls.c

@@ -187,7 +185,8 @@ tfw_tls_connection_recv(TfwConn *conn, struct sk_buff *skb)
 			tfw_tls_purge_io_ctx(&tls->io_in);
 			kfree_skb(nskb);
 			spin_unlock(&tls->lock);
-			return T_BAD;
+			return tfw_handle_error(T_BAD, save_err_code,


Why we calling tfw_handle_error() instead of return SS_BLOCK_WITH_RST? It looks confusing, we must drop connection in case of such errors.

This function returns T_BAD if it first error (in this case we send tcp_shutdown as it was before) or BLOCK_WITH_RST if it is a seconf error

EvgeniiMekhanik · 2024-11-15T00:00:32Z

@krizhanovsky I have a question. If something wen wrong and we close connection with TCP RST, we call ss_close -> ss_do_close. But ss_do_close we be called later in ss_tx_action. So if some new skbs from client come before ss_do_close will be called there will be some TLS warnings in dmesg (because we can't decrypt them). Is it ok or we should implement special flag to drop such skbs?

keshonok · 2024-11-18T11:58:20Z

I've got just a cursory glance, so I might be mistaken. The logic looks a bit complex with these fine-tune flags and details. Does this work for HTTP2 only? I far as I understand, thus should not happen for HTTP. It's not obvious, not from the commit message, and not from comments in the code. Perhaps, this should be made obvious both in the code comments in appropriate places, and in the commit message. Thanks!

There is a lag between calling ss_close\ss_shutdown and real socket closing, because we close socket later from `ss_tx_action`. Set Conn_Stop bit to prevent any further processing if connection is closinga and this bit was not set by upper layer.

In case when error occurs we should continue to process WINDOW_UPDATE frames. If we drop skb in case of error we can't to continue decrypt new tls records and can't continue to process WINDOW_UPDATE frames. Also there are a lot of annoying TLS warnings in dmesg. This patch fixes this problems. For HTTP1 connections we stop processing new incomming messages immediately.

krizhanovsky

LGTM.

Regarding TLS warnings it's not OK since it will bother users. However,

this seems not a crucial event, so maybe we just should replace the warnings with T_DBG
I'm worried that we put a lot of effort and add significant complexity to the code just to accept HTTP/2 WINDOW_UPDATE frames while we're sending an error message to an attacker or an unhappy client. Should we really do this? This is unlucky situation anyway. Also probably in most cases we'll be fine with the current widow and won't need to adjust it during the error response transmission. Also this could impact security since already blocked client still able to send us some messages and we do spend resources on them. We do need to process WINDOW_UPDATE on normal response transmission with closing connection, but it's debatable for error responses.

Probably we should discuss this on the next call.

krizhanovsky · 2024-11-19T16:30:08Z

fw/http.c

-		 * but still continue to process rest of SKBs and parse data,
-		 * that is dangerous.
-		 */
-		TFW_CONN_TYPE(conn) |= Conn_Stop;


Why do we remove the code? IIUC ss_tcp_data_ready() now sets the flag, but only after tfw_http_resp_process() call. But we benefit from earlier setting of Conn_Stop flag, i.e. in tfw_http_resp_process(). It seems we should leave setting the flag in ss_tcp_data_ready(), but still leave this code as well.

krizhanovsky · 2024-11-19T18:07:53Z

lib/error_code.h

+		*save_err_code = T_OK;
+	}
+	return r;
+}


This function specific for fw/ code and has nothing with tls or db, so probably shouldn't be in lib

EvgeniiMekhanik requested review from const-t and ai-tmpst October 31, 2024 17:18

EvgeniiMekhanik marked this pull request as draft October 31, 2024 17:19

EvgeniiMekhanik force-pushed the MekhanikEvgenii/fix-2267-next branch from b60311c to 269783c Compare October 31, 2024 18:46

EvgeniiMekhanik marked this pull request as ready for review October 31, 2024 18:47

EvgeniiMekhanik changed the title ~~TMP~~ Do not drop skb to have ability to processs WINDOW_UPDATE Oct 31, 2024

ai-tmpst approved these changes Nov 2, 2024

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii/fix-2267-next branch from 269783c to 6c9235d Compare November 4, 2024 09:32

const-t requested changes Nov 8, 2024

View reviewed changes

EvgeniiMekhanik requested a review from krizhanovsky November 14, 2024 15:54

EvgeniiMekhanik marked this pull request as draft November 15, 2024 08:35

EvgeniiMekhanik force-pushed the MekhanikEvgenii/fix-2267-next branch from 6c9235d to 06644cf Compare November 18, 2024 11:39

EvgeniiMekhanik force-pushed the MekhanikEvgenii/fix-2267-next branch 3 times, most recently from 6fe12a2 to d8ddf2d Compare November 18, 2024 15:47

EvgeniiMekhanik added 3 commits November 18, 2024 17:49

Move error codes to separate header

a80429a

EvgeniiMekhanik force-pushed the MekhanikEvgenii/fix-2267-next branch from d8ddf2d to 8863547 Compare November 18, 2024 15:56

EvgeniiMekhanik marked this pull request as ready for review November 18, 2024 15:57

EvgeniiMekhanik requested a review from const-t November 19, 2024 11:02

krizhanovsky approved these changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not drop skb to have ability to processs WINDOW_UPDATE #2274

Do not drop skb to have ability to processs WINDOW_UPDATE #2274

EvgeniiMekhanik commented Oct 31, 2024

ai-tmpst left a comment

const-t Nov 8, 2024

EvgeniiMekhanik Nov 18, 2024

const-t Nov 8, 2024

EvgeniiMekhanik Nov 18, 2024

const-t Nov 8, 2024

EvgeniiMekhanik Nov 18, 2024

const-t Nov 8, 2024

EvgeniiMekhanik Nov 18, 2024

const-t Nov 8, 2024

EvgeniiMekhanik Nov 15, 2024 •

edited

Loading

const-t Nov 8, 2024

EvgeniiMekhanik Nov 18, 2024

EvgeniiMekhanik commented Nov 15, 2024 •

edited

Loading

keshonok commented Nov 18, 2024

krizhanovsky left a comment

krizhanovsky Nov 19, 2024

krizhanovsky Nov 19, 2024

Do not drop skb to have ability to processs WINDOW_UPDATE #2274

Are you sure you want to change the base?

Do not drop skb to have ability to processs WINDOW_UPDATE #2274

Conversation

EvgeniiMekhanik commented Oct 31, 2024

ai-tmpst 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

EvgeniiMekhanik Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvgeniiMekhanik commented Nov 15, 2024 • edited Loading

keshonok commented Nov 18, 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

EvgeniiMekhanik Nov 15, 2024 •

edited

Loading

EvgeniiMekhanik commented Nov 15, 2024 •

edited

Loading