[bug#36424] expat-2.2.7 for CVE-2018-20843

On Tue, 2 Jul 2019, Jack Hill wrote:

>> Apparently these symbols were never supposed to be exported:
>> <https://github.com/libexpat/libexpat/pull/197>.  However, there could
>> be packages "in the wild" that uses these symbols and would silently
>> break with the grafted Expat.
>> 
>> IIUC the fix for CVE-2018-20843 is this commit:
>> <https://github.com/libexpat/libexpat/commit/11f8838bf99ea0a6f0b76f9760c43704d00c4ff6>.
>> 
>> I think it's better to graft a variant with only this patch to be on the
>> safe side.  Can you try that?
>
> Good idea. I didn't think to check. Yes, I can try to do that.
>
>> Could you also submit a second patch that adds GitHub as an additional
>> download location for the regular Expat package?  :-)
>
> I'll try that as well.

I've prepared the two attached patches that I believe implement Marius's 
proposed solution.

Thanks,
Jack

Woops, looks like I still mangled the patches (by adding carriage-returns), 
but hopefully in a way that they still apply without infecting the code 
with that problem.

I guess Alpine has let me down. At any rate, hopefully they're still 
useful and fix the problem. Let me know if you'd like me to try again.

Best,
Jack

Also, sorry for the extra noise in gnu/local.mk. I had inserted my patch 
in the wrong place and alphabetized a number of lines using my 
en_us.UTF-8 locale to fix it. Let me know if I should re-submit without 
the extraneous changes.

Today hasn't been the best day for computer use for me I'm afraid.

Best,
Jack

Jack Hill <jackhill@jackhill.us> writes:

> On Tue, 2 Jul 2019, Jack Hill wrote:
>
>>> Apparently these symbols were never supposed to be exported:
>>> <https://github.com/libexpat/libexpat/pull/197>.  However, there could
>>> be packages "in the wild" that uses these symbols and would silently
>>> break with the grafted Expat.
>>> 
>>> IIUC the fix for CVE-2018-20843 is this commit:
>>> <https://github.com/libexpat/libexpat/commit/11f8838bf99ea0a6f0b76f9760c43704d00c4ff6>.
>>> 
>>> I think it's better to graft a variant with only this patch to be on the
>>> safe side.  Can you try that?
>>
>> Good idea. I didn't think to check. Yes, I can try to do that.
>>
>>> Could you also submit a second patch that adds GitHub as an additional
>>> download location for the regular Expat package?  :-)
>>
>> I'll try that as well.
>
> I've prepared the two attached patches that I believe implement Marius's 
> proposed solution.

Thanks!

One minor problem... the expat patch does not actually apply on our copy
of expat!  Can you look into it?

> From 4186a68b660c93b5800be8f126051da92749dc9a Mon Sep 17 00:00:00 2001
> From: Jack Hill <jackhill@jackhill.us>
> Date: Thu, 4 Jul 2019 17:00:27 -0400
> Subject: [PATCH 1/2] gnu: expat: Add additional source URI
>
> The expat sourceforge page announces that the project is in the process of
> moving to GitHub.
>
> * gnu/packages/xml.scm (expat)[source]: Add GitHub URI.
> ---
>  gnu/packages/xml.scm | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)

[...]
  
