Re: [PATCH] CLEANUP: ssl: Clean up error handling

2019-11-25 Thread Willy Tarreau
On Mon, Nov 25, 2019 at 09:17:51PM +0100, Tim Düsterhus wrote:
> Willy,
> 
> Am 25.11.19 um 17:57 schrieb Willy Tarreau:
> > I agree that it's cleaner this way, however it then uncovers another
> > issue which is that *if* ever called with a NULL err then it will leak
> > memory. William said in the issue discussion that the functions are not
> 
> Will it actually leak memory? `memprintf` is a no-op if the given `char
> **out` is NULL. That is: It won't allocate anything.

Ah you're right, sorry. I always make this confusion because its initial
implementation behave like this and it was painful to use, so it changed
but I must have been traumatized by the old behavior as you see :-)

> Unless my C foo totally leaves me right now only if the functions are
> being passed a valid pointer to a null pointer (*err = NULL) there will
> be a leak if the caller does not free the memory at *err after it has
> possibly been re-allocated by `memprintf`.

You're totally right. Then your patch is OK and I'm applying it.

Thanks!
Willy



Re: [PATCH] CLEANUP: ssl: Clean up error handling

2019-11-25 Thread William Dauchy
On Mon, Nov 25, 2019 at 05:57:04PM +0100, Willy Tarreau wrote:
> What I'd suggest instead as a better and more durable cleanup would be
> to explicitly mention above the function's prototype that it must not
> be called with a null err pointer, and remove all "if (err)" or "err &&"
> tests so that we are consistent across the whole function. This way it
> will be easier to spot any offender. Because even if right now nobody
> calls it with a NULL, it suffices to read the first few lines to see
> the check being done and believe that it's permitted, so we do have a
> latent bug quietly waiting for a victim to pass by.

I also agree with this. Removing all if and "err &&" will make it much
more readable as long as it is clearly stated in the function
definition.

-- 
William



Re: [PATCH] CLEANUP: ssl: Clean up error handling

2019-11-25 Thread Tim Düsterhus
Willy,

Am 25.11.19 um 17:57 schrieb Willy Tarreau:
> I agree that it's cleaner this way, however it then uncovers another
> issue which is that *if* ever called with a NULL err then it will leak
> memory. William said in the issue discussion that the functions are not

Will it actually leak memory? `memprintf` is a no-op if the given `char
**out` is NULL. That is: It won't allocate anything.

Unless my C foo totally leaves me right now only if the functions are
being passed a valid pointer to a null pointer (*err = NULL) there will
be a leak if the caller does not free the memory at *err after it has
possibly been re-allocated by `memprintf`.

Best regards
Tim Düsterhus



Re: [PATCH] BUG/MINOR: ssl: Stop passing dynamic strings as format arguments

2019-11-25 Thread Tim Düsterhus
William,

Am 25.11.19 um 08:57 schrieb William Lallemand:
> Merged, Thanks Tim.
> 
> I removed the mention to the backport because it's in master only and mustn't
> be backported.
> 

When the other commit is not going to be backported either then that's
okay :-)

Thanks
Tim Düsterhus



Re: [PATCH] CLEANUP: ssl: Clean up error handling

2019-11-25 Thread Willy Tarreau
On Sat, Nov 23, 2019 at 11:45:10PM +0100, Tim Duesterhus wrote:
> This commit removes the explicit checks for `if (err)` before
> passing `err` to `memprintf`. `memprintf` already checks itself
> whether the `**out*` parameter is `NULL` before doing anything.
> This reduces the indentation depth and makes the code more readable,
> before there is less boilerplate code.
> 
> Instead move the check into the ternary conditional when the error
> message should be appended to a previous message. This is consistent
> with the rest of ssl_sock.c and with the rest of HAProxy.
> 
> Thus this patch is the arguably cleaner fix for issue #374 and builds
> upon
> 5f1fa7db86c53827c97f8a8c3f5fa75bfcb5be9a and
> 8b453912ce9a4e1a3b1329efb2af04d1e470852e
> 
> Additionally it fixes a few places where the check *still* was missing.

