Fix farious frang errors #1200

vankoven · 2019-03-01T07:16:18Z

fix #1028

Fixed:

test trailer headers across frang limits after body is processed: maximum header len, don't allow same header in header and trailer parts. Last check requires to know, whether is message is fully parsed.
close|block client connection immediately once response breaking code block rules is encountered
fix possible null pointer dereferencing while testing response code block rules for Health Monitor originated requests.
add missing frang limits names to tempesta_fw.conf
~~TfwHttpReq->frang_st duplicates existing Gfsm state in frang FSM and it was removed. Frang states are hookable now, so advanced analytics can be attached to it.~~

UPD

Major updates in handling frang directives.

Frang directives now are really per-vhost and per-location. Each vhost and location may include frang_limits section now. Previous behaviour: frang limits may be defined only globally and only inside locations (and not inside vhosts!), but limits was listed in locations without grouping into section frang_limits
Nested locations and vhosts inherit Frang configuration from parents. frang_limits section now can appear many times on the same level to force new defaults before new bunch of vhosts|locations. Previous behaviour: no inheritance, every location must define it's own frang settings, so less copy-paste now.
Frang limits was split into two groups: global and per-vhost. Since messages are received by parts, it's not possible to deduce which per-vhost limits should be applied before the whole message is received. So some limits has no sense in beeing per-vhost, they're global now.

krizhanovsky

I made a very quick high-level review, a more accurate reviews are required.

tempesta_fw/http_limits.c

krizhanovsky

It seems not all #1028 requirements are satisfied and some logic should be reworked.

tempesta_fw/http_limits.c

tempesta_fw/gfsm.c

tempesta_fw/http_limits.c

krizhanovsky

Basically this version is good, but there are several architectural questions and notes, so I'd like to discuss the notes and review the next changes afterwards.

tempesta_fw/http_limits.c

tempesta_fw/http_limits.h

tempesta_fw/http_limits.c

tempesta_fw/vhost.c

krizhanovsky

No changes since last review, please fix them ASAP.

krizhanovsky

Small improvements are still required. The most crucial change is to call Frang on moving from HTTP headers to HTTP body.

tempesta_fw/http_limits.c

krizhanovsky · 2019-07-10T13:45:52Z

tempesta_fw/http_limits.c

-		if (hdr_tmt) {
-			req->tm_header = jiffies;
+	if (!f_cfg->http_trailer_split)
+		return TFW_PASS;


Please add framg_msg() here to notify about the reason for a request blocking

krizhanovsky · 2019-07-10T14:06:41Z

tempesta_fw/http_limits.c

+				: req->vhost->frang_gconf;
+		tfw_cli_conn_close_all_sync((TfwClient *)req->conn->peer);
+		if (fg_cfg->ip_block)
+			tfw_filter_block_ip(&FRANG_ACC2CLI(ra)->addr);


This comment is till not addressed. If we block IP, but leave a client connection, then we can send data to a client, but we won't receive neither ACK not probbing response from it - the connection will spend our resources untill OS closes it.

I believe we should call tfw_filter_block_ip() only with connection closing, i.e. there should be a new API function or tfw_filter_block_ip() just can call connection closing. Next, we can not do normal close here, because on blocked IP, we can not receive FIN-ACK and the connection hangs. We need to use RST closing or just free the connection resources silently.

The proper closing is the subject for #861 and sycnhronous doesn't solve the issue.

krizhanovsky · 2019-07-10T14:34:52Z

tempesta_fw/http_limits.c

+				: req->vhost->frang_gconf;
+		tfw_cli_conn_close_all_sync((TfwClient *)req->conn->peer);
+		if (fg_cfg->ip_block)
+			tfw_filter_block_ip(&FRANG_ACC2CLI(ra)->addr);


I think it makes sense just to remove or comment (out?) the synchronous closing with TODO referering #860.

krizhanovsky · 2019-07-10T14:50:30Z

tempesta_fw/http_limits.c

-		if (r == TFW_PASS && resp_cblk)
-			r = frang_bad_resp_limit(ra, resp_cblk);
-		__FRANG_FSM_MOVE(Frang_Req_Hdr_Start);
+	r = frang_http_req_incomplete_body_check(ra, data, fg_cfg, f_cfg);


OK, make sense. Please write explicitly in the Wiki that we process trailers accordingly the body limits.

tempesta_fw/http_limits.h

krizhanovsky · 2019-07-10T15:00:44Z

tempesta_fw/http_limits.h

+
+	bool			http_ct_required;
+	bool			http_host_required;
+	bool			http_trailer_split;


Please comment the field above

krizhanovsky · 2019-07-10T15:03:33Z

tempesta_fw/vhost.c

+		.deflt = "false",
+		.handler = tfw_cfgop_frang_trailer_split,
+		.allow_reconfig = true,
+	},