>  (define-public expat
> -  (package
> -    (name "expat")
> -    (version "2.2.6")
> -    (source (origin
> -             (method url-fetch)
> -             (uri (string-append "mirror://sourceforge/expat/expat/"
> -                                 version "/expat-" version ".tar.bz2"))
> -             (sha256
> -              (base32
> -               "1wl1x93b5w457ddsdgj0lh7yjq4q6l7wfbgwhagkc8fm2qkkrd0p"))))
> -    (build-system gnu-build-system)
> -    (home-page "https://libexpat.github.io/")
> -    (synopsis "Stream-oriented XML parser library written in C")
> -    (description
> -     "Expat is an XML parser library written in C.  It is a
> +  (let ((dot->underscore (lambda (c) (if (equal? #\. c) #\_ c))))
> +      (package
> +        (name "expat")
> +        (version "2.2.6")
> +        (source (origin
> +                  (method url-fetch)
> +                  (uri (list (string-append
> +                              "mirror://sourceforge/expat/expat/"
> +                              version "/expat-" version ".tar.bz2")
> +                             (string-append
> +                              "https://github.com/libexpat/libexpat/releases/download/R_"
> +                              (string-map dot->underscore version)
> +                              "/expat-" version ".tar.bz2")))
> +                  (sha256
> +                   (base32
> +                    "1wl1x93b5w457ddsdgj0lh7yjq4q6l7wfbgwhagkc8fm2qkkrd0p"))))
> +        (build-system gnu-build-system)
> +        (home-page "https://libexpat.github.io/")
> +        (synopsis "Stream-oriented XML parser library written in C")
> +        (description
> +         "Expat is an XML parser library written in C.  It is a

Can you move this let binding inside the (source ...) field?  That way
we don't have to reindent the whole thing.

> From 2f8268a0b549b9c08744d8bc05e2cf135e40be99 Mon Sep 17 00:00:00 2001
> From: Jack Hill <jackhill@jackhill.us>
> Date: Thu, 4 Jul 2019 19:41:30 -0400
> Subject: [PATCH 2/2] gnu: expat: fix CVE-2018-20843.
>
> * gnu/packages/xml.scm (expat)[replacement]: New field.
> (expat/fixed): New variable.
> * gnu/packages/patches/expat-CVE-2018-20843.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add patch file.
> ---
>  gnu/local.mk                                    |  7 ++++---
>  gnu/packages/patches/expat-CVE-2018-20843.patch | 16 ++++++++++++++++
>  gnu/packages/xml.scm                            |  9 +++++++++
>  3 files changed, 29 insertions(+), 3 deletions(-)
>  create mode 100644 gnu/packages/patches/expat-CVE-2018-20843.patch
>
> diff --git a/gnu/local.mk b/gnu/local.mk
> index 6e90d88689..bcf47d7378 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -764,20 +764,21 @@ dist_patch_DATA =						\
>    %D%/packages/patches/einstein-build.patch			\
>    %D%/packages/patches/emacs-exec-path.patch			\
>    %D%/packages/patches/emacs-fix-scheme-indent-function.patch	\
> -  %D%/packages/patches/emacs-json-reformat-fix-tests.patch	\
>    %D%/packages/patches/emacs-highlight-stages-add-gexp.patch	\
> +  %D%/packages/patches/emacs-json-reformat-fix-tests.patch	\
>    %D%/packages/patches/emacs-scheme-complete-scheme-r5rs-info.patch	\
>    %D%/packages/patches/emacs-source-date-epoch.patch		\
> -  %D%/packages/patches/emacs-unpackaged-req.patch		\
>    %D%/packages/patches/emacs-undohist-ignored.patch	\
> +  %D%/packages/patches/emacs-unpackaged-req.patch		\
>    %D%/packages/patches/emacs-wordnut-require-adaptive-wrap.patch	\
>    %D%/packages/patches/emacs-zones-called-interactively.patch	\
>    %D%/packages/patches/enlightenment-fix-setuid-path.patch	\
>    %D%/packages/patches/erlang-man-path.patch			\
>    %D%/packages/patches/eudev-rules-directory.patch		\
>    %D%/packages/patches/evilwm-lost-focus-bug.patch		\
> -  %D%/packages/patches/exiv2-CVE-2017-14860.patch		\
>    %D%/packages/patches/exiv2-CVE-2017-14859-14862-14864.patch	\
> +  %D%/packages/patches/exiv2-CVE-2017-14860.patch		\
> +  %D%/packages/patches/expat-CVE-2018-20843.patch		\

You addressed this in another email, and I do think we should try to
avoid needless moving around of these lines.  There are enough merge
conflicts on this file as-is, no need to introduce artificial ones.  :-)

>    %D%/packages/patches/extundelete-e2fsprogs-1.44.patch		\
>    %D%/packages/patches/fastcap-mulGlobal.patch			\
>    %D%/packages/patches/fastcap-mulSetup.patch			\
> diff --git a/gnu/packages/patches/expat-CVE-2018-20843.patch b/gnu/packages/patches/expat-CVE-2018-20843.patch
> new file mode 100644
> index 0000000000..dd64b91965
> --- /dev/null
> +++ b/gnu/packages/patches/expat-CVE-2018-20843.patch
> @@ -0,0 +1,16 @@
> +Fix extraction of namespace prefix from XML name.
> +Fixes CVE-2018-20843
> +
> +diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c
> +index 30d55c5..737d7cd 100644
> +--- a/expat/lib/xmlparse.c
> ++++ b/expat/lib/xmlparse.c
         ^^^^^^
It looks like this has to be removed from the patch file.  Could you
also add a link to the upstream commit for reference?

It's also good practice to provide an URL to the MITRE CVE page:
<https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-20843>.

Thanks for working on this!  :-)