Re: req.cook_cnt() broken?

[Willy Tarreau]

Hi Daniel,

On Fri, Aug 25, 2017 at 12:47:41PM +0200, Daniel Schneller wrote:
> On 24. Aug. 2017, at 01:50, Cyril Bonté  wrote:
> > 
> > You're right. currently, the code and the documentation don't say the same 
> > things.
> > 
> > Can you try the attached patch ?
> > 
> > -- 
> > Cyril Bonté
> > 
> 
> Thanks for the patch!
> 
> Tried against 1.8,  1.7.9, and 1.6.13 just now. Works as expected with all 
> three. :D
> 
> Any chance of getting this fix backported to the 1.7 and ideally 1.6 branches?
> 
> It would come in handy on a production system currently running 1.6 that I
> cannot easily upgrade to 1.7.

Don't worry, we *always* backport fixes as far as relevant (so possibly
even 1.5 and 1.4). We know that haproxy is such a sensitive component
which once deployed rarely experiences major upgrades, so what's most
important is that what is deployed works.

We're late on 1.6 fixes by the way, the bug chasing in 1.7 has made
us uncertain about a few fixes for a while, causing us to wait before
taking risks on 1.6. I think we'll soon be more confident in preparing
1.6.14, likely with the aforementionned fix :-)

Cheers,
Willy

Re: req.cook_cnt() broken?

[Daniel Schneller]

On 24. Aug. 2017, at 01:50, Cyril Bonté  wrote:
> 
> You're right. currently, the code and the documentation don't say the same 
> things.
> 
> Can you try the attached patch ?
> 
> -- 
> Cyril Bonté
> 

Thanks for the patch!

Tried against 1.8,  1.7.9, and 1.6.13 just now. Works as expected with all 
three. :D

Any chance of getting this fix backported to the 1.7 and ideally 1.6 branches?

It would come in handy on a production system currently running 1.6 that I 
cannot easily upgrade to 1.7.


Cheers,
Daniel


-- 
Daniel Schneller
Principal Cloud Engineer
 
CenterDevice GmbH  | Hochstraße 11
   | 42697 Solingen
tel: +49 1754155711| Deutschland
daniel.schnel...@centerdevice.de   | www.centerdevice.de

Geschäftsführung: Dr. Patrick Peschlow, Dr. Lukas Pustina,
Michael Rosbach, Handelsregister-Nr.: HRB 18655,
HR-Gericht: Bonn, USt-IdNr.: DE-815299431

Re: req.cook_cnt() broken?

[Cyril Bonté]

Hi Daniel,

Le 23/08/2017 à 11:50, Daniel Schneller a écrit :

Kindly bumping this during the summer vacation time for potentially new 
recipients :)



On 21. Aug. 2017, at 21:14, Daniel Schneller 
 wrote:
Hi!

According to the documentation

  req.cook_cnt([]) : integer
  Returns an integer value representing the number of occurrences of the cookie
   in the request, or all cookies if  is not specified.

it should be possible to do something like this to reject a request if it contains 
more than  cookies total. I do not know the cookie names in advance. I am 
trying to reject malicious requests with hundreds or thousands of cookies, trying to 
exhaust memory in my backend servers. Tomcat has a maximum number of cookies per 
request setting, but I’d like to reject these before they even get to the backends.

I thought this would work (for n=2):

frontend fe-test
bind 0.0.0.0:8070
http-request deny deny_status 400 if { req.cook_cnt() gt 2 }
http-request auth realm tomcat
default_backend be-test


However, it does not work. The count is always 0, hence the ACL always pass >> 
[...]
When I change the ACL to include a cookie name, it works:

http-request deny deny_status 400 if { req.cook_cnt("C1") gt 2 }
[...]
I tried to figure out what the code does, to see if I am doing something wrong 
and found this in proto_http.c:

--
/* Iterate over all cookies present in a request to count how many occurrences
* match the name in args and args->data.str.len. If  is non-null, then
* multiple cookies may be parsed on the same line. The returned sample is of
* type UINT. Accepts exactly 1 argument of type string.
*/
static int
smp_fetch_cookie_cnt(const struct arg *args, struct sample *smp, const char 
*kw, void *private)
{
struct http_txn *txn;
struct hdr_idx *idx;
struct hdr_ctx ctx;
const struct http_msg *msg;
const char *hdr_name;
int hdr_name_len;
int cnt;
char *val_beg, *val_end;
char *sol;

if (!args || args->type != ARGT_STR)
return 0;
--

So without being very C-savvy, this appears to exit early when there is no 
parameter of type string passed in.

I hope someone can shed some light on this. :)


You're right. currently, the code and the documentation don't say the 
same things.


Can you try the attached patch ?