Wiki and #673 must be updated for the new configuration option.

krizhanovsky

The Frang changes are move to #1302 since they depend on #309. So now I'm generally good with the PR and only small changes are required.

i-rinat

Patchset is large so I'll submit my comments so far. From the part I looked into, I found no major issues. Mostly spelling typos, and also some side notes. (Feel free to mark them as resolved at your discretion.)
I'll continue to read the patchset further.

i-rinat · 2019-07-11T13:08:48Z

etc/tempesta_fw.conf

@@ -921,10 +921,14 @@

 # TAG: frang_limits
 #
-# The section containing static limits for the classifier.
+# The section containing static limits for the classifier. Can apper at top


Suggested change

# The section containing static limits for the classifier. Can apper at top

# The section containing static limits for the classifier. Can appear at the top

i-rinat · 2019-07-11T13:10:47Z

etc/tempesta_fw.conf

-# The section containing static limits for the classifier.
+# The section containing static limits for the classifier. Can apper at top
+# level, inside 'vhost' directive, inside 'location' directive. Once the
+# directive is listed in the configuration it redefine default vaues for


Suggested change

# directive is listed in the configuration it redefine default vaues for

# directive is listed in the configuration it redefines default values for the

i-rinat · 2019-07-11T13:44:35Z

tempesta_fw/http.c

+	 * - get vhost from the HTTP session information (by Sticky cookie);
+	 * - get Vhost according to TLS SNI header parsing.
+	 * But vhost returned from all that functions can be very different
+	 * depending on HTTP chain configuration due to it flexibility and


Suggested change

* depending on HTTP chain configuration due to it flexibility and

* depending on HTTP chain configuration due to its flexibility and

i-rinat · 2019-07-11T13:46:11Z

tempesta_fw/http.c

+	 * But vhost returned from all that functions can be very different
+	 * depending on HTTP chain configuration due to it flexibility and
+	 * client (attacker) behaviour.
+	 * The most secure and a slow way: compare all of them and reject if


Suggested change

* The most secure and a slow way: compare all of them and reject if

* The most secure and slow way: compare all of them and reject if

i-rinat · 2019-07-11T13:46:40Z

tempesta_fw/http.c

+	 * depending on HTTP chain configuration due to it flexibility and
+	 * client (attacker) behaviour.
+	 * The most secure and a slow way: compare all of them and reject if
+	 * at least some doesn't match. Can be too strict, service quality can


Suggested change

* at least some doesn't match. Can be too strict, service quality can

* at least some do not match. Can be too strict, service quality can

i-rinat · 2019-07-12T14:36:17Z

tempesta_fw/sock_clnt.c

@@ -298,6 +310,21 @@ tfw_cli_conn_close_all(void *data)
 	return tfw_peer_for_each_conn(cli, conn, list, __cli_conn_close_cb);
 }

