Re: [Lynx-dev] [oss-security] Re: Bug#991971: bug in Lynx' SSL certificate validation -> leaks password in clear text via SNI (under some circumstances)

[Axel Beckert]

Hi Salvatore, Dear Ariadne,

Salvatore Bonaccorso wrote:
> > This is more severe than it initially looked like: Due to TLS Server
> > Name Indication (SNI) the hostname as parsed by Lynx (i.e with
> > "user:pass@" included) is sent in _clear_ text over the wire even
> > _before_ I can even said "n" for "no, don't continue to talk with this
> > server" in Lynx's prompt as shown above.
[…]
> > IMHO this nevertheless needs a CVE-ID.
>
> MITRE did assign CVE-2021-38165.

Thanks Salvatore. I updated the debian/changelog entry for the next
upload as well as the title of the Debian bug report.

> MITRE raised the question: Does 2.9.0dev.9 (mentioned on the
> https://lynx.invisible-island.net/current/CHANGES.html page) fix the
> entire problem?

At this point a huge thanks to Thomas Dickey (Lynx upstream) for
providing a fixed version so quickly!

> https://www.openwall.com/lists/oss-security/2021/08/07/7 claims that
> credentials appear in the HTTP Host header to an http:// (i.e.,
> non-SSL) website. 

Indeed and a good point.

Citing from Ariadne's mail:
> The issue itself is far more severe: HTParse() does not understand
> the authn part of the URI at all.
[…]
> But it will also leak in the Host: header on unencrypted
> connections, and also probably SSL ones too.

But that looks to me as if Ariadne just refers to the code and hasn't
actually checked it by trying it. Nevertheless thanks to Ariadne for
having had a look and proposing a patch!

To be on the safe side I tested Lynx 2.9.0dev.6-2 as in Debian
Unstable/Testing (yet unpatched for CVE-2021-38165) and the (claimed
to be fixed) Lynx 2.9.0dev.9-1 as uploaded to Debian Experimental by
Andreas Metzler very recently (still only on incoming.debian.org,
fetched it from there). And I also tested Lynx 2.8.9dev1 from Debian 8
Jessie ELTS, the oldest compiled version of Lynx I could easily get my
hands on.

Neither of them leaked the user name or password in Host header of a
plain HTTP request. (So I assume that the versions inbetween don't do
that either.)

I also kinda would have expected that if this would have been the case
in the plain HTTP Host header, it would have been noticed way earlier
than with the TLS SNI extension, namely before the HTTPS era. Then
again, the much more obvious facet of this issue which Thorsten
initially found was reported way later than I'd expected, so maybe my
gut feeling is wrong here, too. :-)

So I also looked in the code (of the unpatched 2.9.0dev.8). The patch
for 2.9.0dev.9 added a function StripUserAuthents and added it only
into the HTTPS code path. So I looked for "strip" and "Strip" in the
HTTP code path and found this code in WWW/Library/Implementation/HTTP.c
(which also has the HTTPS code btw.):

   1353         if ((host = HTParse(anAnchor->address, "", PARSE_HOST)) != 
NULL) {
-> 1354             strip_userid(host, TRUE);
-> 1355             HTBprintf(&command, "Host: %s%c%c", host, CR, LF);
   1356             FREE(host);
   1357         }

So from my point of view, this also invalidates that claim that the
HTTP Host header contains username or password. HTParse (as mentioned
by Ariadne occurs quite often in the code and its exact workings are
unclear to me from just looking at how it is called as it seems very
variable in what it parses and it appears before and after the above
mentioned code snippet for printing the Host header.

$ fgrep -n 'HTParse(' WWW/Library/Implementation/HTTP.c
891:        connect_host = HTParse(connect_url, "https", PARSE_HOST);
900:        connect_host = HTParse(connect_url, "snews", PARSE_HOST);
977:    ssl_host = HTParse(url, "", PARSE_HOST);
1319:   char *p1 = (HTParse(url, "", PARSE_PATH | PARSE_PUNCTUATION));
1371:   if ((host = HTParse(anAnchor->address, "", PARSE_HOST)) != NULL) {
1564:       abspath = HTParse(arg, "", PARSE_PATH | PARSE_PUNCTUATION);
1565:       docname = HTParse(arg, "", PARSE_PATH);
1566:       hostname = HTParse(arg, "", PARSE_HOST);
1590:           host2 = HTParse(docname, "", PARSE_HOST);
1591:           path2 = HTParse(docname, "", PARSE_PATH | PARSE_PUNCTUATION);
2370:                       !HTAA_HaveUserinfo(HTParse(arg, "", PARSE_HOST)) &&

So from my point of view the user and password are stripped only once
(in 2.9.0dev8), namely directly before printing the Host header (in
both HTTP and HTTPS code paths). Which also seems the reason why it
got forgotten for SNI and why Thomas added a new, way less complex
function for stripping them of the SNI header. Calling strip_userid
again would extract the credentials again which likely isn't wanted.
(The same IMHO applies to the often called HTParse function.)

I assume that Ariadne just oversaw that call to strip_userid just
before printing the HTTP Host header when looking at the code. (And I
clearly had the advantage of having looked at the code _after_ the
official upstream patch has been published, so I clearly had an
advantage when looking at the code, too.)

I though didn't check the strip_userid function in detail as it's way
more complex than the rather simple StripUserAuthents function added
in 2.9.0dev.9. This likely because strip_userid does not only strip
those credentials, it also seems to extract them. And it clearly looks
for an "@" as delimiter. Together with the PCAPs of Lynx HTTP traffic
I analysed, I'm quite confident, that it's fine.

                Regards, Axel
-- 
 ,''`.  |  Axel Beckert <abe@debian.org>, https://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-    |  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

signature.asc
Description: PGP signature