I agree that it's cleaner this way, however it then uncovers another
issue which is that *if* ever called with a NULL err then it will leak
memory. William said in the issue discussion that the functions are not
meant to be called with err=NULL but there are checks for this situation
at various places which make all this still very confusing, and the
function header makes no mention about input/output arguments.

What I'd suggest instead as a better and more durable cleanup would be
to explicitly mention above the function's prototype that it must not
be called with a null err pointer, and remove all "if (err)" or "err &&"
tests so that we are consistent across the whole function. This way it
will be easier to spot any offender. Because even if right now nobody
calls it with a NULL, it suffices to read the first few lines to see
the check being done and believe that it's permitted, so we do have a
latent bug quietly waiting for a victim to pass by.

Cheers,
Willy



[ANNOUNCE] haproxy-2.1.0

2019-11-25 Thread Willy Tarreau
Hi,

HAProxy 2.1.0 was released on 2019/11/25. It added 45 new commits
after version 2.1-dev5.

As some might have noticed, the last week was quite calm except the last
few days with a few unexpected bugs to deal with. But that's better than
having bugs immediately after the release forcing a new version to be
emitted, so I'm not complaining :-)

For those not following development closely, 2.1 is a stable branch that
will be maintained till around Q1 2021, and is mostly aimed at experienced
users, just like 1.9 was.

The most sensitive changes since 2.0 that may possibly burn you include :
  - improvements to multi-threading: it's now possible to wake up a
tasklet scheduled on another thread. The multi-queue connection
listener now exploits these multi-threaded tasklets to further
increase its performance and decrease latency (it used to rely on
the heavier tasks in 2.0).

  - fd-cache removal: I/O handlers are now updated directly from the
pollers, and I/O completion enable/disable the pollers. It could
theorically result in more calls to epoll_ctl() if we missed
something but practically speaking we've seen a boost of ~20% of
connection rate thanks to this. Any report of regression on a
corner case workload is welcome.

  - legacy HTTP mode removal, HTX is now mandatory. That's it. As
planned, only HTX remains implemented, and with the drop of the
18-years old HTTP engine that had become extremely difficult to
maintain and adapt to new features, we also got rid of a large
number of tricky corner cases and pending bugs. Still we know
that HTX remains young but given that it's already required for
H2 backends, L7 retries, fastcgi, prometheus and I don't remember
what else, it didn't make sense to keep an old mechanism conflicting
with existing features and preventing from cleaning them up. By the
way, this also implied the removal of the old deprecated "http-tunnel"
mode.

And for the user-visible stuff, we can enumerate this :
  - support of FastCGI servers (FastCGI is basically a different encoding
of HTTP, it was an obvious next step with HTX always on). For some
simple setups, it can simplify deployments by avoiding the need for
multiple layers.

  - merging of same certificates: this will boot much faster on configs
with insane amounts of certificates (10k-100k) and will save a lot of
memory when multiple bind lines use the same certificate.

  - support of runtime certificate updates. It's now possible to change
existing certs without reloading. Creation is yet another challenge
and I understood that there are also some limitations to certain
situations where updates are still not possible (though an error
message will indicate it).

  - logging to CLI: it's now possible to log to a ring buffer that can
be consulted from the CLI. This can help when logs are exported far
away and there's no local storage to keep a recent history.

  - tracing of H1/H2/FCGI: the 3 HTTP-based protocols received lots of
trace points which can dynamically enabled at run time at various
verbosity levels and triggers in order to observe what is happening,
entering/leaving haproxy. At a low verbosity level this can simply
be used as a live request logger from the CLI.

  - the prometheus-exporter now supports filtering exported metrics by
scope. The principle is to avoid dumping everything when only servers
or frontends are required for example.

  - all stats metrics include a human readable description of what the
metric is and what it relates to. This is visible using "show info desc"
or "show stat typed desc".

  - new directives to work around bogus web applications which incorrectly
expect that some HTTP header fields match a certain case. This feature
was backported to 2.0.10 to ease transition to HTX.

  - some long-obsolete keywords were now removed. These include the reqadd,