+/**
+ * Close all connections with given client, called on security events. Unlike
+ * asynchronous function above, this one must guarantee that all the close


I'd propose to change "asynchronous function above" to "tfw_cli_conn_close_all". It's too easy to add another function between these two, and so to accidentally make the comment wrong.

i-rinat · 2019-07-12T14:37:36Z

tempesta_fw/sock_clnt.c

+ * Close all connections with given client, called on security events. Unlike
+ * asynchronous function above, this one must guarantee that all the close
+ * requests will be done. Attackers can spam Tempesta with lot of requests and
+ * connections, trying to cause work queue overrun and delay of security events


Suggested change

* connections, trying to cause work queue overrun and delay of security events

* connections, trying to cause a work queue overrun and delay security events

i-rinat · 2019-07-12T18:50:35Z

tempesta_fw/tls.c

+	}
+
+	if (!READ_ONCE(tfw_tls.fallback_to_dflt_sni) || !vhost->vhost_dflt) {
+		tfw_vhost_put(vhost);


Function name says it "gets", but here it puts the refcounter on vhost. Wouldn't be better to leave putting vhosts to the calling side?

i-rinat · 2019-07-12T18:55:28Z

tempesta_fw/tls.c

+/**
+ * Find matching vhost according to server name in SNI extension. The function
+ * is also called if there is no SNI extension and fallback to some default
+ * configuration is required. In the last case @data is NULL and @len is 0.


Suggested change

* configuration is required. In the last case @data is NULL and @len is 0.

* configuration is required. In the latter case @data is NULL and @len is 0.

i-rinat · 2019-07-12T18:58:49Z

tempesta_fw/tls.c

+		return -TTLS_ERR_BAD_HS_CLIENT_HELLO;
+
+	if (data && len)
+		 vhost = tfw_vhost_lookup(&srv_name);


Suggested change

vhost = tfw_vhost_lookup(&srv_name);

vhost = tfw_vhost_lookup(&srv_name);

i-rinat

Found no issues with the PR, looks good.

There are some more spelling comments below. And also a comment about locking required for configuration processing. If that turns out to be a real issue and not something I mistakenly saw as a bug, its fix is beyond the scope of this PR anyway.

i-rinat · 2019-07-12T22:19:43Z

tempesta_fw/tls.c

@@ -625,6 +630,74 @@ static TfwConnHooks tls_conn_hooks = {
 	.conn_send	= tfw_tls_conn_send,
 };

+static bool
+tfw_tls_get_if_configured(TlsCtx *ctx, TfwVhost *vhost)


The function only uses ctx to set ctx->peer_conf. This parameter can be easily removed along with making function to return cfg on success or NULL on failure. That way it will be obvious that function won't touch TlsCtx at all, and only needs TfwVhost to make a decision.

i-rinat · 2019-07-12T22:40:07Z

tempesta_fw/tls.c

-	ttls_pk_context	key;
-	unsigned long	crt_pg_addr;
-	unsigned int	crt_pg_order;
+	bool		allow_unknown_sni;


"Allow unknown server name indication" sounds a bit off. Perhaps, "allow_unknown_server_name"?

Also, what does this flag mean? As far as I can understand from the code, it's something like: "If client did not provide SNI extension, or if the name in that extension did not correspond to any known vhosts, use the default one". Is that correct?

i-rinat · 2019-07-12T22:40:52Z

tempesta_fw/tls.c

-	unsigned long	crt_pg_addr;
-	unsigned int	crt_pg_order;
+	bool		allow_unknown_sni;
+	bool		fallback_to_dflt_sni;


"sni" here also contains "indication" part, which doesn't make sense in a flag name.

Also, what does this flag mean? As far as I can tell, it's: "If selected vhost does not contain required TLS configuration, use configuration from the default vhost". Is that correct?

i-rinat · 2019-07-12T23:00:19Z

tempesta_fw/tls_conf.c

+static int
+tfw_tls_peer_tls_init(TfwVhost *vhost)
+{
+	TlsConfEntry *conf = (TlsConfEntry *)vhost->tls_cfg.priv;


Explicit cast is not necessary here.

i-rinat · 2019-07-15T12:20:07Z

tempesta_fw/tls_conf.c

+static inline TlsCertConf *
+tfw_tls_get_cert_conf(TfwVhost *vhost, unsigned int directive)
+{
+	TlsConfEntry *conf = (TlsConfEntry *)vhost->tls_cfg.priv;


Type casting is not required here.

i-rinat · 2019-07-15T18:57:38Z

tempesta_fw/vhost.c

-		return 0;
-
+	/*
+	 * Implicit default vhost is still useful even if it never used to


Suggested change

* Implicit default vhost is still useful even if it never used to

* Implicit default vhost is still useful even if it's never used to

i-rinat · 2019-07-15T18:59:18Z

tempesta_fw/vhost.c

+	if (tfw_frang_cfg_inherit(vh_dflt->loc_dflt->frang_cfg,
+				  &tfw_frang_vhost_reconfig))
+		return -ENOMEM;
+	sg_def = tfw_sg_lookup_reconfig(TFW_VH_DFT_NAME, SLEN("default"));


Suggested change

sg_def = tfw_sg_lookup_reconfig(TFW_VH_DFT_NAME, SLEN("default"));

sg_def = tfw_sg_lookup_reconfig(TFW_VH_DFT_NAME, SLEN(TFW_VH_DFT_NAME));

i-rinat · 2019-07-15T19:36:21Z

tempesta_fw/vhost.c

+	rcu_read_lock();
+	vh_list = rcu_dereference(tfw_vhosts);
+	rcu_read_unlock();
+	rcu_assign_pointer(tfw_vhosts, tfw_vhosts_reconfig);


I believe here is an issue with multiple threads calling this function. For example, if administrator starts Tempesta multiple times in parallel. Then we can get here in two or more threads, get non-NULL vh_list, and then try to release its contents multiple times.

The obvious solution is to wrap rcu_dereference and rcu_assign_pointer with a lock. But since this line uses tfw_vhosts_reconfig which contents is updated in many places, we need a larger scope lock that protect whole configuration sequence.

i-rinat · 2019-07-15T19:36:57Z

tempesta_fw/vhost.c

-static TfwCfgSpec tfw_vhost_location_specs[] = {
+/*
+ * Not all Frang specs can be applied to nested locations and can be applied
+ * only as high-level options. It's possible to provide it's own sets for global


Suggested change

* only as high-level options. It's possible to provide it's own sets for global

* only as high-level options. It's possible to provide their own sets for global

i-rinat · 2019-07-15T19:53:47Z

tls/ttls.h

- *		ttls_set_hs_ca_chain() as well as the client
- *		authentication mode with \c ttls_set_hs_authmode(),
+ *		certificate is found, the callback must fill the
+ *		peer tls configuration part in TLS context,


Suggested change

* peer tls configuration part in TLS context,

* peer tls configuration part in the TLS context,

avbelov23 · 2019-07-15T22:12:04Z

Corrected tests, because in this PR blocking by http_resp_code_block occurs at the response stage, and this correct.
tempesta-tech/tempesta-test#90

avbelov23 · 2019-07-15T21:58:10Z

tempesta_fw/http_limits.c

@@ -252,7 +252,7 @@ typedef struct {
 do {									\
 	char abuf[TFW_ADDR_STR_BUF_SIZE] = {0};				\
 	tfw_addr_fmt(addr, TFW_NO_PORT, abuf);				\
-	TFW_DBG("frang: " fmt_msg, abuf, ##__VA_ARGS__);		\
+	T_DBG("frang: " fmt_msg, abuf, ##__VA_ARGS__);			\


Strange space at the end of the line.

It's a tabulation. As there is \ at the end of the line, some spacing is required anyway.

avbelov23 · 2019-07-15T22:06:42Z

tls/ttls.h

+typedef struct {
+	/** allowed ciphersuites per version */
+	const int			*ciphersuite_list[4];
+	/** allowed curves	*/