--
Cyril Bonté
diff --git a/src/proto_http.c b/src/proto_http.c
index 2b9b0d01e..0935f0145 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -11318,8 +11318,8 @@ extract_cookie_value(char *hdr, const char *hdr_end,
 		 * its value between val_beg and val_end.
 		 */
 
-		if (att_end - att_beg == cookie_name_l &&
-		memcmp(att_beg, cookie_name, cookie_name_l) == 0) {
+		if (cookie_name_l == 0 || (att_end - att_beg == cookie_name_l &&
+		memcmp(att_beg, cookie_name, cookie_name_l) == 0)) {
 			/* let's return this value and indicate where to go on from */
 			*value = val_beg;
 			*value_l = val_end - val_beg;
@@ -11617,7 +11617,7 @@ smp_fetch_cookie_cnt(const struct arg *args, struct sample *smp, const char *kw,
 	char *val_beg, *val_end;
 	char *sol;
 
-	if (!args || args->type != ARGT_STR)
+	if (!args || (args->type != ARGT_STR && args->type != ARGT_STOP))
 		return 0;
 
 	CHECK_HTTP_MESSAGE_FIRST();

Re: req.cook_cnt() broken?

[Daniel Schneller]

Kindly bumping this during the summer vacation time for potentially new 
recipients :)


> On 21. Aug. 2017, at 21:14, Daniel Schneller 
>  wrote:
> 
> Hi!
> 
> According to the documentation
> 
>  req.cook_cnt([]) : integer
>  Returns an integer value representing the number of occurrences of the cookie
>   in the request, or all cookies if  is not specified.
> 
> it should be possible to do something like this to reject a request if it 
> contains more than  cookies total. I do not know the cookie names in 
> advance. I am trying to reject malicious requests with hundreds or thousands 
> of cookies, trying to exhaust memory in my backend servers. Tomcat has a 
> maximum number of cookies per request setting, but I’d like to reject these 
> before they even get to the backends.
> 
> I thought this would work (for n=2):
> 
>   frontend fe-test
>   bind 0.0.0.0:8070
>   http-request deny deny_status 400 if { req.cook_cnt() gt 2 }
>   http-request auth realm tomcat
>   default_backend be-test
> 
> 
> However, it does not work. The count is always 0, hence the ACL always passes 
> and I get a 401 response from the next ACL in line.
> 
> root@tomcat:~# curl -v -b 'C1=v1; C1=v2; C1=v3' tomcat:8070
> * Rebuilt URL to: tomcat:8070/
> * Hostname was NOT found in DNS cache
> *   Trying 127.0.1.1...
> * Connected to tomcat (127.0.1.1) port 8070 (#0)
>> GET / HTTP/1.1
>> User-Agent: curl/7.35.0
>> Host: tomcat:8070
>> Accept: */*
>> Cookie: C1=v1; C1=v2; C1=v3
>> 
> * HTTP 1.0, assume close after body
> < HTTP/1.0 401 Unauthorized
> < Cache-Control: no-cache
> < Connection: close
> < Content-Type: text/html
> < WWW-Authenticate: Basic realm="tomcat"
> <
> 401 Unauthorized
> You need a valid user and password to access this content.
> 
> * Closing connection 0
> 
> 
> When I change the ACL to include a cookie name, it works:
> 
>http-request deny deny_status 400 if { req.cook_cnt("C1") gt 2 }
> 
> root@tomcat:~# curl -v -b 'C1=v1; C1=v2; C1=v3' tomcat:8070
> * Rebuilt URL to: tomcat:8070/
> * Hostname was NOT found in DNS cache
> *   Trying 127.0.1.1...
> * Connected to tomcat (127.0.1.1) port 8070 (#0)
>> GET / HTTP/1.1
>> User-Agent: curl/7.35.0
>> Host: tomcat:8070
>> Accept: */*
>> Cookie: C1=v1; C1=v2; C1=v3
>> 
> * HTTP 1.0, assume close after body
> < HTTP/1.0 400 Bad request
> < Cache-Control: no-cache
> < Connection: close
> < Content-Type: text/html
> <
> 400 Bad request
> Your browser sent an invalid request.
> 
> * Closing connection 0
> 
> 
> 
> I tried to figure out what the code does, to see if I am doing something 
> wrong and found this in proto_http.c:
> 
> --
> /* Iterate over all cookies present in a request to count how many occurrences
> * match the name in args and args->data.str.len. If  is non-null, then
> * multiple cookies may be parsed on the same line. The returned sample is of
> * type UINT. Accepts exactly 1 argument of type string.
> */
> static int
> smp_fetch_cookie_cnt(const struct arg *args, struct sample *smp, const char 
> *kw, void *private)
> {
>   struct http_txn *txn;
>   struct hdr_idx *idx;
>   struct hdr_ctx ctx;
>   const struct http_msg *msg;
>   const char *hdr_name;
>   int hdr_name_len;
>   int cnt;
>   char *val_beg, *val_end;
>   char *sol;
> 
>   if (!args || args->type != ARGT_STR)
>   return 0;
> --
> 
> So without being very C-savvy, this appears to exit early when there is no 
> parameter of type string passed in.
> 
> I hope someone can shed some light on this. :)
> 
> Thanks in advance,
> Daniel
> 
> 
> -- 
> Daniel Schneller
> Principal Cloud Engineer
> 
> CenterDevice GmbH  | Hochstraße 11
>   | 42697 Solingen
> tel: +49 1754155711| Deutschland
> daniel.schnel...@centerdevice.de   | www.centerdevice.de
> 
> Geschäftsführung: Dr. Patrick Peschlow, Dr. Lukas Pustina,
> Michael Rosbach, Handelsregister-Nr.: HRB 18655,
> HR-Gericht: Bonn, USt-IdNr.: DE-815299431
> 
> 

req.cook_cnt() broken?

[Daniel Schneller]

Hi!

According to the documentation

  req.cook_cnt([]) : integer
  Returns an integer value representing the number of occurrences of the cookie
   in the request, or all cookies if  is not specified.

it should be possible to do something like this to reject a request if it 
contains more than  cookies total. I do not know the cookie names in 
advance. I am trying to reject malicious requests with hundreds or thousands of 
cookies, trying to exhaust memory in my backend servers. Tomcat has a maximum 
number of cookies per request setting, but I’d like to reject these before they 
even get to the backends.

I thought this would work (for n=2):

frontend fe-test
bind 0.0.0.0:8070
http-request deny deny_status 400 if { req.cook_cnt() gt 2 }
http-request auth realm tomcat
default_backend be-test


However, it does not work. The count is always 0, hence the ACL always passes 
and I get a 401 response from the next ACL in line.

root@tomcat:~# curl -v -b 'C1=v1; C1=v2; C1=v3' tomcat:8070
* Rebuilt URL to: tomcat:8070/
* Hostname was NOT found in DNS cache
*   Trying 127.0.1.1...
* Connected to tomcat (127.0.1.1) port 8070 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.35.0
> Host: tomcat:8070
> Accept: */*
> Cookie: C1=v1; C1=v2; C1=v3
>
* HTTP 1.0, assume close after body
< HTTP/1.0 401 Unauthorized
< Cache-Control: no-cache
< Connection: close
< Content-Type: text/html
< WWW-Authenticate: Basic realm="tomcat"
<
401 Unauthorized
You need a valid user and password to access this content.

* Closing connection 0


When I change the ACL to include a cookie name, it works:

http-request deny deny_status 400 if { req.cook_cnt("C1") gt 2 }

root@tomcat:~# curl -v -b 'C1=v1; C1=v2; C1=v3' tomcat:8070
* Rebuilt URL to: tomcat:8070/
* Hostname was NOT found in DNS cache
*   Trying 127.0.1.1...
* Connected to tomcat (127.0.1.1) port 8070 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.35.0
> Host: tomcat:8070
> Accept: */*
> Cookie: C1=v1; C1=v2; C1=v3
>
* HTTP 1.0, assume close after body
< HTTP/1.0 400 Bad request
< Cache-Control: no-cache
< Connection: close
< Content-Type: text/html
<
400 Bad request
Your browser sent an invalid request.

* Closing connection 0



I tried to figure out what the code does, to see if I am doing something wrong 
and found this in proto_http.c:

--
/* Iterate over all cookies present in a request to count how many occurrences
 * match the name in args and args->data.str.len. If  is non-null, then
 * multiple cookies may be parsed on the same line. The returned sample is of
 * type UINT. Accepts exactly 1 argument of type string.
 */
static int
smp_fetch_cookie_cnt(const struct arg *args, struct sample *smp, const char 
*kw, void *private)
{
struct http_txn *txn;
struct hdr_idx *idx;
struct hdr_ctx ctx;
const struct http_msg *msg;
const char *hdr_name;
int hdr_name_len;
int cnt;
char *val_beg, *val_end;
char *sol;

if (!args || args->type != ARGT_STR)
return 0;
--

So without being very C-savvy, this appears to exit early when there is no 
parameter of type string passed in.

I hope someone can shed some light on this. :)

Thanks in advance,
Daniel


-- 
Daniel Schneller
Principal Cloud Engineer
 
CenterDevice GmbH  | Hochstraße 11
   | 42697 Solingen
tel: +49 1754155711| Deutschland
daniel.schnel...@centerdevice.de   | www.centerdevice.de

Geschäftsführung: Dr. Patrick Peschlow, Dr. Lukas Pustina,
Michael Rosbach, Handelsregister-Nr.: HRB 18655,
HR-Gericht: Bonn, USt-IdNr.: DE-815299431