reqdel, reqrep, etc that were designed in version 1.1 to match a full
line from the incoming stream using regexes. They were totally emulated
for a while and since 1.9 with HTX it became a total mess as the request
had to be reformatted on the fly just for the purpose of matching a regex.
Not to mention the mess of these "(^[^\ ]\+)" rules to match a method
before a path. The config parser will suggest what to use instead when
facing such a rule.

  - strict-limits: we've all been used to see haproxy warn on startup that
it didn't have enough FDs to allocate the required number of connections
but startup nevertheless. A number of people got caught in production
with this, especially more recently with systemd where warnings do not
appear on the console by default anymore. The new "strict-limits"
directive makes haproxy refuse to start when conditions are not met. It
is not enabled by default but the default will change in 2.3 to be 

[ANNOUNCE] haproxy-1.8.23

2019-11-25 Thread Willy Tarreau
Hi,

HAProxy 1.8.23 was released on 2019/11/25. It added 14 new commits
after version 1.8.22.

This version is mostly aimed at addressing the header name encoding
issue in HTTP/2. In addition it fixes a corner case where a listener
may loop eating CPU when reaching the frontend/process' connection
limit, and a few other minor issues.

Please find the usual URLs below :
   Site index   : http://www.haproxy.org/
   Discourse: http://discourse.haproxy.org/
   Slack channel: https://slack.haproxy.org/
   Issue tracker: https://github.com/haproxy/haproxy/issues
   Sources  : http://www.haproxy.org/download/1.8/src/
   Git repository   : http://git.haproxy.org/git/haproxy-1.8.git/
   Git Web browsing : http://git.haproxy.org/?p=haproxy-1.8.git
   Changelog: http://www.haproxy.org/download/1.8/src/CHANGELOG
   Cyril's HTML doc : http://cbonte.github.io/haproxy-dconv/

Willy
---
Complete changelog :
Baptiste Assmann (1):
  BUG: dns: timeout resolve not applied for valid resolutions

Christopher Faulet (2):
  BUG/MEDIUM: stream: Be sure to support splicing at the mux level to 
enable it
  BUG/MEDIUM: stream: Be sure to release allocated captures for TCP streams

Emmanuel Hocdet (1):
  BUG/MINOR: ssl: fix crt-list neg filter for openssl < 1.1.1

Eric Salama (1):
  BUILD/MINOR: ssl: fix compiler warning about useless statement

Joao Morais (1):
  BUG/MINOR: config: Update cookie domain warn to RFC6265

William Dauchy (1):
  MINOR: tcp: avoid confusion in time parsing init

William Lallemand (1):
  BUG/MINOR: cli: don't call the kw->io_release if kw->parse failed

Willy Tarreau (6):
  BUG/MEDIUM: listeners: always pause a listener on out-of-resource 
condition
  MINOR: ist: add ist_find_ctl()
  BUG/MAJOR: h2: reject header values containing invalid chars
  BUG/MAJOR: h2: make header field name filtering stronger
  SCRIPTS: create-release: show the correct origin name in suggested 
commands
  SCRIPTS: git-show-backports: add "-s" to proposed cherry-pick commands

---



[ANNOUNCE] haproxy-1.9.13

2019-11-25 Thread Willy Tarreau
Hi,

HAProxy 1.9.13 was released on 2019/11/25. It added 39 new commits
after version 1.9.12.

It addresses the same security issues as announced in 2.0.10:

- The first one, found by Tim Düsterhus, lets an attacker pass control
  characters into header fields, leading to a possibility of content
  smuggling attacks on HTTP/1 backends, which is mainly a concern if
  http-reuse is in use.

- The second, found by Christopher Faulet, is a direct consequence of a
  flaw in the H2 spec making no special case of HEADER frames received
  on an IDLE stream on the response path. As such, such a frame passes
  all validity checks but no stream is allocated since it's a response,
  and the decoding of the headers on a read-only dummy stream results
  in a crash of the process.

