Re: http2 smuggling
I could be wrong, but I think he is stating that if you have that allowed, it can be used to get a direct connection to the backend bypassing any routing or acls you have in the load balancer, so if you some endpoints are blocked, or internal only, they could potentially be accessed this way. For example, if you have something like: acl is_restrict path_sub /.git/ http-request deny if is_restrict !is_safe_ip The acl could be bypassed by using the method to connect directly to a backend. That's not to say it's a security flaw in haproxy, but a potential misconfiguration that could allow traffic you thought was blocked by the proxy. On Fri, Sep 11, 2020 at 2:07 AM Willy Tarreau wrote: > > Hi Igor, > > On Fri, Sep 11, 2020 at 01:55:10PM +1000, Igor Cicimov wrote: > > Should we be worried? > > > > https://portswigger.net/daily-swig/http-request-smuggling-http-2-opens-a-new-attack-tunnel > > But this stuff is total non-sense. Basically the guy is complaining > that the products he tested work exactly as desired, designed and > documented! > > The principle of the upgrade at the gateway level precisely is to say > "OK both the client and the server want to speak another protocol you > agreed upon, let me retract" and let them talk over a tunnel. That's > exactly what is needed to support WebSocket for example. The simple > fact that he found that many proxies/gateways work like this should > ring a bell about the intended behavior! > > In addition there is zero smuggling here as there is no desynchronisation. > It's just a tunnel between the client and the server, both agreeing to > do so. It does *exactly* the same as if the client had connected to the > server using a CONNECT method and the server had returned 200. So there > is absolutely no attack nor whatever here, just a connection that remains > dedicated to a client and a server till the end. > > Sadly, as usual after people discover protocols during the summer, some > journalists will surely want to make noise about this to put some bread > on their table... > > Thanks for the link anyway I had a partial laugh; partial only because > it makes useless noise. > > Cheers, > Willy >
Re: [*EXT*] Re: http2 smuggling
Hi Willy, Being devil's advocate : isn't the point that even if this is a documented, standardized and intended behavior, users relying on the reverse proxy for security/sanity checks could by tricked by this feature inadvertently ? -- Ionel GARDAIS Tech'Advantage CIO - IT Team manager - Mail original - De: "Willy Tarreau" À: "Igor Cicimov" Cc: "haproxy" Envoyé: Vendredi 11 Septembre 2020 08:19:12 Objet: [*EXT*] Re: http2 smuggling On Fri, Sep 11, 2020 at 08:07:02AM +0200, Willy Tarreau wrote: > Sadly, as usual after people discover protocols during the summer, some > journalists will surely want to make noise about this to put some bread > on their table... > > Thanks for the link anyway I had a partial laugh; partial only because > it makes useless noise. And sadly, this one already started to make some noise there about his recent discovery of a 20-years old standard: https://twitter.com/theBumbleSec Had he asked if we supported 101, we could even have saved him time in his HTTP discover test by pointing him to the doc: http://git.haproxy.org/?p=haproxy.git;a=blob;f=doc/configuration.txt;h=c1f6f82;hb=HEAD#l332 Probably that next year he will discover that we also support CONNECT. It's not even funny, the world is really doomed... Willy -- 232 avenue Napoleon BONAPARTE 92500 RUEIL MALMAISON Capital EUR 219 300,00 - RCS Nanterre B 408 832 301 - TVA FR 09 408 832 301
Re: http2 smuggling
On Fri, Sep 11, 2020 at 08:07:02AM +0200, Willy Tarreau wrote: > Sadly, as usual after people discover protocols during the summer, some > journalists will surely want to make noise about this to put some bread > on their table... > > Thanks for the link anyway I had a partial laugh; partial only because > it makes useless noise. And sadly, this one already started to make some noise there about his recent discovery of a 20-years old standard: https://twitter.com/theBumbleSec Had he asked if we supported 101, we could even have saved him time in his HTTP discover test by pointing him to the doc: http://git.haproxy.org/?p=haproxy.git;a=blob;f=doc/configuration.txt;h=c1f6f82;hb=HEAD#l332 Probably that next year he will discover that we also support CONNECT. It's not even funny, the world is really doomed... Willy
Re: http2 smuggling
Hi Igor, On Fri, Sep 11, 2020 at 01:55:10PM +1000, Igor Cicimov wrote: > Should we be worried? > > https://portswigger.net/daily-swig/http-request-smuggling-http-2-opens-a-new-attack-tunnel But this stuff is total non-sense. Basically the guy is complaining that the products he tested work exactly as desired, designed and documented! The principle of the upgrade at the gateway level precisely is to say "OK both the client and the server want to speak another protocol you agreed upon, let me retract" and let them talk over a tunnel. That's exactly what is needed to support WebSocket for example. The simple fact that he found that many proxies/gateways work like this should ring a bell about the intended behavior! In addition there is zero smuggling here as there is no desynchronisation. It's just a tunnel between the client and the server, both agreeing to do so. It does *exactly* the same as if the client had connected to the server using a CONNECT method and the server had returned 200. So there is absolutely no attack nor whatever here, just a connection that remains dedicated to a client and a server till the end. Sadly, as usual after people discover protocols during the summer, some journalists will surely want to make noise about this to put some bread on their table... Thanks for the link anyway I had a partial laugh; partial only because it makes useless noise. Cheers, Willy
Re: [PATCH 0/5] More deinit / free stuff
On Thu, Sep 10, 2020 at 07:46:37PM +0200, Tim Duesterhus wrote: > Willy, > > this basically is a continuation from my series in July: > > https://www.mail-archive.com/haproxy@formilux.org/msg37808.html > > One more real bugfix and 4 more frees of live allocations for consistency with > the surrounding code. Whole series applied, thank you Tim! willy
http2 smuggling
Should we be worried? https://portswigger.net/daily-swig/http-request-smuggling-http-2-opens-a-new-attack-tunnel IC
[PATCH 5/5] CLEANUP: haproxy: Free post_check_list in deinit()
This allocation is technically always reachable and cannot leak, but so are
a few others that *are* freed.
---
src/haproxy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index 0dc81c78c..e70870ab0 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2407,6 +2407,7 @@ void deinit(void)
struct per_thread_alloc_fct *taf, *tafb;
struct per_thread_free_fct *tff, *tffb;
struct post_server_check_fct *pscf, *pscfb;
+ struct post_check_fct *pcf, *pcfb;
struct post_proxy_check_fct *ppcf, *ppcfb;
deinit_signals();
@@ -2722,6 +2723,11 @@ void deinit(void)
free(srvdf);
}
+ list_for_each_entry_safe(pcf, pcfb, &post_check_list, list) {
+ LIST_DEL(&pcf->list);
+ free(pcf);
+ }
+
list_for_each_entry_safe(pscf, pscfb, &post_server_check_list, list) {
LIST_DEL(&pscf->list);
free(pscf);
--
2.28.0
[PATCH 3/5] CLEANUP: haproxy: Free post_proxy_check_list in deinit()
This allocation is technically always reachable and cannot leak, but so are
a few others that *are* freed.
---
src/haproxy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index 663922e72..b01707096 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2403,6 +2403,7 @@ void deinit(void)
struct proxy_deinit_fct *pxdf, *pxdfb;
struct server_deinit_fct *srvdf, *srvdfb;
struct post_server_check_fct *pscf, *pscfb;
+ struct post_proxy_check_fct *ppcf, *ppcfb;
deinit_signals();
while (p) {
@@ -2722,6 +2723,11 @@ void deinit(void)
free(pscf);
}
+ list_for_each_entry_safe(ppcf, ppcfb, &post_proxy_check_list, list) {
+ LIST_DEL(&ppcf->list);
+ free(ppcf);
+ }
+
vars_prune(&global.vars, NULL, NULL);
pool_destroy_all();
deinit_pollers();
--
2.28.0
[PATCH 1/5] BUG/MINOR: haproxy: Free uri_auth->scope during deinit
Given the following example configuration:
listen http
bind *:80
mode http
stats scope .
Running a configuration check with valgrind reports:
==16341== 26 (24 direct, 2 indirect) bytes in 1 blocks are definitely lost
in loss record 3 of 13
==16341==at 0x4C2FB55: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16341==by 0x571C2E: stats_add_scope (uri_auth.c:296)
==16341==by 0x46CE29: cfg_parse_listen (cfgparse-listen.c:1901)
==16341==by 0x45A112: readcfgfile (cfgparse.c:2078)
==16341==by 0x50A0F5: init (haproxy.c:1828)
==16341==by 0x418248: main (haproxy.c:3012)
After this patch is applied the leak is gone as expected.
This is a very minor leak that can only be observed if deinit() is called,
shortly before the OS will free all memory of the process anyway. No
backport needed.
---
src/haproxy.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index e8cbdf410..5ae21824a 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2637,6 +2637,8 @@ void deinit(void)
}/* end while(p) */
while (ua) {
+ struct stat_scope *scope, *scopep;
+
uap = ua;
ua = ua->next;
@@ -2648,6 +2650,15 @@ void deinit(void)
userlist_free(uap->userlist);
deinit_act_rules(&uap->http_req_rules);
+ scope = uap->scope;
+ while (scope) {
+ scopep = scope;
+ scope = scope->next;
+
+ free(scopep->px_id);
+ free(scopep);
+ }
+
free(uap);
}
--
2.28.0
[PATCH 2/5] CLEANUP: Free old_argv on deinit
This allocation technically is always reachable and cannot leak, however other global variables such as `oldpids` are already being freed. This is in an attempt to get HAProxy to a state where there are zero live allocations after a clean exit. --- src/haproxy.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/haproxy.c b/src/haproxy.c index 5ae21824a..663922e72 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -2680,6 +2680,7 @@ void deinit(void) free(global.node);global.node = NULL; free(global.desc);global.desc = NULL; free(oldpids);oldpids = NULL; + free(old_argv); old_argv = NULL; free(localpeer); localpeer = NULL; task_destroy(idle_conn_task); idle_conn_task = NULL; -- 2.28.0
[PATCH 4/5] CLEANUP: haproxy: Free per_thread_*_list in deinit()
This allocation is technically always reachable and cannot leak, but so are
a few others that *are* freed.
---
src/haproxy.c | 24
1 file changed, 24 insertions(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index b01707096..0dc81c78c 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2402,6 +2402,10 @@ void deinit(void)
struct post_deinit_fct *pdf, *pdfb;
struct proxy_deinit_fct *pxdf, *pxdfb;
struct server_deinit_fct *srvdf, *srvdfb;
+ struct per_thread_init_fct *tif, *tifb;
+ struct per_thread_deinit_fct *tdf, *tdfb;
+ struct per_thread_alloc_fct *taf, *tafb;
+ struct per_thread_free_fct *tff, *tffb;
struct post_server_check_fct *pscf, *pscfb;
struct post_proxy_check_fct *ppcf, *ppcfb;
@@ -2728,6 +2732,26 @@ void deinit(void)
free(ppcf);
}
+ list_for_each_entry_safe(tif, tifb, &per_thread_init_list, list) {
+ LIST_DEL(&tif->list);
+ free(tif);
+ }
+
+ list_for_each_entry_safe(tdf, tdfb, &per_thread_deinit_list, list) {
+ LIST_DEL(&tdf->list);
+ free(tdf);
+ }
+
+ list_for_each_entry_safe(taf, tafb, &per_thread_alloc_list, list) {
+ LIST_DEL(&taf->list);
+ free(taf);
+ }
+
+ list_for_each_entry_safe(tff, tffb, &per_thread_free_list, list) {
+ LIST_DEL(&tff->list);
+ free(tff);
+ }
+
vars_prune(&global.vars, NULL, NULL);
pool_destroy_all();
deinit_pollers();
--
2.28.0
[PATCH 0/5] More deinit / free stuff
Willy, this basically is a continuation from my series in July: https://www.mail-archive.com/haproxy@formilux.org/msg37808.html One more real bugfix and 4 more frees of live allocations for consistency with the surrounding code. Best regards Tim Düsterhus (5): BUG/MINOR: haproxy: Free uri_auth->scope during deinit CLEANUP: Free old_argv on deinit CLEANUP: haproxy: Free post_proxy_check_list in deinit() CLEANUP: haproxy: Free per_thread_*_list in deinit() CLEANUP: haproxy: Free post_check_list in deinit() src/haproxy.c | 48 1 file changed, 48 insertions(+) -- 2.28.0
Re: [PATCH] ci: travis-ci: help coverity to recognize abort
Hi Ilya, On Thu, Sep 10, 2020 at 09:45:08PM +0500, ??? wrote: > ping :) Ah sorry, thanks for the reminedr, I remember reading it and thought it was merged, but I was wrong. However I'm seeing two mistakes: - first patch accidently merged a copy of your libwurfl.a. Don't worry, I'll edit the patch to get rid of it, that's easy. - the second one doesn't explain what the problem was and it's hard to figure it from just the patch itself. Please keep in mind that the purpose of the commit message is in the first place to quickly explain why the patch has to exist in the first place. I can help redact a bit of the message if you're not comfortable with it but I need a bit of input. Thanks! Willy
Re: [PATCH] ci: travis-ci: help coverity to recognize abort
ping :) вс, 6 сент. 2020 г. в 13:40, Илья Шипицин : > Hello, > > based on discussion https://github.com/haproxy/haproxy/issues/755 > > cheers, > Ilya Shipitcin >
Re: Debugging ssl handshake failures
Thanks Bruno, I'll see if I can get this working. -- Kevin On 2020-09-09 9:41 p.m., Bruno Henc wrote: Hi, I take it that means theres no internal debug logging for the tls errors that we can just expose via logfile? Proof of concept patches are attached with build instructions. You may wish to edit the haproxy-2.2.3/rules/debian folder to increase the -j setting to your current number of cores. The "disambiguate-ssl-handshake-errors-1.patch" only adds additional error messages for the initial ClientHello processing - realistically, it's only useful to see if there is no SNI being sent (bots, healthchecks are the usual offenders). The "disambiguate-ssl-handshake-errors-2.patch" implements everything the first patch implements, adds a trash chunk for logging additional error data to the conn structure, and reuses the SSL error logging logic from ssl_sock_dump_errors Practically, this means that memory usage is higher - if I recall correctly (and it's way to late/early at this point) it's a 16KB overhead per connection (echo "show pools" | socat stdio /var/run/haproxy.sock will have a more detailed breakdown). Watching the output of show pools is recommended - while I haven't noticed a memory leak, keeping an eye on the trash pool is a good idea. The fcgi protocol is also affected by the addition of the extra_err_data. I have to do a smoke test if "proto fcgi" behaves as expected, or if there's a potential segfault. The patch works, but it requires more extensive testing. Sharing it as-is since I might not be able to pursue this further in a significant way for some time. The mapping between the error messages and the potential causes can be a bit obscure, but it's still useful. E.g. an invalid SNI when using strict-sni maps to: tls_post_process_client_hello: no shared cipher If there's a cipher mismatch, this also maps to the above error message. A protocol version mismatch (e.g. trying TLS1 when only TLS1.2 is supported) results in: tls_early_post_process_client_hello: unsupported protocol. The list of error codes is available upstream at https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/err/openssl.txt#L2774 . Regarding the packet capture question - exporting libpcap data via SPOE might be possible. It's an ongoing topic. Regards, Bruno
