| Message ID | 20210525123604.2dc745b3@perso.pw |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#48648] gnu: gnutls: Update to 3.6.16 [fixes CVE-2021-20305]. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
On Tue, May 25, 2021 at 12:36:04PM +0200, Solene Rapenne via Guix-patches via wrote: > I removed the 2 patches for previous CVEs that are now merged within > gnutls sources. Thanks for this patch! > I deliberately committed it to master branch despite > guix refresh --list-dependent gnutls returns 5287 packages and that > https://guix.gnu.org/manual/en/guix.html#Submitting-Patches says such > packages with more than 3000 impacted packages should be committed > on core-updates. I did this because it's a minor update to fix a CVE > so this would be weird to wait 6 months for this update. Whether or not the update is minor, we still have to use a "graft" [0] to change packages with this many dependents on the master branch. Due to the "functional packaging model" of Guix, every dependent of GnuTLS must be recompiled when the GnuTLS package is changed. We would constantly be rebuilding nearly every single package if we did not use grafts for security updates, and that would be infeasible and inefficient. Grafts effectively rewrite binary references in compiled software, so it's kind of a kludge. The binary interface of the new grafted replacement must be compatible with the original package, and if it's not, the problems can be hidden and subtle. For that reason, it's important to make the smallest change possible when grafting, to reduce the chance of breakage. So, the question is, does 3.6.16 include only the fix for CVE-2021-20305? Or does it also include other changes? If the former, we should instead cherry-pick the CVE bug fix instead of updating. Can you look into that and let us know? > --- a/gnu/packages/patches/gnutls-CVE-2021-20231.patch > +++ /dev/null If we do decide to update to 3.6.16, it's also necessary to deregister the removed patch files in 'gnu/local.mk'. Check this commit for an example: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=7c4c781aa40c42d4cd10b8d9482199f3db345e1b Finally, here is an example of setting up a graft that includes a single new patch file: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=7c4c781aa40c42d4cd10b8d9482199f3db345e1b And here is an example of a graft that "updates" a package: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=250a216cdc2d5425ee0053f3e614d54e0fb6aa90
Leo Famulari <leo@famulari.name> skriver: > Grafts effectively rewrite binary references in compiled software, so > it's kind of a kludge. The binary interface of the new grafted > replacement must be compatible with the original package, and if it's > not, the problems can be hidden and subtle. > > For that reason, it's important to make the smallest change possible > when grafting, to reduce the chance of breakage. > > So, the question is, does 3.6.16 include only the fix for > CVE-2021-20305? Or does it also include other changes? If the former, we > should instead cherry-pick the CVE bug fix instead of updating. GnuTLS usually mention whether or not an update is ABI-compatible: https://lists.gnupg.org/pipermail/gnutls-help/2021-May/004707.html However it's good practice to verify that with something like 'abidiff' (from the 'libabigail' package). I.e.: abidiff $(guix build gnutls)/lib/libgnutls.so \ $(./pre-inst-env guix build gnutls)/lib/libgnutls.so (this won't work because of multiple outputs, but you get the drill) When there is no change, the graft _should_ be perfectly safe. If there are changes, it becomes a judgement call. The 'abidiff' output is of great assistance in that case. Anyway, just some general notes on grafting. Thanks a lot for looking after security issues Solene.
On Tue, May 25, 2021 at 09:46:10PM +0200, Marius Bakke wrote: > GnuTLS usually mention whether or not an update is ABI-compatible: > > https://lists.gnupg.org/pipermail/gnutls-help/2021-May/004707.html Ah, that's great. They say it's compatible. > However it's good practice to verify that with something like 'abidiff' > (from the 'libabigail' package). I.e.: > > abidiff $(guix build gnutls)/lib/libgnutls.so \ > $(./pre-inst-env guix build gnutls)/lib/libgnutls.so > > (this won't work because of multiple outputs, but you get the drill) Solene, can you try it and let us know the result?
Le Tue, 25 May 2021 16:00:53 -0400, Leo Famulari <leo@famulari.name> a écrit : > On Tue, May 25, 2021 at 09:46:10PM +0200, Marius Bakke wrote: > > GnuTLS usually mention whether or not an update is ABI-compatible: > > > > https://lists.gnupg.org/pipermail/gnutls-help/2021-May/004707.html > > Ah, that's great. They say it's compatible. > > > However it's good practice to verify that with something like 'abidiff' > > (from the 'libabigail' package). I.e.: > > > > abidiff $(guix build gnutls)/lib/libgnutls.so \ > > $(./pre-inst-env guix build gnutls)/lib/libgnutls.so > > > > (this won't work because of multiple outputs, but you get the drill) > > Solene, can you try it and let us know the result? abidiff is an interesting program, very useful. $ abidiff \ /gnu/store/5yvzilh78996627i8avq532sl2c03i95-gnutls-3.6.15/lib/libgnutls.so \ /gnu/store/akc7l65z459pnifrr6bcm97cjvmpvp9k-gnutls-3.6.16/lib/libgnutls.so $ echo $? 0 I understand from the output that there is no ABI change.
On Tue, May 25, 2021 at 10:47:57PM +0200, Solene Rapenne wrote:
> I understand from the output that there is no ABI change.
Great! So, what's left for this patch is to set up the graft.
Concretely, that means creating a new variable 'gnutls-3.6.16' that
inherits from 'gnutls' and adjusts the version and source fields. Then,
add a replacement field to the new 'gnutls' package that uses
'gnutls-3.6.16'.
Can you send a revised patch?
diff --git a/gnu/packages/patches/gnutls-CVE-2021-20231.patch b/gnu/packages/patches/gnutls-CVE-2021-20231.patch deleted file mode 100644 index 5186522eee..0000000000 --- a/gnu/packages/patches/gnutls-CVE-2021-20231.patch +++ /dev/null @@ -1,62 +0,0 @@ -From 15beb4b193b2714d88107e7dffca781798684e7e Mon Sep 17 00:00:00 2001 -From: Daiki Ueno <ueno@gnu.org> -Date: Fri, 29 Jan 2021 14:06:05 +0100 -Subject: [PATCH 1/2] key_share: avoid use-after-free around realloc - -Signed-off-by: Daiki Ueno <ueno@gnu.org> ---- - lib/ext/key_share.c | 12 +++++------- - 1 file changed, 5 insertions(+), 7 deletions(-) - -diff --git a/lib/ext/key_share.c b/lib/ext/key_share.c -index ab8abf8fe..a8c4bb5cf 100644 ---- a/lib/ext/key_share.c -+++ b/lib/ext/key_share.c -@@ -664,14 +664,14 @@ key_share_send_params(gnutls_session_t session, - { - unsigned i; - int ret; -- unsigned char *lengthp; -- unsigned int cur_length; - unsigned int generated = 0; - const gnutls_group_entry_st *group; - const version_entry_st *ver; - - /* this extension is only being sent on client side */ - if (session->security_parameters.entity == GNUTLS_CLIENT) { -+ unsigned int length_pos; -+ - ver = _gnutls_version_max(session); - if (unlikely(ver == NULL || ver->key_shares == 0)) - return 0; -@@ -679,16 +679,13 @@ key_share_send_params(gnutls_session_t session, - if (!have_creds_for_tls13(session)) - return 0; - -- /* write the total length later */ -- lengthp = &extdata->data[extdata->length]; -+ length_pos = extdata->length; - - ret = - _gnutls_buffer_append_prefix(extdata, 16, 0); - if (ret < 0) - return gnutls_assert_val(ret); - -- cur_length = extdata->length; -- - if (session->internals.hsk_flags & HSK_HRR_RECEIVED) { /* we know the group */ - group = get_group(session); - if (unlikely(group == NULL)) -@@ -736,7 +733,8 @@ key_share_send_params(gnutls_session_t session, - } - - /* copy actual length */ -- _gnutls_write_uint16(extdata->length - cur_length, lengthp); -+ _gnutls_write_uint16(extdata->length - length_pos - 2, -+ &extdata->data[length_pos]); - - } else { /* server */ - ver = get_version(session); --- -2.30.2 - diff --git a/gnu/packages/patches/gnutls-CVE-2021-20232.patch b/gnu/packages/patches/gnutls-CVE-2021-20232.patch deleted file mode 100644 index dc3a0be690..0000000000 --- a/gnu/packages/patches/gnutls-CVE-2021-20232.patch +++ /dev/null @@ -1,60 +0,0 @@ -From 75a937d97f4fefc6f9b08e3791f151445f551cb3 Mon Sep 17 00:00:00 2001 -From: Daiki Ueno <ueno@gnu.org> -Date: Fri, 29 Jan 2021 14:06:23 +0100 -Subject: [PATCH 2/2] pre_shared_key: avoid use-after-free around realloc - -Signed-off-by: Daiki Ueno <ueno@gnu.org> ---- - lib/ext/pre_shared_key.c | 15 ++++++++++++--- - 1 file changed, 12 insertions(+), 3 deletions(-) - -diff --git a/lib/ext/pre_shared_key.c b/lib/ext/pre_shared_key.c -index a042c6488..380bf39ed 100644 ---- a/lib/ext/pre_shared_key.c -+++ b/lib/ext/pre_shared_key.c -@@ -267,7 +267,7 @@ client_send_params(gnutls_session_t session, - size_t spos; - gnutls_datum_t username = {NULL, 0}; - gnutls_datum_t user_key = {NULL, 0}, rkey = {NULL, 0}; -- gnutls_datum_t client_hello; -+ unsigned client_hello_len; - unsigned next_idx; - const mac_entry_st *prf_res = NULL; - const mac_entry_st *prf_psk = NULL; -@@ -428,8 +428,7 @@ client_send_params(gnutls_session_t session, - assert(extdata->length >= sizeof(mbuffer_st)); - assert(ext_offset >= (ssize_t)sizeof(mbuffer_st)); - ext_offset -= sizeof(mbuffer_st); -- client_hello.data = extdata->data+sizeof(mbuffer_st); -- client_hello.size = extdata->length-sizeof(mbuffer_st); -+ client_hello_len = extdata->length-sizeof(mbuffer_st); - - next_idx = 0; - -@@ -440,6 +439,11 @@ client_send_params(gnutls_session_t session, - } - - if (prf_res && rkey.size > 0) { -+ gnutls_datum_t client_hello; -+ -+ client_hello.data = extdata->data+sizeof(mbuffer_st); -+ client_hello.size = client_hello_len; -+ - ret = compute_psk_binder(session, prf_res, - binders_len, binders_pos, - ext_offset, &rkey, &client_hello, 1, -@@ -474,6 +478,11 @@ client_send_params(gnutls_session_t session, - } - - if (prf_psk && user_key.size > 0 && info) { -+ gnutls_datum_t client_hello; -+ -+ client_hello.data = extdata->data+sizeof(mbuffer_st); -+ client_hello.size = client_hello_len; -+ - ret = compute_psk_binder(session, prf_psk, - binders_len, binders_pos, - ext_offset, &user_key, &client_hello, 0, --- -2.30.2 - diff --git a/gnu/packages/tls.scm b/gnu/packages/tls.scm index 174438ad87..ed8fc6532a 100644 --- a/gnu/packages/tls.scm +++ b/gnu/packages/tls.scm @@ -15,6 +15,7 @@ ;;; Copyright © 2018 Clément Lassieur <clement@lassieur.org> ;;; Copyright © 2019 Mathieu Othacehe <m.othacehe@gmail.com> ;;; Copyright © 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org> +;;; Copyright © 2021 Solene Rapenne <solene@perso.pw> ;;; ;;; This file is part of GNU Guix. ;;; @@ -164,7 +165,7 @@ living in the same process.") (define-public gnutls (package (name "gnutls") - (version "3.6.15") + (version "3.6.16") (source (origin (method url-fetch) ;; Note: Releases are no longer on ftp.gnu.org since the @@ -173,12 +174,10 @@ living in the same process.") (version-major+minor version) "/gnutls-" version ".tar.xz")) (patches (search-patches "gnutls-skip-trust-store-test.patch" - "gnutls-cross.patch" - "gnutls-CVE-2021-20231.patch" - "gnutls-CVE-2021-20232.patch")) + "gnutls-cross.patch")) (sha256 (base32 - "0n0m93ymzd0q9hbknxc2ycanz49sqlkyyf73g9fk7n787llc7a0f")))) + "1czk511pslz367shf32f2jvvkp7y1323bcv88c2qng98mj0v6y8v")))) (build-system gnu-build-system) (arguments `(#:tests? ,(not (or (%current-target-system)