Bug 1415298

Thanks for the analysis!  I see there was a regression fix upstream:

https://github.com/curl/curl/commit/curl-7_49_0~79

... which refers to the following issue:

https://github.com/curl/curl/pull/655

Could you please check whether the fix has any effect on the bug in question?

I can prepare a scratch build if needed...

(In reply to Kamil Dudka from comment #2)
> Thanks for the analysis!  I see there was a regression fix upstream:
> 
> https://github.com/curl/curl/commit/curl-7_49_0~79
> 
> ... which refers to the following issue:
> 
> https://github.com/curl/curl/pull/655
> 
> Could you please check whether the fix has any effect on the bug in question?

  Unfortunately the error happened again after enabling
back curl-7.19.7-CVE-2015-3148.patch and adding the
suggested patch.

> I can prepare a scratch build if needed...

  Patch was rediffed as:
"""
+diff -up curl-7.19.7/lib/http.c.orig curl-7.19.7/lib/http.c
+--- curl-7.19.7/lib/http.c.orig        2017-01-23 15:51:19.810640755 -0200
++++ curl-7.19.7/lib/http.c     2017-01-23 15:54:57.269206226 -0200
+@@ -1922,8 +1922,10 @@ CURLcode Curl_http_done(struct connectda
+      data->state.negotiate.state == GSS_AUTHSENT) {
+     /* add forbid re-use if http-code != 401/407 as a WA only needed for
+      * 401/407 that signal auth failure (empty) otherwise state will be RECV
+-     * with current code */
+-    if((data->req.httpcode != 401) && (data->req.httpcode != 407))
++     * with current code.
++     * Do not close CONNECT_ONLY connections. */
++    if((data->req.httpcode != 401) && (data->req.httpcode != 407) &&
++       !data->set.connect_only)
+       conn->bits.close = TRUE; /* Negotiate transfer completed */
+     Curl_cleanup_negotiate(data);
+   }
"""

Thanks for checking!  I guess you have some local environment to test it.  Could you please check whether the issue occurs also with RHEL-7 or Fedora (lib)curl?

  I do not have a test environment. But I build a rhel6 backport
of latest rawhide curl, and asked the user to test it, and revert
back after testing to a rhel6 supported version.
  It did work.
  Build used this patch:
"""
--- curl.spec.orig	2017-01-24 16:30:03.634274220 -0200
+++ curl.spec	2017-01-24 16:30:40.759173884 -0200
@@ -20,12 +20,14 @@
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(id -nu)
 BuildRequires: groff
 BuildRequires: krb5-devel
+%if 0%{?fedora} != 0 || 0%{?rhel} > 6
 BuildRequires: libidn2-devel
 BuildRequires: libmetalink-devel
 BuildRequires: libnghttp2-devel
 BuildRequires: libpsl-devel
 BuildRequires: libssh2-devel
 BuildRequires: multilib-rpm-config
+%endif
 BuildRequires: nss-devel
 BuildRequires: openldap-devel
 BuildRequires: openssh-clients
@@ -35,8 +37,10 @@
 BuildRequires: stunnel
 BuildRequires: zlib-devel
 
+%if 0%{?fedora} != 0 || 0%{?rhel} > 6
 # nghttpx (an HTTP/2 proxy) is used by the upstream test-suite
 BuildRequires: nghttp2
+%endif
 
 # perl modules used in the test suite
 BuildRequires: perl(Cwd)
@@ -65,9 +69,11 @@
 
 Requires: libcurl%{?_isa} = %{version}-%{release}
 
+%if 0%{?fedora} != 0 || 0%{?rhel} > 6
 # require at least the version of libssh2 that we were built against,
 # to ensure that we have the necessary symbols available (#525002, #642796)
 %global libssh2_version %(pkg-config --modversion libssh2 2>/dev/null || echo 0)
+%endif
 
 %description
 curl is a command line tool for transferring data with URL syntax, supporting
@@ -82,9 +88,11 @@
 Group: Development/Libraries
 Requires: libssh2%{?_isa} >= %{libssh2_version}
 
+%if 0%{?fedora} != 0 || 0%{?rhel} > 6
 # libnsspem.so is no longer included in the nss package (#1347336)
 BuildRequires: nss-pem
 Requires: nss-pem
+%endif
 
 %description -n libcurl
 libcurl is a free and easy-to-use client-side URL transfer library, supporting
@@ -150,11 +158,13 @@
     --enable-threaded-resolver \
     --with-ca-bundle=%{_sysconfdir}/pki/tls/certs/ca-bundle.crt \
     --with-gssapi${KRB5_PREFIX} \
+%if 0%{?fedora} != 0 || 0%{?rhel} > 6
     --with-libidn2 \
     --with-libmetalink \
     --with-libpsl \
     --with-libssh2 \
     --with-nghttp2 \
+%endif
     --without-ssl --with-nss
 #    --enable-debug
 # use ^^^ to turn off optimizations, etc.
@@ -166,6 +176,7 @@
 
 make %{?_smp_mflags} V=1
 
+%if 0
 %check
 # we have to override LD_LIBRARY_PATH because we eliminated rpath
 LD_LIBRARY_PATH="$RPM_BUILD_ROOT%{_libdir}:$LD_LIBRARY_PATH"
@@ -177,6 +188,7 @@
 
 # run the upstream test-suite
 ./runtests.pl -a -p -v '!flaky'
+%endif
 
 %install
 rm -rf $RPM_BUILD_ROOT
@@ -193,8 +205,10 @@
 install -d $RPM_BUILD_ROOT%{_datadir}/aclocal
 install -m 644 docs/libcurl/libcurl.m4 $RPM_BUILD_ROOT%{_datadir}/aclocal
 
+%if 0%{?fedora} != 0 || 0%{?rhel} > 6
 # Make libcurl-devel multilib-ready (bug #488922)
 %multilib_fix_c_header --file %{_includedir}/curl/curlbuild.h
+%endif
 
 %clean
 rm -rf $RPM_BUILD_ROOT
"""

Thanks!  Hopefully it means that the regression has been already fixed upstream.  If we do not have any in house reproducer, I have no idea how to further debug it, except bisecting further to the potential fix.

Could you please check whether it works with curl-7.42.0-1.fc23 (the first upstream release including the fix for CVE-2015-3148)?

https://koji.fedoraproject.org/koji/buildinfo?buildID=629950
http://pkgs.fedoraproject.org/cgit/rpms/curl.git/tree/curl.spec?id=13074767

I suspect that this regression could be related to the following sentence from the security advisory [1]:

    "We decided to do this blunt short-term fix to avoid this current problem
    and then continue to work on a better fix."

[1] https://curl.haxx.se/docs/adv_20150422B.html

  It works with curl-7.42.0-1.fc23 backported to rhel6.

Cool.  This means that we must be missing some commit that landed _before_ the actual fix for CVE-2015-3148.  Could you please check it with curl-7.29.0-35.el7 (the latest RHEL-7 build)?

It works with curl-7.29.0-35.el7, rebuilt as
curl-7.29.0-35.el6.x86_64 as well.
Indeed, should be a single/minor missing patch in
rhel6 curl-7.19.7-46.el6.x86_64 or newer.

Created attachment 1248648 [details]
curl-7.19.7-sfdc01750392.patch

  ***Note, this is not a proposed patch but hopefully
can help in getting some insight of where else to
look for a possible patch to correct the problem***

  The attached patch was an attempt on backporting
some curl-7.29.0-35.el7 changes to curl-7.19.7-52.el6,
but still, known that backport of curl-7.29.0-35.el7
to rhel6 works, and last rhel6 package that works is
rhel6 curl-7.19.7-45.el6.x86_64

  The attached patch is in hackish mode, as was added
to test if it were the new code under NTLM_WB_ENABLED
preprocessor option that was the cause of the rhel7
package working.

Thanks for the patch!

Do I understand it correctly that the latest el6 curl works as expected if you apply attachment #1248648 [details] on top of our downstream patches?

  No, I am afraid it does work.

  I just added it to show what was my best guess of
what could resolve the problem, but still did not work,
but hopefully get some feedback about it :)

  My next step is to just run latest rhel6 curl under gdb
