There's a very common openssl patch on the net meant to significantly
reduce openssl's memory usage. This patch has been provided for many
versions now, and it makes sense to add support for it given that it
is very simple. It only requires to add an extra SSL_MODE flag. Just
like for other flags, if the flag is unknown, it's unset. About 44kB
of memory may be saved per SSL session with the patch.
When memory becomes scarce and openssl refuses to allocate a new SSL
session, it is worth freeing the pools and trying again instead of
rejecting all incoming SSL connection. This can happen when some
memory usage limits have been assigned to the haproxy process using
-m or with ulimit -m/-v.
This is mostly an enhancement of previous fix and is worth backporting
to 1.5.
Some SSL context's init functions errors were not handled and
can cause a segfault due to an incomplete SSL context
initialization.
This fix must be backported to 1.5.
HAProxy will crash with the following configuration:
global
...
tune.bufsize 1024
tune.maxrewrite 0
frontend xxx
...
backend yyy
...
cookie cookie insert maxidle 300s
If client sends a request of which object size is more than tune.bufsize (1024
bytes), HAProxy will crash.
After doing some debugging, the crash was caused by http_header_add_tail2() ->
buffer_insert_line2() while inserting cookie at the end of response header.
Part codes of buffer_insert_line2() are as below:
int buffer_insert_line2(struct buffer *b, char *pos, const char *str, int len)
{
int delta;
delta = len + 2;
if (bi_end(b) + delta >= b->data + b->size)
return 0; /* no space left */
/* first, protect the end of the buffer */
memmove(pos + delta, pos, bi_end(b) - pos);
...
}
Since tune.maxrewrite is 0, HAProxy can receive 1024 bytes once which is equals
to full buffer size. Under such condition, the buffer is full and bi_end(b)
will be wrapped to the start of buffer which pointed to b->data. As a result,
though there is no space left in buffer, the check condition
if (bi_end(b) + delta >= b->data + b->size)
will be true, then memmove() is called, and (pos + delta) will exceed the end
of buffer (b->data + b->size), HAProxy crashes
Just take buffer_replace2() as a reference, the other check when input data in
a buffer is wrapped should be also added into buffer_insert_line2().
This fix must be backported to 1.5.
Signed-off-by: Godbach <nylzhaowei@gmail.com>
Commit 0bed994 ("BUG/MINOR: ssl: correctly initialize ssl ctx for
invalid certificates") accidently left a change in the Makefile
resulting in -ldl being appended to the LDFLAGS. As reported by
Dmitry Sivachenko, this will break build on systems without libdl
such as FreeBSD.
This fix must be backported to 1.5.
Bug reported by John Leach: no-sslv3 does not work using some certificates.
It appears that ssl ctx is not updated with configured options if the
CommonName of the certificate's subject is not found.
It applies only on the first cerificate of a configured bind line.
There is no security impact, because only invalid nameless certficates
are concerned.
This fix must be backported to 1.5
Adds global statements 'ssl-default-server-options' and
'ssl-default-bind-options' to force on 'server' and 'bind' lines
some ssl options.
Currently available options are 'no-sslv3', 'no-tlsv10', 'no-tlsv11',
'no-tlsv12', 'force-sslv3', 'force-tlsv10', 'force-tlsv11',
'force-tlsv12', and 'no-tls-tickets'.
Example:
global
ssl-default-server-options no-sslv3
ssl-default-bind-options no-sslv3
There's an issue when using SO_ORIGINAL_DST to retrieve the original
destination of a connection's address before being translated by
Netfilter's DNAT/REDIRECT or the old TPROXY. SO_ORIGINAL_DST is
able to retrieve an IPv4 address when the original destination was
IPv4 mapped into IPv6. At first glance it's not a big deal, but it
is for logging and for the proxy protocol, because we then have
two different address families for the source and destination. In
this case, the proxy protocol correctly detects the issue and emits
"UNKNOWN".
In order to fix this, we perform getsockname() first, and only if
the address family is AF_INET, then we perform the getsockopt() call.
This fix must be backported to 1.5, and probably even to 1.4 and 1.3.
ssl_c_der : binary
Returns the DER formatted certificate presented by the client when the
incoming connection was made over an SSL/TLS transport layer. When used for
an ACL, the value(s) to match against can be passed in hexadecimal form.
ssl_f_der : binary
Returns the DER formatted certificate presented by the frontend when the
incoming connection was made over an SSL/TLS transport layer. When used for
an ACL, the value(s) to match against can be passed in hexadecimal form.
pcre_study() may return NULL even though it succeeded. In this case error is
NULL otherwise error is not NULL. Also see man 3 pcre_study.
Previously a ACL pattern of e.g. ".*" would cause error because pcre_study did
not found anything to speed up matching and returned regex->extra = NULL and
error = NULL which in this case was a false-positive. That happend only when
PCRE_JIT was enabled for HAProxy but libpcre has been built without JIT.
Signed-off-by: Christian Ruppert <c.ruppert@babiel.com>
[wt: this needs to be backported to 1.5 as well]
When building on openssl-0.9.8, since commit 23d5d37 ("MINOR: ssl: use
SSL_get_ciphers() instead of directly accessing the cipher list.") we get
the following warning :
src/ssl_sock.c: In function 'ssl_sock_prepare_ctx':
src/ssl_sock.c:1592: warning: passing argument 1 of 'SSL_CIPHER_description' discards qualifiers from pointer target type
This is because the openssl API has changed between 0.9.8 and 1.0.1 :
0.9.8: char *SSL_CIPHER_description(SSL_CIPHER *,char *buf,int size);
1.0.1: char *SSL_CIPHER_description(const SSL_CIPHER *,char *buf,int size);
So let's remove the "const" type qualifier to satisfy both versions.
Note that the fix above was backported to 1.5, so this one should as well.
This converter escapes string to use it as json/ascii escaped string.
It can read UTF-8 with differents behavior on errors and encode it in
json/ascii.
json([<input-code>])
Escapes the input string and produces an ASCII ouput string ready to use as a
JSON string. The converter tries to decode the input string according to the
<input-code> parameter. It can be "ascii", "utf8", "utf8s", "utf8"" or
"utf8ps". The "ascii" decoder never fails. The "utf8" decoder detects 3 types
of errors:
- bad UTF-8 sequence (lone continuation byte, bad number of continuation
bytes, ...)
- invalid range (the decoded value is within a UTF-8 prohibited range),
- code overlong (the value is encoded with more bytes than necessary).
The UTF-8 JSON encoding can produce a "too long value" error when the UTF-8
character is greater than 0xffff because the JSON string escape specification
only authorizes 4 hex digits for the value encoding. The UTF-8 decoder exists
in 4 variants designated by a combination of two suffix letters : "p" for
"permissive" and "s" for "silently ignore". The behaviors of the decoders
are :
- "ascii" : never fails ;
- "utf8" : fails on any detected errors ;
- "utf8s" : never fails, but removes characters corresponding to errors ;
- "utf8p" : accepts and fixes the overlong errors, but fails on any other
error ;
- "utf8ps" : never fails, accepts and fixes the overlong errors, but removes
characters corresponding to the other errors.
This converter is particularly useful for building properly escaped JSON for
logging to servers which consume JSON-formated traffic logs.
Example:
capture request header user-agent len 150
capture request header Host len 15
log-format {"ip":"%[src]","user-agent":"%[capture.req.hdr(1),json]"}
Input request from client 127.0.0.1:
GET / HTTP/1.0
User-Agent: Very "Ugly" UA 1/2
Output log:
{"ip":"127.0.0.1","user-agent":"Very \"Ugly\" UA 1\/2"}
During a tcp connection setup in tcp_connect_server(), we check if
there are pending data to start polling for writes immediately. We
also use the same test to know if we can disable the quick ack and
merge the first data packet with the connection's ACK. This last
case is also valid for the proxy protocol.
The problem lies in the way it's done, as the "data" variable is
improperly completed with the presence of the proxy protocol, resulting
in the connection being polled for data writes if the proxy protocol is
enabled. It's not a big issue per se, except that the proxy protocol
uses the fact that we're polling for data to know if it can use MSG_MORE.
This causes no problem on HTTP/HTTPS, but with banner protocols, it
introduces a 200ms delay if the server waits for the PROXY header.
This has been caused by the connection management changes introduced in
1.5-dev12, specifically commit a1a7474 ("MEDIUM: proxy-proto: don't use
buffer flags in conn_si_send_proxy()"), so this fix must be backported
to 1.5.
Colin Ingarfield reported some unexplainable flags in the logs.
For example, a "LR" termination state was set on a request which was forwarded
to a server, where "LR" means that the request should have been handled
internally by haproxy.
This case happens when at least client side keep-alive is enabled. Next
requests in the connection will inherit the flags from the previous request.
2 fields are impacted : "termination_state" and "Tt" in the timing events,
where a "+" can be added, when a previous request was redispatched.
This is not critical for the service itself but can confuse troubleshooting.
The fix must be backported to 1.5 and 1.4.
Dmitry Sivachenko reported an embarrassing problem where haproxy
would sometimes segfault upon reload. After careful analysis and
code inspection, what happens is related to the "show sess" command
on the CLI, and it is not limited to reload operations only.
When a "show sess" is running, once the output buffer is full, the
stats applet grabs a reference to the session being dumped in order
for the current pointer to be able to advance by itself should this
session disappear while the buffer is full. The applet also uses a
release handler that is called when the applet terminates to release
such references.
The problem is that upon error, the command line parser sets the
applet state to STAT_CLI_O_END indicating it wants to terminate the
processing. Unfortunately, the release handler which is called later
to clean everything up relies on the applet's state to know what
operations were in progress, and as such it does not release the
reference. A later "show sess" or the completion of the task being
watched lead to a LIST_DEL() on the task's list which point to a
location that does not match the applet's reference list anymore
and the process dies.
One solution to this would be to add a flag to the current applet's
state mentionning it must leave, without affecting the state indicating
the current operation. It's a bit invasive but could be the long term
solution. The short term solution simply consists in calling the
release handler just before changing the state to STAT_CLI_O_END.
That way everything that must be released is released in time.
Note that the probability to encounter this issue is very low.
It requires a lot of "show sess" or "show sess all" calls, and
that one of them dies before being completed. That can happen
if "show sess" is run in scripts which truncate the output (eg:
"echo show sess|socat|head"). This could be the worst case as it
almost ensures that haproxy fills a buffer, grabs a reference and
detects the error on the socket.
There's no config-based workaround to this issue, except refraining
from issuing "show sess" on large connection counts or "show sess all".
If that's not possible to block everyone, restricting permissions on
the stats socket ensures only authorized tools can connect.
This fix must be backported to 1.5 and to 1.4 (with some changes in
1.4 since the release function does not exist so the LIST_DEL sequence
must be open-coded).
Special thanks to Dmitry for the fairly complete report.
When the HTTP parser is in state HTTP_MSG_ERROR, we don't know if it was
already initialized or not. If the error happens before HTTP_MSG_RQBEFORE,
random offsets might be present and we don't want to display such random
strings in debug mode.
While it's theorically possible to randomly crash the process when running
in debug mode here, this bug was not tagged MAJOR because it would not
make sense to run in debug mode in production.
This fix must be backported to 1.5 and 1.4.
Commit 98634f0 ("MEDIUM: backend: Enhance hash-type directive with an
algorithm options") cleaned up the hashing code by using a centralized
function. A bug appeared in get_server_uh() which is the URI hashing
function. Prior to the patch, the function would stop hashing on the
question mark, or on the trailing slash of a maximum directory count.
Consecutive to the patch, this last character is included into the
hash computation. This means that :
GET /0
GET /0?
Are not hashed similarly. The following configuration reproduces it :
mode http
balance uri
server s1 0.0.0.0:1234 redir /s1
server s2 0.0.0.0:1234 redir /s2
Many thanks to Vedran Furac for reporting this issue. The fix must
be backported to 1.5.
MAX_SESS_STKCTR allows one to define the number of stick counters that can
be used in parallel in track-sc* rules. The naming of this macro creates
some confusion because the value there is sometimes used as a max instead
of a count, and the config parser accepts values from 0 to MAX_SESS_STKCTR
and the processing ignores anything tracked on the last one. This means
that by default, track-sc3 is allowed and ignored.
This fix must be backported to 1.5 where the problem there only affects
TCP rules.
Paul Taylor and Bryan Talbot found that after commit 419ead8 ("MEDIUM:
config: compute the exact bind-process before listener's maxaccept"),
a backend marked "disabled" would cause the next backend to be skipped
and if it was the last one it would cause a segfault.
The reason is that the commit above changed the "while" loop for a "for"
loop but a "continue" statement still incrementing the current proxy was
left in the code for disabled proxies, causing the next one to be skipped
as well and the last one to try to dereference NULL when seeking ->next.
The quick workaround consists in not disabling backends, or adding an
empty dummy one after a disabled section.
This fix must be backported to 1.5.
By default systemd will send SIGTERM to all processes in the service's
control group. In our case, this includes the wrapper, the master
process and all worker processes.
Since commit c54bdd2a the wrapper actually catches SIGTERM and survives
to see the master process getting killed by systemd and regard this as
an error, placing the unit in a failed state during "systemctl stop".
Since the wrapper now handles SIGTERM by itself, we switch the kill mode
to 'mixed', which means that systemd will deliver the initial SIGTERM to
the wrapper only, and if the actual haproxy processes don't exit after a
given amount of time (default: 90s), a SIGKILL is sent to all remaining
processes in the control group. See systemd.kill(5) for more
information.
This should also be backported to 1.5.
A segfault was reported with the introduction of the propagate_processes()
function. It was caused when a use_backend rule was declared with a dynamic
name, using a log-format string. The backend is not resolved during the
configuration, which lead to the segfault.
The patch prevents the process binding propagation for such dynamic rules, it
should also be backported to 1.5.
The step number was reported by checking only last_started_step, which
was not set in case of error during the initial connection phase, and
caused "step 1" to be returned with an invalid check type (typically
SEND). So now we first verify that a test was started before returning
this.
In addition to this, the indication of the test type was taken from
current_step instead of last_started_step, so the error description
was matching the next action instead of the one reported in the step
ID. Thus we could get the confusing "step 1 (send)" report below :
tcp-check connect
tcp-check send foo
In order to ease debugging, when the port number is known for a connect,
it is indicated in the error report.
Note that this only affects asynchronous error messages, synchronous ones
are correct.
This fix must be backported to 1.5.
When "option tcp-check" is specified without any tcp-check rules, the
documentation says that it's the same as the default check method. But
the code path is a bit different, and we used to consider that since
the end of rules was reached, the check is always successful regardless
of the connection status.
This patch reorganizes the error detection, and considers the special
case where there's no tcp-check rule as a real L4 check. It also avoids
dereferencing the rule list head as a rule by itself.
While fixing this bug, another one related to the output messages'
accuracy was noticed, it will be fixed in a separate commit and is
much less important.
This bug is also present in 1.5, so this fix must be backported.
propagate_processes() must not be called with unresolved proxies, but
nothing prevents it from being called in check_config_validity(). The
resulting effect is that an unresolved proxy can cause a recursion
loop if called in such a situation, ending with a segfault after the
fatal error report. There's no side effect beyond this.
This patch refrains from calling the function when any error was met.
This bug also affects 1.5, it should be backported.
Commit 179085c ("MEDIUM: http: move Connection header processing earlier")
introduced a regression : the backend's HTTP mode is not considered anymore
when setting the session's HTTP mode, because wait_for_request() is only
called once, when the frontend receives the request (or when the frontend
is in TCP mode, when the backend receives the request).
The net effect is that in some situations when the frontend and the backend
do not work in the same mode (eg: keep-alive vs close), the backend's mode
is ignored.
This patch moves all that processing to a dedicated function, which is
called from the original place, as well as from session_set_backend()
when switching from an HTTP frontend to an HTTP backend in different
modes.
This fix must be backported to 1.5.
Kristoffer Grönlund reported that after my recent update to the
systemd-wrapper, I accidentely left the debugging code which
consists in disabling the fork :-(
The fix needs to be backported to 1.5 as well since I pushed it
there as well.
Having to use a hard-coded "haproxy" executable name next to the systemd
wrapper is not always convenient, as it's sometimes desirable to run with
multiple versions in parallel.
Thus this patch performs a minor change to the wrapper : if the name ends
with "-systemd-wrapper", then it trims that part off and what remains
becomes the target haproxy executable. That makes it easy to have for
example :
haproxy-1.5.4-systemd-wrapper haproxy-1.5.4
haproxy-1.5.3-systemd-wrapper haproxy-1.5.3
and so on, in a same directory.
This patch also fixes a rare bug caused by readlink() not adding the
trailing zero and leaving possible existing contents, including possibly
a randomly placed "/" which would make it unable to locate the correct
binary. This case is not totally unlikely as I got a \177 a few times
at the end of the executable names, so I could have got a '/' as well.
Back-porting to 1.5 is desirable.
If a frontend has any tcp-request content rule relying on request contents
without any inspect delay, we now emit a warning as this will randomly match.
This can be backported to 1.5 as it reduces the support effort.
Since commit 1b71eb5 ("BUG/MEDIUM: counters: fix track-sc* to wait on
unstable contents"), we don't need the "if HTTP" anymore. But the doc
was not updated to reflect this.
Since this change was backported to 1.5, this doc update should be
backported as well.
A config where a tcp-request rule appears after an http-request rule
might seem valid but it is not. So let's report a warning about this
since this case is hard to detect by the naked eye.
Some users want to have a stats frontend with one line per process, but while
100% valid and safe, the config parser emits a warning. Relax this check to
ensure that the warning is only emitted if at least one of the listeners is
bound to multiple processes, or if the directive is placed in a backend called
from multiple processes (since in this case we don't know if it's safe).
This is a continuation of previous patch, the listener's maxaccept is divided
by the number of processes, so it's best if we can swap the two blocks so that
the number of processes is already known when computing the maxaccept value.
When a frontend does not have any bind-process directive, make it
automatically bind to the union of all of its listeners' processes
instead of binding to all processes. That will make it possible to
have the expected behaviour without having to explicitly specify a
bind-process directive.
Note that if the listeners are not bound to a specific process, the
default is still to bind to all processes.
This change could be backported to 1.5 as it simplifies process
management, and was planned to be done during the 1.5 development phase.
We now recursively propagate the bind-process values between frontends
and backends instead of doing it during name resolving. This ensures
that we're able to properly propagate all the bind-process directives
even across "listen" instances, which are not perfectly covered at the
moment, depending on the declaration order.
This basically reverts 3507d5d ("MEDIUM: proxy: only adjust the backend's
bind-process when already set"). It was needed during the transition to
the new process binding method but is causing trouble now because frontend
to backend binding is not properly propagated.
This fix should be backported to 1.5.
Ryan Brock reported that server stickiness did not work for WebSocket
because the cookies and headers are not modified on 1xx responses. He
found that his browser correctly presents the cookies learned on 101
responses, which was not specifically defined in the WebSocket spec,
nor in the cookie spec. 101 is a very special case. Being part of 1xx,
it's an interim response. But within 1xx, it's special because it's
the last HTTP/1 response that transits on the wire, which is different
from 100 or 102 which may appear multiple times. So in that sense, we
can consider it as a final response regarding HTTP/1, and it makes
sense to allow header processing there. Note that we still ensure not
to mangle the Connection header, which is critical for HTTP upgrade to
continue to work smoothly with agents that are a bit picky about what
tokens are found there.
The rspadd rules are now processed for 101 responses as well, but the
cache-control checks are not performed (since no body is delivered).
Ryan confirmed that this patch works for him.
It would make sense to backport it to 1.5 given that it improves end
user experience on WebSocket servers.
My proposal is to let haproxy-systemd-wrapper also accept normal
SIGHUP/SIGTERM signals to play nicely with other process managers
besides just systemd. In my use case, this will be for using with
runit which has to ability to change the signal used for a
"reload" or "stop" command. It also might be worth renaming this
bin to just haproxy-wrapper or something of that sort to separate
itself away from systemd. But that's a different discussion. :)
Commit bb2e669 ("BUG/MAJOR: http: correctly rewind the request body
after start of forwarding") was incorrect/incomplete. It used to rely on
CF_READ_ATTACHED to stop updating msg->sov once data start to leave the
buffer, but this is unreliable because since commit a6eebb3 ("[BUG]
session: clear BF_READ_ATTACHED before next I/O") merged in 1.5-dev1,
this flag is only ephemeral and is cleared once all analysers have
seen it. So we can start updating msg->sov again each time we pass
through this place with new data. With a sufficiently large amount of
data, it is possible to make msg->sov wrap and validate the if()
condition at the top, causing the buffer to advance by about 2GB and
crash the process.
Note that the offset cannot be controlled by the attacker because it is
a sum of millions of small random sizes depending on how many bytes were
read by the server and how many were left in the buffer, only because
of the speed difference between reading and writing. Also, nothing is
written, the invalid pointer resulting from this operation is only read.
Many thanks to James Dempsey for reporting this bug and to Chris Forbes for
narrowing down the faulty area enough to make its root cause analysable.
This fix must be backported to haproxy 1.5.
When an unknown encryption algorithm is used in userlists or the password is
not pasted correctly in the configuration, http authentication silently fails.
An initial check is now performed during the configuration parsing, in order to
verify that the encrypted password is supported. An unsupported password will
fail with a fatal error.
This patch should be backported to 1.4 and 1.5.
Grégoire Morpain reported a segfault when a secured password is used for http
authentication. It was caused by the use of an unsupported encryption algorithm
with libcrypto. In this case, crypt() returns a NULL pointer.
The fix should be backported to 1.4 and 1.5.
This code aims at clearing up the ACL parsing code a bit to make it
more obvious what happens in the case of an ACL keyword and what happens
in the case of a sample expression. Variables prev_type and cur_type were
merged, and ACL keyword processing functions are checked only once.
This patch could be backported into 1.5 after the previous patch, in order
to keep the code more maintainable.