It also addresses a number of issues which were already fixed in 2.0.9,
such as an occasional risk of double free causing crashes in idle
connections, disabling splicing on chunked responses, listeners eating
CPU when reaching the frontend/process' maxconn, and improved handling
of idle connections facing errors.

Please find the usual URLs below :
   Site index   : http://www.haproxy.org/
   Discourse: http://discourse.haproxy.org/
   Slack channel: https://slack.haproxy.org/
   Issue tracker: https://github.com/haproxy/haproxy/issues
   Sources  : http://www.haproxy.org/download/1.9/src/
   Git repository   : http://git.haproxy.org/git/haproxy-1.9.git/
   Git Web browsing : http://git.haproxy.org/?p=haproxy-1.9.git
   Changelog: http://www.haproxy.org/download/1.9/src/CHANGELOG
   Cyril's HTML doc : http://cbonte.github.io/haproxy-dconv/

Willy
---
Complete changelog :
Baptiste Assmann (1):
  BUG: dns: timeout resolve not applied for valid resolutions

Christopher Faulet (10):
  BUG/MAJOR: stream-int: Don't receive data from mux until SI_ST_EST is 
reached
  BUG/MEDIUM: mux-h1: Disable splicing for chunked messages
  BUG/MEDIUM: stream: Be sure to support splicing at the mux level to 
enable it
  BUG/MEDIUM: stream: Be sure to release allocated captures for TCP streams
  BUG/MEDIUM: filters: Don't call TCP callbacks for HTX streams
  BUG/MINOR: mux-h1: Don't set CS_FL_EOS on a read0 when receiving data to 
pipe
  BUG/MEDIUM: stream-int: Don't loose events on the CS when an EOS is 
reported
  BUG/MINOR: mux-h1: Fix tunnel mode detection on the response path
  BUG/MINOR: stream-int: Fix si_cs_recv() return value
  DOC: Add documentation about the use-service action

Emmanuel Hocdet (1):
  BUG/MINOR: ssl: fix crt-list neg filter for openssl < 1.1.1

Eric Salama (1):
  BUILD/MINOR: ssl: fix compiler warning about useless statement

Joao Morais (1):
  BUG/MINOR: config: Update cookie domain warn to RFC6265

Jérôme Magnin (2):
  DOC: management: document reuse and connect counters in the CSV format
  DOC: management: document cache_hits and cache_lookups in the CSV format

Lukas Tribus (1):
  MINOR: doc: http-reuse connection pool fix

Olivier Houchard (3):
  MINOR: mux: Add a new method to get informations about a mux.
  BUG/MEDIUM: servers: Only set SF_SRV_REUSED if the connection if fully 
ready.
  BUG/MEDIUM: Make sure we leave the session list in session_free().

William Dauchy (1):
  MINOR: tcp: avoid confusion in time parsing init

William Lallemand (2):
  BUG/MINOR: cli: don't call the kw->io_release if kw->parse failed
  BUG/MINOR: cli: fix out of bounds in -S parser

Willy Tarreau (16):
  MINOR: config: warn on presence of "\n" in header values/replacements
  BUG/MINOR: mux-h2: do not emit logs on backend connections
  BUG/MINOR: spoe: fix off-by-one length in UUID format string
  BUG/MEDIUM: mux-h2: report no available stream on a connection having 
errors
  BUG/MEDIUM: mux-h2: immediately remove a failed connection from the idle 
list
  BUG/MEDIUM: mux-h2: immediately report connection errors on streams
  DOC: management: fix typo on "cache_lookups" stats output
  BUG/MINOR: queue/threads: make the queue unlinking atomic
  BUG/MEDIUM: listeners: always pause a listener on out-of-resource 
condition
  BUG/MINOR: log: limit the size of the startup-logs
  MINOR: ist: add ist_find_ctl()
  BUG/MAJOR: h2: reject header values containing invalid chars
  BUG/MAJOR: h2: make header field name filtering stronger
  BUG/MAJOR: mux-h2: don't try to decode a response HEADERS frame in idle 
state
  SCRIPTS: create-release: show the correct origin name in suggested 