in a remote support session, and attempt to find out where
and why it fails.

Kamil,

Running under gdb, if I set a breakpoint at
curl-7.19.7/lib/http_negotiate.c:340
like this:

338:   else
339:     conn->allocptr.userpwd = userp;
340:   free(encoded);
341:   return (userp == NULL) ? CURLE_OUT_OF_MEMORY : CURLE_OK;

and in the second time it stops there I call
Curl_cleanup_negotiate like this:

(gdb) call Curl_cleanup_negotiate (conn->data)
(gdb) continue

it works.

  Trying to figure some condition, with some obfuscated data,
I cannot see of any condition to check, to make the call
conditional, other than it had a redirect...

Breakpoint 2, Curl_output_negotiate (conn=0x635500, proxy=false) at http_negotiate.c:340
340       free(encoded);
(gdb) p conn.data.state
$6 = {used_interface = Curl_if_easy, connc = 0x62f150, keeps_speed = {tv_sec = 0, tv_usec = 0}, lastconnect = 0,
  headerbuff = 0x62dad0 "\r\n", headersize = 256,
  buffer = "HTTP/1.1 307 TEMPORARY_REDIRECT\r\nCache-Control: no-cache\r\nExpires: Fri, 10 Feb 2017 15:29:35 GMT\r\nDate: Fri, 10 Feb 2017 15:29:35 GMT\r\nPragma: no-cache\r\nExpires: Fri, 10 Feb 2017 15:29:35 GMT\r\nDate: F"...,
  uploadbuffer = '\000' <repeats 16384 times>, current_speed = -1, this_is_a_follow = true,
  first_host = 0x630320 "EDITED.EXAMPLE.COM", session = 0x62f370, sessionage = 1, tempwrite = 0x0, tempwritesize = 0,
  tempwritetype = 0, scratch = 0x0, errorbuf = false, os_errno = 0, prev_signal = 0, allow_port = false, digest = {
    nonce = 0x0, cnonce = 0x0, realm = 0x0, algo = 0, stale = false, opaque = 0x0, qop = 0x0, algorithm = 0x0, nc = 0},
  proxydigest = {nonce = 0x0, cnonce = 0x0, realm = 0x0, algo = 0, stale = false, opaque = 0x0, qop = 0x0, algorithm = 0x0,
    nc = 0}, negotiate = {state = GSS_AUTHNONE, gss = false, protocol = 0x7ffff7dcf9e7 "Negotiate", status = 0,
    context = 0x631850, server_name = 0x630020, output_token = {length = 1463, value = 0x6343b0}}, proxyneg = {
    state = GSS_AUTHNONE, gss = false, protocol = 0x0, status = 0, context = 0x0, server_name = 0x0, output_token = {
      length = 0, value = 0x0}}, authhost = {want = 4, picked = 4, avail = 0, done = true, multi = false, iestyle = false},
  authproxy = {want = 1, picked = 1, avail = 0, done = true, multi = false, iestyle = false}, authproblem = false,
  expiretime = {tv_sec = 0, tv_usec = 0}, timenode = {smaller = 0x0, larger = 0x0, same = 0x0, key = {tv_sec = 0,
      tv_usec = 0}, payload = 0x0}, most_recent_ftp_entrypath = 0x0, ftp_trying_alternative = false, httpversion = 11,
  expect100header = false, pipe_broke = false, prev_block_had_trailing_cr = false, crlf_conversions = 0, shared_conn = 0x0,
  closed = false, pathbuffer = 0x62fd60 "/EDITED/PATH",
  path = 0x62fd60 "/EDITED/PATH", use_range = false, rangestringalloc = false,
  range = 0x0, resume_from = 0, proto = {http = 0x630290, https = 0x630290, ftp = 0x630290, file = 0x630290,
    telnet = 0x630290, generic = 0x630290, ssh = 0x630290}, current_conn = 0x635500}

  I believe this is not correct, at least in the sense of a backport:
https://github.com/curl/curl/commit/f78ae415d24b9bd89d6c121c556e411fdb21c6aa

and, not fully understanding the CVE, it looks like it is even making the
CVE condition worse:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-3148

by keeping the authentication, but in the user case, failing, apparently
after the redirect.

  So, if reverting this chunk:
diff --git a/lib/http_negotiate.c b/lib/http_negotiate.c
index 535a427..b56e7d0 100644
--- a/lib/http_negotiate.c
+++ b/lib/http_negotiate.c
@@ -338,7 +338,6 @@ CURLcode Curl_output_negotiate(struct connectdata *conn, bool proxy)
   else
     conn->allocptr.userpwd = userp;
   free(encoded);
-  Curl_cleanup_negotiate (conn->data);
   return (userp == NULL) ? CURLE_OUT_OF_MEMORY : CURLE_OK;
 }
 
-- 
it works for the user.

Fixed as CVE-2017-2628 via RHSA-2017:0847:
https://rhn.redhat.com/errata/RHSA-2017-0847.html