Strange tabs at the end of comments and also below

…list

…ined Default vhost stores default policies for the connection or requests. Remove differences between implicit and explicit 'default' vhosts and make it possible to assign such vhost to a connection or request.

… to end user

It was discussed, that all client connections with unknown or not set sni must always use default vhost. Thus there is no need in 'tls_fallback_default' directive at all: it would be always enabled.

# Conflicts: # tempesta_fw/vhost.c

vankoven added the ready for review label Mar 1, 2019

krizhanovsky reviewed Mar 3, 2019

View reviewed changes

krizhanovsky requested review from avbelov23 and i-rinat March 3, 2019 03:36

krizhanovsky assigned i-rinat and avbelov23 Mar 3, 2019

vankoven force-pushed the ik-fix-frang-errors branch from ca946b0 to 8993afa Compare March 6, 2019 13:50

krizhanovsky requested changes Mar 13, 2019

View reviewed changes

vankoven force-pushed the ik-fix-frang-errors branch 2 times, most recently from e10a035 to ce8d74d Compare May 25, 2019 23:27

krizhanovsky mentioned this pull request Jun 2, 2019

Functional test for Frang #673

Closed

krizhanovsky requested changes Jun 2, 2019

View reviewed changes

krizhanovsky requested changes Jul 8, 2019

View reviewed changes

krizhanovsky added crucial and removed ready for review labels Jul 8, 2019

vankoven force-pushed the ik-fix-frang-errors branch from 7288cda to 2cbd599 Compare July 9, 2019 18:40

krizhanovsky reviewed Jul 10, 2019

View reviewed changes

krizhanovsky mentioned this pull request Jul 10, 2019

Separate HTTP request headers and body processing #1302

Open

krizhanovsky approved these changes Jul 10, 2019

View reviewed changes

i-rinat reviewed Jul 12, 2019

View reviewed changes

i-rinat approved these changes Jul 15, 2019

View reviewed changes

avbelov23 approved these changes Jul 15, 2019

View reviewed changes

vankoven added 7 commits July 25, 2019 12:14

Move tfw_hash_str function to separate header