commands
  SCRIPTS: git-show-backports: add "-s" to proposed cherry-pick commands

---



[ANNOUNCE] haproxy-2.0.10

2019-11-25 Thread Willy Tarreau
Hi,

HAProxy 2.0.10 was released on 2019/11/25. It added 37 new commits
after version 2.0.9.

This version addresses two potential security issues in the H2 decoder.

The first one, found by Tim Düsterhus, lets an attacker pass control
characters into header fields, leading to a possibility of content
smuggling attacks on HTTP/1 backends, which is mainly a concern if
http-reuse is in use.

The second, found by Christopher Faulet, is a direct consequence of a
flaw in the H2 spec making no special case of HEADER frames received
on an IDLE stream on the response path. As such, such a frame passes
all validity checks but no stream is allocated since it's a response,
and the decoding of the headers on a read-only dummy stream results
in a crash of the process.

New versions of 1.9 and 1.8 will be issued to fix these flaws as well
(1.8 is only affected by the first one).

A few other issues were addressed, such as certain cases of server errors
being reported while the client closed first, and some peers
desynchronization issues.

At the HAProxyConf, a few people asked for the "h1-case-adjust" feature
to be backported to help them fix bogus applications and smoothly
transition to HTX. Indeed, since HTX, header field names are lower cased
(as is the case in HTTP/2) and it was reported that a few decades-old
application still living in field incorrectly expect various CaMeLCaSe.
As 2.1 dropped support for legacy mode it's not convenient for users
to quickly switch between one mode and the other when trying to work
around problems. With this patch backported into 2.0, it now becomes
easier to address one application at a time using h1-case-adjust and
h1-case-adjust-file and only switch once all applications work in HTX.

Please find the usual URLs below :
   Site index   : http://www.haproxy.org/
   Discourse: http://discourse.haproxy.org/
   Slack channel: https://slack.haproxy.org/
   Issue tracker: https://github.com/haproxy/haproxy/issues
   Sources  : http://www.haproxy.org/download/2.0/src/
   Git repository   : http://git.haproxy.org/git/haproxy-2.0.git/
   Git Web browsing : http://git.haproxy.org/?p=haproxy-2.0.git
   Changelog: http://www.haproxy.org/download/2.0/src/CHANGELOG
   Cyril's HTML doc : http://cbonte.github.io/haproxy-dconv/

Willy
---
Complete changelog :
Christopher Faulet (19):
  BUG/MEDIUM: stream-int: Don't loose events on the CS when an EOS is 
reported
  BUILD: debug: Avoid warnings in dev mode with -02 because of some BUG_ON 
tests
  BUG/MINOR: mux-h1: Fix tunnel mode detection on the response path
  BUG/MINOR: http-ana: Properly catch aborts during the payload forwarding
  MINOR: freq_ctr: Make the sliding window sums thread-safe
  MINOR: stream: Remove the lock on the proxy to update time stats
  MINOR: counters: Add fields to store the max observed for {q,c,d,t}_time
  MINOR: contrib/prometheus-exporter: Report metrics about max times for 
sessions
  BUG/MINOR: contrib/prometheus-exporter: Rename some metrics
  MINOR: contrib/prometheus-exporter: report the number of idle conns per 
server
  MINOR: contrib/prometheus-exporter: filter exported metrics by scope
  MINOR: contrib/prometheus-exporter: Add a param to ignore servers in 
maintenance
  BUG/MINOR: stream-int: Fix si_cs_recv() return value
  MINOR: stats: Report max times in addition of the averages for sessions
  MEDIUM: mux-h1: Add the support of headers adjustment for bogus HTTP/1 
apps
  BUG/MINOR: mux-h1: Fix a UAF in cfg_h1_headers_case_adjust_postparser()
  BUG/MINOR: mux-h1: Adjust header case when chunked encoding is add to a 
message
  DOC: Add missing stats fields in the management manual
  DOC: Add documentation about the use-service action

Emmanuel Hocdet (1):
  BUG/MINOR: ssl: fix crt-list neg filter for openssl < 1.1.1

