[PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[Zhixiong Chi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 5795 bytes --]

CVE-2017-13089:The http.c:skip_short_body() function is called in some
circumstances, such as when processing redirects. When the response is
sent chunked in wget before 1.19.2, the chunk parser uses strtol() to
read each chunk's length, but doesn't check that the chunk length is a
non-negative number. The code then tries to skip the chunk in pieces of
512 bytes by using the MIN() macro, but ends up passing the negative
chunk length to connect.c:fd_read(). As fd_read() takes an int argument,
the high 32 bits of the chunk length are discarded, leaving fd_read()
with a completely attacker controlled length argument.

CVE-2017-13090:The retr.c:fd_read_body() function is called when
processing OK responses. When the response is sent chunked in wget before
1.19.2, the chunk parser uses strtol() to read each chunk's length, but
doesn't check that the chunk length is a non-negative number. The code
then tries to read the chunk in pieces of 8192 bytes by using the MIN()
macro, but ends up passing the negative chunk length to retr.c:fd_read().
As fd_read() takes an int argument, the high 32 bits of the chunk length
are discarded, leaving fd_read() with a completely attacker controlled
length argument. The attacker can corrupt malloc metadata after the
allocated buffer.

External References:
https://nvd.nist.gov/vuln/detail/CVE-2017-13089
https://nvd.nist.gov/vuln/detail/CVE-2017-13090

Patches from:
http://git.savannah.gnu.org/cgit/wget.git/patch/?id=d892291fb8ace4c3b734ea5125770989c215df3f
http://git.savannah.gnu.org/cgit/wget.git/patch/?id=ba6b44f6745b14dce414761a8e4b35d31b176bba

CVE: CVE-2017-13089 CVE-2017-13090

Signed-off-by: Zhixiong Chi <zhixiong.chi@windriver.com>
---
 .../wget/wget/wget-CVE-2017-13089.patch            | 36 ++++++++++++++++++++
 .../wget/wget/wget-CVE-2017-13090.patch            | 39 ++++++++++++++++++++++
 meta/recipes-extended/wget/wget_1.19.1.bb          |  2 ++
 3 files changed, 77 insertions(+)
 create mode 100644 meta/recipes-extended/wget/wget/wget-CVE-2017-13089.patch
 create mode 100644 meta/recipes-extended/wget/wget/wget-CVE-2017-13090.patch

diff --git a/meta/recipes-extended/wget/wget/wget-CVE-2017-13089.patch b/meta/recipes-extended/wget/wget/wget-CVE-2017-13089.patch
new file mode 100644
index 0000000..f7fa90c
--- /dev/null
+++ b/meta/recipes-extended/wget/wget/wget-CVE-2017-13089.patch
@@ -0,0 +1,36 @@
+From d892291fb8ace4c3b734ea5125770989c215df3f Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Tim=20R=C3=BChsen?= <tim.ruehsen@gmx.de>
+Date: Fri, 20 Oct 2017 10:59:38 +0200
+Subject: Fix stack overflow in HTTP protocol handling (CVE-2017-13089)
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+* src/http.c (skip_short_body): Return error on negative chunk size
+
+CVE: CVE-2017-13089
+
+Upstream-Status: Backport
+
+Reported-by: Antti Levomäki, Christian Jalio, Joonas Pihlaja from Forcepoint
+Reported-by: Juhani Eronen from Finnish National Cyber Security Centre
+---
+ src/http.c | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/src/http.c b/src/http.c
+index 5536768..dc31823 100644
+--- a/src/http.c
++++ b/src/http.c
+@@ -973,6 +973,9 @@ skip_short_body (int fd, wgint contlen, bool chunked)
+               remaining_chunk_size = strtol (line, &endl, 16);
+               xfree (line);
+ 
++              if (remaining_chunk_size < 0)
++                return false;
++
+               if (remaining_chunk_size == 0)
+                 {
+                   line = fd_read_line (fd);
+-- 
+cgit v1.0-41-gc330
diff --git a/meta/recipes-extended/wget/wget/wget-CVE-2017-13090.patch b/meta/recipes-extended/wget/wget/wget-CVE-2017-13090.patch
new file mode 100644
index 0000000..34de81e
--- /dev/null
+++ b/meta/recipes-extended/wget/wget/wget-CVE-2017-13090.patch
@@ -0,0 +1,39 @@
+From ba6b44f6745b14dce414761a8e4b35d31b176bba Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Tim=20R=C3=BChsen?= <tim.ruehsen@gmx.de>
+Date: Fri, 20 Oct 2017 15:15:47 +0200
+Subject: Fix heap overflow in HTTP protocol handling (CVE-2017-13090)
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+* src/retr.c (fd_read_body): Stop processing on negative chunk size
+
+CVE: CVE-2017-13090
+
+Upstream-Status: Backport
+
+Reported-by: Antti Levomäki, Christian Jalio, Joonas Pihlaja from Forcepoint
+Reported-by: Juhani Eronen from Finnish National Cyber Security Centre
+---
+ src/retr.c | 6 ++++++
+ 1 file changed, 6 insertions(+)
+
+diff --git a/src/retr.c b/src/retr.c
+index c1bc600..6555ed4 100644
+--- a/src/retr.c
++++ b/src/retr.c
+@@ -378,6 +378,12 @@ fd_read_body (const char *downloaded_filename, int fd, FILE *out, wgint toread,
+               remaining_chunk_size = strtol (line, &endl, 16);
+               xfree (line);
+ 
++              if (remaining_chunk_size < 0)
++                {
++                  ret = -1;
++                  break;
++                }
++
+               if (remaining_chunk_size == 0)
+                 {
+                   ret = 0;
+-- 
+cgit v1.0-41-gc330
diff --git a/meta/recipes-extended/wget/wget_1.19.1.bb b/meta/recipes-extended/wget/wget_1.19.1.bb
index 78bde95..09520ca 100644
--- a/meta/recipes-extended/wget/wget_1.19.1.bb
+++ b/meta/recipes-extended/wget/wget_1.19.1.bb
@@ -1,6 +1,8 @@
 SRC_URI = "${GNU_MIRROR}/wget/wget-${PV}.tar.gz \
            file://0001-Unset-need_charset_alias-when-building-for-musl.patch \
            file://CVE-2017-6508.patch \
+           file://wget-CVE-2017-13089.patch \
+           file://wget-CVE-2017-13090.patch \
           "
 
 SRC_URI[md5sum] = "87cea36b7161fd43e3fd51a4e8b89689"
-- 
1.9.1

Re: [PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[Alexander Kanavin]

On 10/31/2017 11:02 AM, Zhixiong Chi wrote:
> Patches from:
> http://git.savannah.gnu.org/cgit/wget.git/patch/?id=d892291fb8ace4c3b734ea5125770989c215df3f
> http://git.savannah.gnu.org/cgit/wget.git/patch/?id=ba6b44f6745b14dce414761a8e4b35d31b176bba
> 
> CVE: CVE-2017-13089 CVE-2017-13090

Update the master to 1.19.2 instead please.

Alex

✗ patchtest: failure for wget: CVE-2017-13089 and CVE-2017-13090

[Patchwork]

== Series Details ==

Series: wget: CVE-2017-13089 and CVE-2017-13090
Revision: 1
URL   : https://patchwork.openembedded.org/series/9553/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             A patch file has been added, but does not have a Signed-off-by tag [test_signed_off_by_presence] 
  Suggested fix    Sign off the added patch file (meta/recipes-extended/wget/wget/wget-CVE-2017-13089.patch)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Guidelines:     https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

Re: [PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[Zhixiong Chi]

On 2017年10月31日 17:13, Alexander Kanavin wrote:
> On 10/31/2017 11:02 AM, Zhixiong Chi wrote:
>> Patches from:
>> http://git.savannah.gnu.org/cgit/wget.git/patch/?id=d892291fb8ace4c3b734ea5125770989c215df3f 
>>
>> http://git.savannah.gnu.org/cgit/wget.git/patch/?id=ba6b44f6745b14dce414761a8e4b35d31b176bba 
>>
>>
>> CVE: CVE-2017-13089 CVE-2017-13090
>
> Update the master to 1.19.2 instead please.

Copy that, thanks for your suggestion.
>
> Alex

-- 
---------------------
Thanks,
Zhixiong Chi
Tel: +86-10-8477-7036

Re: [PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[Andre McCurdy]

On Tue, Oct 31, 2017 at 2:48 AM, Zhixiong Chi
<zhixiong.chi@windriver.com> wrote:
> On 2017年10月31日 17:13, Alexander Kanavin wrote:
>> On 10/31/2017 11:02 AM, Zhixiong Chi wrote:
>>>
>>> Patches from:
>>>
>>> http://git.savannah.gnu.org/cgit/wget.git/patch/?id=d892291fb8ace4c3b734ea5125770989c215df3f
>>> http://git.savannah.gnu.org/cgit/wget.git/patch/?id=ba6b44f6745b14dce414761a8e4b35d31b176bba
>>>
>>> CVE: CVE-2017-13089 CVE-2017-13090
>>
>> Update the master to 1.19.2 instead please.

Patching 1.19.1 does have the advantage of creating a commit which can
easily be cherry-picked into rocko (and pyro, which also uses wget
1.19.1).

Master should certainly update to 1.19.2 but doing so in two steps
might be appreciated by the stable branch maintainers.

Re: [PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[Alexander Kanavin]

On 11/02/2017 10:29 PM, Andre McCurdy wrote:
>>> Update the master to 1.19.2 instead please.
> 
> Patching 1.19.1 does have the advantage of creating a commit which can
> easily be cherry-picked into rocko (and pyro, which also uses wget
> 1.19.1).

Yes, but this is coincidental. If the versions wouldn't exactly match, 
cherry-picking would not be possible.

> Master should certainly update to 1.19.2 but doing so in two steps
> might be appreciated by the stable branch maintainers.

When fixing CVEs, the yocto branches should be considered separately, 
and patched all at the same time by the same person. For master, 
updating to latest upstream release without the vulnerability is the 
best, as it lessens the load on people who have to keep master up to 
date. For stable branches, it depends. If the upstream maintains a 
stable branch themselves where cves and other bugs are fixed, I think we 
should trust that rather than backport patches.


Alex

Re: [PATCH] wget: CVE-2017-13089 and CVE-2017-13090

[Andre McCurdy]

On Fri, Nov 3, 2017 at 2:03 AM, Alexander Kanavin
<alexander.kanavin@linux.intel.com> wrote:
> On 11/02/2017 10:29 PM, Andre McCurdy wrote:
>>>>
>>>> Update the master to 1.19.2 instead please.
>>
>> Patching 1.19.1 does have the advantage of creating a commit which can
>> easily be cherry-picked into rocko (and pyro, which also uses wget
>> 1.19.1).
>
> Yes, but this is coincidental. If the versions wouldn't exactly match,
> cherry-picking would not be possible.

Of course. But in cases where the same fix can be shared between
master and stable branches I think it makes sense to try to do so (or
at least not actively block doing so).

>> Master should certainly update to 1.19.2 but doing so in two steps
>> might be appreciated by the stable branch maintainers.
>
> When fixing CVEs, the yocto branches should be considered separately, and
> patched all at the same time by the same person.

Difficult in practice since we don't have a security team etc who can
bypass the normal flow of patches into the stable branches. It also
places an extra burden on anyone proposing updates for master that
happen to fix CVEs if they need to simultaneously create fixes for
stable branches too.

> For master, updating to
> latest upstream release without the vulnerability is the best, as it lessens
> the load on people who have to keep master up to date.

Agreed. Question is just whether (in special cases such as this one)
to make the update in master as a single commit or in two.

> For stable branches,
> it depends. If the upstream maintains a stable branch themselves where cves
> and other bugs are fixed, I think we should trust that rather than backport
> patches.

This is a more general topic. Although version updates in stable
branches is not something we do now, personally I think it's something
we should at least consider. Sometimes it makes more sense to trust
upstream than to cook up our own solutions. For anyone using OE to
actually ship a product, it's far easier to do security audits etc by
checking package versions than reviewing a long list of patches. The
counter argument though is that we just don't have resources to make
case by case decisions and a blanket policy of never updating versions
in stable branches keeps thing simple for the stable branch
maintainers.