[PATCH] gnu: icu4c: Fix CVE-2020-10531.

  • Done
  • quality assurance status badge
Details
2 participants
  • Leo Famulari
  • Marius Bakke
Owner
unassigned
Submitted by
Leo Famulari
Severity
normal
L
L
Leo Famulari wrote on 25 Mar 2020 19:36
(address . guix-patches@gnu.org)
a6bdc446d6d35752876db8ffaf946abfb8de355a.1585161363.git.leo@famulari.name
* gnu/packages/patches/icu4c-CVE-2020-10531.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/icu4c.scm (icu4c)[replacement]: New field.
(icu4c/fixed): New variable.
---
gnu/local.mk | 1 +
gnu/packages/icu4c.scm | 11 ++
.../patches/icu4c-CVE-2020-10531.patch | 126 ++++++++++++++++++
3 files changed, 138 insertions(+)
create mode 100644 gnu/packages/patches/icu4c-CVE-2020-10531.patch

Toggle diff (175 lines)
diff --git a/gnu/local.mk b/gnu/local.mk
index 7cce60b7c0..c905bd3b37 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1033,6 +1033,7 @@ dist_patch_DATA = \
%D%/packages/patches/icecat-use-system-media-libs.patch \
%D%/packages/patches/icedtea-6-hotspot-gcc-segfault-workaround.patch \
%D%/packages/patches/icedtea-7-hotspot-gcc-segfault-workaround.patch \
+ %D%/packages/patches/icu4c-CVE-2020-10531.patch \
%D%/packages/patches/id3lib-CVE-2007-4460.patch \
%D%/packages/patches/id3lib-UTF16-writing-bug.patch \
%D%/packages/patches/ilmbase-fix-tests.patch \
diff --git a/gnu/packages/icu4c.scm b/gnu/packages/icu4c.scm
index 922dfbd348..bc74da5942 100644
--- a/gnu/packages/icu4c.scm
+++ b/gnu/packages/icu4c.scm
@@ -56,6 +56,7 @@
(define-public icu4c
(package
(name "icu4c")
+ (replacement icu4c/fixed)
(version "64.2")
(source (origin
(method url-fetch)
@@ -105,6 +106,16 @@ C/C++ part.")
(license x11)
(home-page "http://site.icu-project.org/")))
+(define icu4c/fixed
+ (package
+ (inherit icu4c)
+ (source (origin
+ (inherit (package-source icu4c))
+ (patches (append
+ (origin-patches (package-source icu4c))
+ (search-patches
+ "icu4c-CVE-2020-10531.patch")))))))
+
(define-public java-icu4j
(package
(name "java-icu4j")
diff --git a/gnu/packages/patches/icu4c-CVE-2020-10531.patch b/gnu/packages/patches/icu4c-CVE-2020-10531.patch
new file mode 100644
index 0000000000..e996783e75
--- /dev/null
+++ b/gnu/packages/patches/icu4c-CVE-2020-10531.patch
@@ -0,0 +1,126 @@
+Fix CVE-2020-10531:
+
+https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10531
+
+Patch copied from upstream source repository:
+
+https://github.com/unicode-org/icu/commit/b7d08bc04a4296982fcef8b6b8a354a9e4e7afca
+
+From b7d08bc04a4296982fcef8b6b8a354a9e4e7afca Mon Sep 17 00:00:00 2001
+From: Frank Tang <ftang@chromium.org>
+Date: Sat, 1 Feb 2020 02:39:04 +0000
+Subject: [PATCH] ICU-20958 Prevent SEGV_MAPERR in append
+
+See #971
+---
+ icu4c/source/common/unistr.cpp | 6 ++-
+ icu4c/source/test/intltest/ustrtest.cpp | 62 +++++++++++++++++++++++++
+ icu4c/source/test/intltest/ustrtest.h | 1 +
+ 3 files changed, 68 insertions(+), 1 deletion(-)
+
+diff --git a/icu4c/source/common/unistr.cpp b/icu4c/source/common/unistr.cpp
+index 901bb3358ba..077b4d6ef20 100644
+--- a/icu4c/source/common/unistr.cpp
++++ b/icu4c/source/common/unistr.cpp
+@@ -1563,7 +1563,11 @@ UnicodeString::doAppend(const UChar *srcChars, int32_t srcStart, int32_t srcLeng
+ }
+
+ int32_t oldLength = length();
+- int32_t newLength = oldLength + srcLength;
++ int32_t newLength;
++ if (uprv_add32_overflow(oldLength, srcLength, &newLength)) {
++ setToBogus();
++ return *this;
++ }
+
+ // Check for append onto ourself
+ const UChar* oldArray = getArrayStart();
+diff --git a/icu4c/source/test/intltest/ustrtest.cpp b/icu4c/source/test/intltest/ustrtest.cpp
+index b6515ea813c..ad38bdf53a3 100644
+--- a/icu4c/source/test/intltest/ustrtest.cpp
++++ b/icu4c/source/test/intltest/ustrtest.cpp
+@@ -67,6 +67,7 @@ void UnicodeStringTest::runIndexedTest( int32_t index, UBool exec, const char* &
+ TESTCASE_AUTO(TestWCharPointers);
+ TESTCASE_AUTO(TestNullPointers);
+ TESTCASE_AUTO(TestUnicodeStringInsertAppendToSelf);
++ TESTCASE_AUTO(TestLargeAppend);
+ TESTCASE_AUTO_END;
+ }
+
+@@ -2310,3 +2311,64 @@ void UnicodeStringTest::TestUnicodeStringInsertAppendToSelf() {
+ str.insert(2, sub);
+ assertEquals("", u"abbcdcde", str);
+ }
++
++void UnicodeStringTest::TestLargeAppend() {
++ if(quick) return;
++
++ IcuTestErrorCode status(*this, "TestLargeAppend");
++ // Make a large UnicodeString
++ int32_t len = 0xAFFFFFF;
++ UnicodeString str;
++ char16_t *buf = str.getBuffer(len);
++ // A fast way to set buffer to valid Unicode.
++ // 4E4E is a valid unicode character
++ uprv_memset(buf, 0x4e, len * 2);
++ str.releaseBuffer(len);
++ UnicodeString dest;
++ // Append it 16 times
++ // 0xAFFFFFF times 16 is 0xA4FFFFF1,
++ // which is greater than INT32_MAX, which is 0x7FFFFFFF.
++ int64_t total = 0;
++ for (int32_t i = 0; i < 16; i++) {
++ dest.append(str);
++ total += len;
++ if (total <= INT32_MAX) {
++ assertFalse("dest is not bogus", dest.isBogus());
++ } else {
++ assertTrue("dest should be bogus", dest.isBogus());
++ }
++ }
++ dest.remove();
++ total = 0;
++ for (int32_t i = 0; i < 16; i++) {
++ dest.append(str);
++ total += len;
++ if (total + len <= INT32_MAX) {
++ assertFalse("dest is not bogus", dest.isBogus());
++ } else if (total <= INT32_MAX) {
++ // Check that a string of exactly the maximum size works
++ UnicodeString str2;
++ int32_t remain = INT32_MAX - total;
++ char16_t *buf2 = str2.getBuffer(remain);
++ if (buf2 == nullptr) {
++ // if somehow memory allocation fail, return the test
++ return;
++ }
++ uprv_memset(buf2, 0x4e, remain * 2);
++ str2.releaseBuffer(remain);
++ dest.append(str2);
++ total += remain;
++ assertEquals("When a string of exactly the maximum size works", (int64_t)INT32_MAX, total);
++ assertEquals("When a string of exactly the maximum size works", INT32_MAX, dest.length());
++ assertFalse("dest is not bogus", dest.isBogus());
++
++ // Check that a string size+1 goes bogus
++ str2.truncate(1);
++ dest.append(str2);
++ total++;
++ assertTrue("dest should be bogus", dest.isBogus());
++ } else {
++ assertTrue("dest should be bogus", dest.isBogus());
++ }
++ }
++}
+diff --git a/icu4c/source/test/intltest/ustrtest.h b/icu4c/source/test/intltest/ustrtest.h
+index 218befdcc68..4a356a92c7a 100644
+--- a/icu4c/source/test/intltest/ustrtest.h
++++ b/icu4c/source/test/intltest/ustrtest.h
+@@ -97,6 +97,7 @@ class UnicodeStringTest: public IntlTest {
+ void TestWCharPointers();
+ void TestNullPointers();
+ void TestUnicodeStringInsertAppendToSelf();
++ void TestLargeAppend();
+ };
+
+ #endif
--
2.26.0
M
M
Marius Bakke wrote on 25 Mar 2020 21:23
87v9msyyii.fsf@devup.no
Leo Famulari <leo@famulari.name> writes:

Toggle quote (5 lines)
> * gnu/packages/patches/icu4c-CVE-2020-10531.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/icu4c.scm (icu4c)[replacement]: New field.
> (icu4c/fixed): New variable.

[...]

Toggle quote (26 lines)
> diff --git a/gnu/packages/patches/icu4c-CVE-2020-10531.patch b/gnu/packages/patches/icu4c-CVE-2020-10531.patch
> new file mode 100644
> index 0000000000..e996783e75
> --- /dev/null
> +++ b/gnu/packages/patches/icu4c-CVE-2020-10531.patch
> @@ -0,0 +1,126 @@
> +Fix CVE-2020-10531:
> +
> +https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10531
> +
> +Patch copied from upstream source repository:
> +
> +https://github.com/unicode-org/icu/commit/b7d08bc04a4296982fcef8b6b8a354a9e4e7afca
> +
> +From b7d08bc04a4296982fcef8b6b8a354a9e4e7afca Mon Sep 17 00:00:00 2001
> +From: Frank Tang <ftang@chromium.org>
> +Date: Sat, 1 Feb 2020 02:39:04 +0000
> +Subject: [PATCH] ICU-20958 Prevent SEGV_MAPERR in append
> +
> +See #971
> +---
> + icu4c/source/common/unistr.cpp | 6 ++-
> + icu4c/source/test/intltest/ustrtest.cpp | 62 +++++++++++++++++++++++++
> + icu4c/source/test/intltest/ustrtest.h | 1 +
> + 3 files changed, 68 insertions(+), 1 deletion(-)

I'm not sure if the new test case as well as this git commit header is
necessary. IMO it mostly adds noise to the patch. I.e. the whole file
could be shortened to 6 lines + your comments at the top.

But no strong opinion, there is an argument to be made for preserving
upstream commits in their entirety too (I think).

So, LGTM either way. Thank you!
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAl57vcYACgkQoqBt8qM6
VPqWPAgAlRC4x83a1FfWz4ThARLooj9aJlSKbI78N4dLFU8dqf9+j1FJ+ylwGRgR
GHRJjdcdSbly0CaeNcdHXHA4t93aDxAMrEWfRoKiv/d4AbAO/jNAjvyIq+erczcb
+9zCoAQYj8T174ck2QEPlT+KL5pA6jctEX7Z2JaFqtJ5qaXta7uFqLssytrT1v6t
LsWByTwIbI76FokXb2Ni/6lAqokrbfRQTDVXTwPWKO83iaNlTWaNoINRfCUy7Vgm
WbSFJEcUhOhziWyLI62VMBVyffqfGMXhftN8RJq1+iEgBxSgtafD2gowsZP3Onu9
r0AuPRZdfRO8Ta+wYZc3HNezQwwSTA==
=FWl5
-----END PGP SIGNATURE-----

L
L
Leo Famulari wrote on 25 Mar 2020 22:54
(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 40227-done@debbugs.gnu.org)
20200325215427.GA29579@jasmine.lan
On Wed, Mar 25, 2020 at 09:23:33PM +0100, Marius Bakke wrote:
Toggle quote (9 lines)
> I'm not sure if the new test case as well as this git commit header is
> necessary. IMO it mostly adds noise to the patch. I.e. the whole file
> could be shortened to 6 lines + your comments at the top.
>
> But no strong opinion, there is an argument to be made for preserving
> upstream commits in their entirety too (I think).
>
> So, LGTM either way. Thank you!

I commented out the changes to the test suite and pushed as
7d57a190f6896c04b5dad66bf4360bc48a4052ff.

Thanks for the review!
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEsFFZSPHn08G5gDigJkb6MLrKfwgFAl570xMACgkQJkb6MLrK
fwjB7RAAgIPq4a6n6FB4Gu76PpFDAOvGoCsQlLviPSVeP43OkkgvpR/g2GjlLLSt
HZDKLYeec1uOAZj9fuGz+aJzRIclrLTdWls8THXXXF/L5FXUhoBWQubSvj54/yiu
o5R9K1/w5pvnHE1gSwGZJGRB8aFH+hcZxMqtnpk7J+XkbQik12PxlNjAh7iubbp5
d/9IDNxdnjoDCH94w3VbSEPTSY+iKSCLLh1bsD8jVL4F2/u3ocO051x+D2NGexMT
0ftnLIU5HQ/PFpWS1hcHXW0MXWn1/hU1yBvjDKksYfwVfV8fM/7a2LeB3qeva63/
K/e17QLnCv5ridxNnO9ndUzSfuHMzaIx3y9en4JrmHQVcPw56flnrbn2BPtmBZN9
AKwWJ//kkYsjcmHjyHOmnbG/LiDCVkfQGc8jt33DLvwFmQuTCiIgvKXJE3UDyyYc
PQe0lmhz2/GjkC/fEdwyXSeA10RU8povQCBqeYemzTug1zi0ncveAIziYaTeBLut
MMJwehgIvdQdul2ALSRUiLTin+p0bfZhqnHDMVEgyUCZzzx256cecSbzsol3NgE4
nl5dZFxrncZ4RIBOpqdoXF0fM5m+In4YVYlUHNq0D+Z1pogAGlovt2/7pxUYjh4w
FvJQEVBnVEVRsobJrfilTBWQnCYHN0d8OciW1Yq2IaIZlgfJCCk=
=ab6S
-----END PGP SIGNATURE-----


Closed
M
M
Marius Bakke wrote on 25 Mar 2020 23:41
(name . Leo Famulari)(address . leo@famulari.name)(address . 40227-done@debbugs.gnu.org)
87sghwys5a.fsf@devup.no
Leo Famulari <leo@famulari.name> writes:

Toggle quote (13 lines)
> On Wed, Mar 25, 2020 at 09:23:33PM +0100, Marius Bakke wrote:
>> I'm not sure if the new test case as well as this git commit header is
>> necessary. IMO it mostly adds noise to the patch. I.e. the whole file
>> could be shortened to 6 lines + your comments at the top.
>>
>> But no strong opinion, there is an argument to be made for preserving
>> upstream commits in their entirety too (I think).
>>
>> So, LGTM either way. Thank you!
>
> I commented out the changes to the test suite and pushed as
> 7d57a190f6896c04b5dad66bf4360bc48a4052ff.

What I meant was that they could be omitted entirely to shorten the
patch (less lines to comb through for reviewers), but no worries! The
important thing is that we get the security fix, thanks for watching out
for those as always. :-)
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAl573gEACgkQoqBt8qM6
VPpXOwgAj2bxdvLOQx97jTNhT6yxPORYcpCHGX6XOW3FRuYWiCT2cCgMJ76IGTjI
JsbhdkHD04WAVKoUYcUOXVIS3q3xP+OdfOqrm/NNlyKVI7CoW2/+QMPfITecOt7w
VeQjd98Z5d+6Q7a3Z0veY4KFGx+KCEcGEAvrQSzOC3d0ex7lyKllU1hrZN5Row4z
PNJ8WwqpbCEF+zUddxTvQa63wYMR70aTpvV6+CbEW9WZuuxNmwLHNEgyf5exZgxa
r0JQ3eQ+aWYHq7tRtJY18CI3eTYmzBG7sAGi9HaM4z459FJaefcXXSGvDJQ8vpdf
6mLnLBfTZlqcHXel1Oz/5IoEr8JbTQ==
=aRlZ
-----END PGP SIGNATURE-----

Closed
?