Merge ~paelzer/ubuntu/+source/haproxy:bug-1841936-openssl1.1.1-tlsv13-bionic into ubuntu/+source/haproxy:ubuntu/bionic-devel

Proposed by Christian Ehrhardt 
Status: Merged
Approved by: Christian Ehrhardt 
Approved revision: ac2e0a36785c8c3a90002c99dac8133b83d41547
Merged at revision: 47a2ee069c0320811815a800661928c84d4ec41b
Proposed branch: ~paelzer/ubuntu/+source/haproxy:bug-1841936-openssl1.1.1-tlsv13-bionic
Merge into: ubuntu/+source/haproxy:ubuntu/bionic-devel
Diff against target: 294 lines (+266/-0) (has conflicts)
4 files modified
debian/changelog (+12/-0)
debian/patches/lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch (+134/-0)
debian/patches/lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch (+118/-0)
debian/patches/series (+2/-0)
Conflict in debian/changelog
Reviewer Review Type Date Requested Status
Robie Basak Approve
Canonical Server Pending
git-ubuntu developers Pending
Review via email: mp+374589@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Revision history for this message
Robie Basak (racb) wrote :

lgtm.

"Conflict in debian/changelog" is a false positive - the commit you based this upload on is not the exact commit that git-ubuntu adopted, but the trees match.