Eric Salama (1):
  BUILD/MINOR: ssl: fix compiler warning about useless statement

Frédéric Lécaille (5):
  MINOR: peers: Alway show the table info for disconnected peers.
  MINOR: peers: Add TX/RX heartbeat counters.
  MINOR: peers: Add debugging information to "show peers".
  BUG/MINOR: peers: Wrong null "server_name" data field handling.
  BUG/MINOR: peers: "peer alive" flag not reset when deconnecting.

Jerome Magnin (1):
  REGTEST: vtest can now enable mcli with its own flag

Lukas Tribus (1):
  BUG/MINOR: ssl: fix curve setup with LibreSSL

William Dauchy (1):
  BUG/MINOR: init: fix set-dumpable when using uid/gid

William Lallemand (2):
  BUG/MEDIUM: mworker: don't fill the -sf argument with -1 during the reexec
  BUG/MINOR: cli: fix out of bounds in -S parser

Willy Tarreau (6):
  MINOR: ist: add ist_find_ctl()
  BUG/MAJOR: h2: reject header values containing invalid chars
  BUG/MAJOR: h2: make header field name filtering stronger
  BUG/MAJOR: mux-h2: don't try to decode a response HEADERS frame in idle 
state

Re: [PATCH] MINOR: contrib/prometheus-exporter: decode parameter and value only

2019-11-25 Thread Christopher Faulet

Le 23/11/2019 à 20:38, William Dauchy a écrit :

we were decoding all substring and then parsing; this could lead to
consider & and = in decoding result as delimiters where it should not.
this patch reverses the order by first parsing and then decoding each key
and value separately.

This patch should be backported to 2.0



Hi William,

Many thanks. I found some issues.

First, a key without value is not properly handled. You must not try to parse 
the value, otherwise the following parameter is read as value. For instance 
"/metrics?no-maint=server".


Then, if empty keys are forbidden (and it seems to be reasonable), you should 
throw an error if the delimiter is an equal sign (so a value with an empty key), 
 or, at least, you should skip the value. For instance, "/metrics?=value" or 
"/metrics?k1=v1&=v2". Another way to catch this case is to consider a key 
without value as equivalent to a value with an empty key. This way 
"/metrics?no-maint" could be equivalent to "/metrics?=no-maint". Your choice :) 
But be careful to make a difference between a key without value ("?a") and a key 
with an empty value ("?a=").


The equal sign should probably be forbidden in a value (before decoding). For 
instance, "/metrics?k1=val=ue" or "/metrics?k1==v1".


Finally, it could be good to stop the parsing on the number sign (#). To not 
parse the fragment part of the uri, if any. The current version is also affected 
by this issue.


--
Christopher Faulet



Re: [PATCH v2] CLEANUP: ssl: check if a transaction exists once before setting it

2019-11-25 Thread William Lallemand
On Sun, Nov 24, 2019 at 03:04:20PM +0100, William Dauchy wrote:
> trivial patch to fix issue #351
> 
> Fixes: bc6ca7ccaa72 ("MINOR: ssl/cli: rework 'set ssl cert' as 'set/commit'")
> Reported-by: Илья Шипицин 
> Signed-off-by: William Dauchy 
> ---
>  src/ssl_sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> index bcfa3e71..0848dc7c 100644
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -10468,7 +10468,7 @@ static int cli_parse_set_cert(char **args, char 
> *payload, struct appctx *appctx,
>   /* we succeed, we can save the ckchs in the transaction */
>  
>   /* if there wasn't a transaction, update the old ckchs */
> - if (!ckchs_transaction.old_ckchs && !ckchs_transaction.old_ckchs) {
> + if (!ckchs_transaction.old_ckchs) {
>   ckchs_transaction.old_ckchs = appctx->ctx.ssl.old_ckchs;
>   ckchs_transaction.path = appctx->ctx.ssl.path;
>   err = memprintf(, "Transaction created for certificate 
> %s!\n", ckchs_transaction.path);
> -- 
> 2.24.0
> 

Thanks, merged!

-- 
William Lallemand