c79acdd

Store vhosts in hashtables

d2ab1af

Split tls config into two parts: generic part and peer part

7ae8485

tls_fallback_default has no sense inside vhost section

5245c80

tls: choose hash functions for signatures with respect to preference …

ab4811e

…list

bump copyrights

0e73900

Update code comments

3b78972

vankoven added 26 commits July 25, 2019 12:22

tls: restyle changes in this PR from mbedTLS to Tempesta style

7a566ad

tls: fix typos

59a71a1

vhost: always prepare implicit vhost instance to be used as fully def…

6921a4e

…ined Default vhost stores default policies for the connection or requests. Remove differences between implicit and explicit 'default' vhosts and make it possible to assign such vhost to a connection or request.

tls: validate configuration more thoroughly and provide more messages…

498080b

… to end user

tls: remove configuration option 'tls_fallback_default'

f37ef0d

It was discussed, that all client connections with unknown or not set sni must always use default vhost. Thus there is no need in 'tls_fallback_default' directive at all: it would be always enabled.

tls: fix repeated call of tls_certificate_key directive

a6199ea

tls: don't open connection if client requests unknown sni

3ae76ee

frang: add functions to remove/add hooks in array manner

3850b28

Make frang limits working per-vhost and per-location

2ff9c4e

Fix frang limits to work per vhost and per location

191310b

Update configuration example

ce24da0

Update deprecated macroses

2665940

# Conflicts: # tempesta_fw/vhost.c

sample conf: add missing frang limits names

ce1dd2f

Fix rebase glitches

40023b8

Add more documentation for code as requested during code review.

7bdaa12

frang: Reduce calculations in hot path

a199430

Adress code review commeents

f457bdd

frang: switchable request trailers checks

ff10369

bump copyrighs

ba5c6f2

frang: add debug message, when the pessage is fully checked

e8bb321

frang: fix incorrect start state in frang callback

4666ff4

fix typos

2735e51

fix misspeling error

f9bb263

frang: make http_body_len long

2b41bf3

frang: correctly update ct_vals pointers when the option is inherited.

8cae88d

frang: fix inheriting frang configuration

e0e1fb7

vankoven force-pushed the ik-fix-frang-errors branch from 6b4f1be to e0e1fb7 Compare July 31, 2019 07:39

hm: assign host and location to HM request to correctly address policies

140f5dd

vankoven merged commit 18f852d into master Aug 1, 2019

vankoven deleted the ik-fix-frang-errors branch August 1, 2019 13:37

	# The section containing static limits for the classifier. Can apper at top
	# The section containing static limits for the classifier. Can appear at the top

	# directive is listed in the configuration it redefine default vaues for
	# directive is listed in the configuration it redefines default values for the

	* depending on HTTP chain configuration due to it flexibility and
	* depending on HTTP chain configuration due to its flexibility and

	* The most secure and a slow way: compare all of them and reject if
	* The most secure and slow way: compare all of them and reject if

	* at least some doesn't match. Can be too strict, service quality can
	* at least some do not match. Can be too strict, service quality can

	* connections, trying to cause work queue overrun and delay of security events
	* connections, trying to cause a work queue overrun and delay security events

	* configuration is required. In the last case @data is NULL and @len is 0.
	* configuration is required. In the latter case @data is NULL and @len is 0.

	vhost = tfw_vhost_lookup(&srv_name);
	vhost = tfw_vhost_lookup(&srv_name);

	* Implicit default vhost is still useful even if it never used to
	* Implicit default vhost is still useful even if it's never used to

	sg_def = tfw_sg_lookup_reconfig(TFW_VH_DFT_NAME, SLEN("default"));
	sg_def = tfw_sg_lookup_reconfig(TFW_VH_DFT_NAME, SLEN(TFW_VH_DFT_NAME));

	* only as high-level options. It's possible to provide it's own sets for global
	* only as high-level options. It's possible to provide their own sets for global

	* peer tls configuration part in TLS context,
	* peer tls configuration part in the TLS context,

Fix farious frang errors #1200

Fix farious frang errors #1200

Conversation

vankoven commented Mar 1, 2019 • edited Loading

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

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

krizhanovsky left a comment

Choose a reason for hiding this comment

i-rinat left a comment • 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

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

i-rinat 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

avbelov23 commented Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven commented Mar 1, 2019 •

edited

Loading

i-rinat left a comment •

edited

Loading

avbelov23 commented Jul 15, 2019 •

edited

Loading