review: Approve
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Tagged and uploaded

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 588b3b6..1e1c3ee 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,15 @@
6+<<<<<<< debian/changelog
7+=======
8+haproxy (1.8.8-1ubuntu0.6) bionic; urgency=medium
9+
10+ * Fix issues around dh_params when building against openssl 1.1.1
11+ to avoid regressing the minimal key size (LP: 1841936)
12+ - d/p/lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch
13+ - d/p/lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch
14+
15+ -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Wed, 23 Oct 2019 11:37:53 +0200
16+
17+>>>>>>> debian/changelog
18 haproxy (1.8.8-1ubuntu0.5) bionic; urgency=medium
19
20 * no change rebuild to pick up openssl 1.1.1 and via that
21diff --git a/debian/patches/lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch b/debian/patches/lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch
22new file mode 100644
23index 0000000..806b425
24--- /dev/null
25+++ b/debian/patches/lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch
26@@ -0,0 +1,134 @@
27+From 19dd0431b06019d5cbd253662822b15412f67144 Mon Sep 17 00:00:00 2001
28+From: Emeric Brun <ebrun@haproxy.com>
29+Date: Thu, 17 Oct 2019 14:53:03 +0200
30+Subject: [PATCH] BUG/MEDIUM: ssl: 'tune.ssl.default-dh-param' value ignored
31+ with openssl > 1.1.1
32+
33+If openssl 1.1.1 is used, c2aae74f0 commit mistakenly enables DH automatic
34+feature from openssl instead of ECDH automatic feature. There is no impact for
35+the ECDH one because the feature is always enabled for that version. But doing
36+this, the 'tune.ssl.default-dh-param' was completely ignored for DH parameters.
37+
38+This patch fix the bug calling 'SSL_CTX_set_ecdh_auto' instead of
39+'SSL_CTX_set_dh_auto'.
40+
41+Currently some users may use a 2048 DH bits parameter, thinking they're using a
42+1024 bits one. Doing this, they may experience performance issue on light hardware.
43+
44+This patch warns the user if haproxy fails to configure the given DH parameter.
45+In this case and if openssl version is > 1.1.0, haproxy will let openssl to
46+automatically choose a default DH parameter. For other openssl versions, the DH
47+ciphers won't be usable.
48+
49+A commonly case of failure is due to the security level of openssl.cnf
50+which could refuse a 1024 bits DH parameter for a 2048 bits key:
51+
52+ $ cat /etc/ssl/openssl.cnf
53+ ...
54+
55+ [system_default_sect]
56+ MinProtocol = TLSv1
57+ CipherString = DEFAULT@SECLEVEL=2
58+
59+This should be backport into any branch containing the commit c2aae74f0.
60+It requires all or part of the previous CLEANUP series.
61+
62+This addresses github issue #324.
63+
64+(cherry picked from commit 6624a90a9ac2edb947a8c70fa6a8a283449750c6)
65+Signed-off-by: Emeric Brun <ebrun@haproxy.com>
66+(cherry picked from commit d6de151248603b357565ae52fe92440e66c1177c)
67+Signed-off-by: Willy Tarreau <w@1wt.eu>
68+(cherry picked from commit 89e61b9ad4db1097a3d53b83d914f6b0b8edc4ee)
69+Signed-off-by: Willy Tarreau <w@1wt.eu>
70+
71+Origin: upstream, http://git.haproxy.org/?p=haproxy-1.8.git;a=commit;h=19dd0431b06019d5cbd253662822b15412f67144
72+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1841936
73+Last-Update: 2019-10-23
74+
75+---
76+ src/ssl_sock.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
77+ 1 file changed, 43 insertions(+), 4 deletions(-)
78+
79+diff --git a/src/ssl_sock.c b/src/ssl_sock.c
80+index 296e93864..773464043 100644
81+--- a/src/ssl_sock.c
82++++ b/src/ssl_sock.c
83+@@ -2651,7 +2651,20 @@ static int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file, char **err)
84+ DH *dh = ssl_sock_get_dh_from_file(file);
85+
86+ if (dh) {
87+- SSL_CTX_set_tmp_dh(ctx, dh);
88++ if (!SSL_CTX_set_tmp_dh(ctx, dh)) {
89++ memprintf(err, "%sunable to load the DH parameter specified in '%s'",
90++ err && *err ? *err : "", file);
91++#if defined(SSL_CTX_set_dh_auto)
92++ SSL_CTX_set_dh_auto(ctx, 1);
93++ memprintf(err, "%s, SSL library will use an automatically generated DH parameter.\n",
94++ err && *err ? *err : "");
95++#else
96++ memprintf(err, "%s, DH ciphers won't be available.\n",
97++ err && *err ? *err : "");
98++#endif
99++ ret |= ERR_WARN;
100++ goto end;
101++ }
102+
103+ if (ssl_dh_ptr_index >= 0) {
104+ /* store a pointer to the DH params to avoid complaining about
105+@@ -2660,7 +2673,20 @@ static int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file, char **err)
106+ }
107+ }
108+ else if (global_dh) {
109+- SSL_CTX_set_tmp_dh(ctx, global_dh);
110++ if (!SSL_CTX_set_tmp_dh(ctx, global_dh)) {
111++ memprintf(err, "%sunable to use the global DH parameter for certificate '%s'",
112++ err && *err ? *err : "", file);
113++#if defined(SSL_CTX_set_dh_auto)
114++ SSL_CTX_set_dh_auto(ctx, 1);
115++ memprintf(err, "%s, SSL library will use an automatically generated DH parameter.\n",
116++ err && *err ? *err : "");
117++#else
118++ memprintf(err, "%s, DH ciphers won't be available.\n",
119++ err && *err ? *err : "");
120++#endif
121++ ret |= ERR_WARN;
122++ goto end;
123++ }
124+ }
125+ else {
126+ /* Clear openssl global errors stack */
127+@@ -2678,7 +2704,20 @@ static int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file, char **err)
128+ goto end;
129+ }
130+
131+- SSL_CTX_set_tmp_dh(ctx, local_dh_1024);
132++ if (!SSL_CTX_set_tmp_dh(ctx, local_dh_1024)) {
133++ memprintf(err, "%sunable to load default 1024 bits DH parameter for certificate '%s'.\n",
134++ err && *err ? *err : "", file);
135++#if defined(SSL_CTX_set_dh_auto)
136++ SSL_CTX_set_dh_auto(ctx, 1);
137++ memprintf(err, "%s, SSL library will use an automatically generated DH parameter.\n",
138++ err && *err ? *err : "");
139++#else
140++ memprintf(err, "%s, DH ciphers won't be available.\n",
141++ err && *err ? *err : "");
142++#endif
143++ ret |= ERR_WARN;
144++ goto end;
145++ }
146+ }
147+ else {
148+ SSL_CTX_set_tmp_dh_callback(ctx, ssl_get_tmp_dh);
149+@@ -4381,7 +4420,7 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_
150+ NULL);
151+
152+ if (ecdhe == NULL) {
153+- SSL_CTX_set_dh_auto(ctx, 1);
154++ SSL_CTX_set_ecdh_auto(ctx, 1);
155+ return cfgerr;
156+ }
157+ #else
158+--
159+2.23.0
160+
161diff --git a/debian/patches/lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch b/debian/patches/lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch
162new file mode 100644
163index 0000000..18ebe1b
164--- /dev/null
165+++ b/debian/patches/lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch
166@@ -0,0 +1,118 @@
167+From b1e3ee6f214d82ebe98140f577777b4c47d88084 Mon Sep 17 00:00:00 2001
168+From: Emeric Brun <ebrun@haproxy.com>
169+Date: Thu, 17 Oct 2019 13:27:40 +0200
170+Subject: [PATCH] CLEANUP: ssl: make ssl_sock_load_dh_params handle
171+ errcode/warn
172+
173+ssl_sock_load_dh_params used to return >0 or -1 to indicate success
174+or failure. Make it return a set of ERR_* instead so that its callers
175+can transparently report its status. Given that its callers only used
176+to know about ERR_ALERT | ERR_FATAL, this is the only code returned
177+for now. An error message was added in the case of failure and the
178+comment was updated.
179+
180+(cherry picked from commit 7a88336cf83cd1592fb8e7bc456d72c00c2934e4)
181+Signed-off-by: Emeric Brun <ebrun@haproxy.com>
182+(cherry picked from commit cfc1afe9f21ec27612ed4ad84c4a066c68ca24af)
183+Signed-off-by: Willy Tarreau <w@1wt.eu>
184+(cherry picked from commit e740508a0dd5e30433a86333b874a0833f989e17)
185+[wt: context adjustments]
186+Signed-off-by: Willy Tarreau <w@1wt.eu>
187+
188+Backport-Note: context changes as we didn't backport further changes and
189+cleanups. Those affect the retval handling of a bunch of functions that
190+had to be adapted. We change ret of ssl_sock_load_dh_params as declared
191+by the patch but not the further one of ssl_sock_load_cert_file.
192+
193+Origin: backport, http://git.haproxy.org/?p=haproxy-1.8.git;a=commit;h=b1e3ee6f214d82ebe98140f577777b4c47d88084
194+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1841936
195+Last-Update: 2019-10-23
196+
197+---
198+ src/ssl_sock.c | 41 ++++++++++++++++++++++++++---------------
199+ 1 file changed, 26 insertions(+), 15 deletions(-)
200+
201+--- a/src/ssl_sock.c
202++++ b/src/ssl_sock.c
203+@@ -2610,15 +2610,24 @@ int ssl_sock_load_global_dh_param_from_f
204+ return -1;
205+ }
206+
207+-/* Loads Diffie-Hellman parameter from a file. Returns 1 if loaded, else -1
208+- if an error occured, and 0 if parameter not found. */
209+-int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
210++/* Loads Diffie-Hellman parameter from a ckchs to an SSL_CTX.
211++ * If there is no DH paramater availaible in the ckchs, the global
212++ * DH parameter is loaded into the SSL_CTX and if there is no
213++ * DH parameter available in ckchs nor in global, the default
214++ * DH parameters are applied on the SSL_CTX.
215++ * Returns a bitfield containing the flags:
216++ * ERR_FATAL in any fatal error case
217++ * ERR_ALERT if a reason of the error is availabine in err
218++ * ERR_WARN if a warning is available into err
219++ * The value 0 means there is no error nor warning and
220++ * the operation succeed.
221++ */
222++static int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file, char **err)
223+ {
224+- int ret = -1;
225++ int ret = 0;
226+ DH *dh = ssl_sock_get_dh_from_file(file);
227+
228+ if (dh) {
229+- ret = 1;
230+ SSL_CTX_set_tmp_dh(ctx, dh);
231+
232+ if (ssl_dh_ptr_index >= 0) {
233+@@ -2629,7 +2638,6 @@ int ssl_sock_load_dh_params(SSL_CTX *ctx
234+ }
235+ else if (global_dh) {
236+ SSL_CTX_set_tmp_dh(ctx, global_dh);
237+- ret = 0; /* DH params not found */
238+ }
239+ else {
240+ /* Clear openssl global errors stack */
241+@@ -2640,16 +2648,18 @@ int ssl_sock_load_dh_params(SSL_CTX *ctx
242+ if (local_dh_1024 == NULL)
243+ local_dh_1024 = ssl_get_dh_1024();
244+
245+- if (local_dh_1024 == NULL)
246++ if (local_dh_1024 == NULL) {
247++ memprintf(err, "%sunable to load default 1024 bits DH parameter for certificate '%s'.\n",
248++ err && *err ? *err : "", file);
249++ ret |= ERR_ALERT | ERR_FATAL;
250+ goto end;
251++ }
252+
253+ SSL_CTX_set_tmp_dh(ctx, local_dh_1024);
254+ }
255+ else {
256+ SSL_CTX_set_tmp_dh_callback(ctx, ssl_get_tmp_dh);
257+ }
258+-
259+- ret = 0; /* DH params not found */
260+ }
261+
262+ end:
263+@@ -3122,8 +3132,8 @@ static int ssl_sock_load_multi_cert(cons
264+ if (ssl_dh_ptr_index >= 0)
265+ SSL_CTX_set_ex_data(cur_ctx, ssl_dh_ptr_index, NULL);
266+
267+- rv = ssl_sock_load_dh_params(cur_ctx, NULL);
268+- if (rv < 0) {
269++ rv = ssl_sock_load_dh_params(cur_ctx, NULL, err);
270++ if (rv & ERR_CODE) {
271+ if (err)
272+ memprintf(err, "%sunable to load DH parameters from file '%s'.\n",
273+ *err ? *err : "", path);
274+@@ -3349,8 +3359,8 @@ static int ssl_sock_load_cert_file(const
275+ SSL_CTX_set_ex_data(ctx, ssl_dh_ptr_index, NULL);
276+ }
277+
278+- ret = ssl_sock_load_dh_params(ctx, path);
279+- if (ret < 0) {
280++ ret = ssl_sock_load_dh_params(ctx, path, err);
281++ if (ret & ERR_CODE) {
282+ if (err)
283+ memprintf(err, "%sunable to load DH parameters from file '%s'.\n",
284+ *err ? *err : "", path);
285diff --git a/debian/patches/series b/debian/patches/series
286index bcf915b..b3d5e76 100644
287--- a/debian/patches/series
288+++ b/debian/patches/series
289@@ -11,3 +11,5 @@ CVE-2018-20102.patch
290 CVE-2018-20103.patch
291 CVE-2018-20615.patch
292 stksess-align.patch
293+lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch
294+lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch

Subscribers

People subscribed via source and target branches