Security advisory from KDE upstream

I just received a notification from the ressource which read "internal server error" and the url https://username:<email address hidden>/remote.php.carddav...

I believe it is not a good idea to have a password in a notication.

Reproducible: Always

somewhere a message is using url() rather than prettyUrl().

but so far I haven't had any luck finding where in the code.

maybe another set of eyes will have more luck. should be an easy fix once we find the offending text.

We need a screenshot or exact error message to find it.

Is there a way to provoke a connection error? It doesn't work when just disconnecting the internet. The cause must have been on the server side, so I will only see the message again, when I can fake a server error or it happens again.

I think this was introduced by 649a97d08771020a4e5151bbc041e82405f5841c, at least that the only commit I can thin of that touched the error messages. If true, there are some chances that the issue comes from KIO.

Looks like the source is in kdelibs/kioslave/http/http.cpp:3059, where url() is used instead of prettyUrl() as the error message.

Do you think this can go into kdelibs for 4.10.4?

yes please.

looks like changing that line to use m_request.url.host() might be the correct solution.

In fact, once this is fixed I'll send a note to the packages that they might want to hotpatch their 4.10.3 releases.

(In reply to comment #6)
> looks like changing that line to use m_request.url.host() might be the
> correct solution.

Having the full URL that triggered this error would help finding the issue, so I'm not certain that just keeping the hostname would be satisfying to most users. As for the usage of this string there's a new line between 'Internal error in server' and the error text, which makes mes doubt that %1 stands for the hostname in the full message.

Otherwise, looking around this line the m_request.url.url() is also used in the same way (lines 3075 and 3077). I'll also replace those with prettyUrl().

Git commit 65d736dab592bced4410ccfa4699de89f78c96ca by Grégory Oestreicher.
Committed on 08/05/2013 at 23:16.
Pushed by goestreicher into branch 'KDE/4.10'.

Don't show passwords contained in HTTP URLs in error messages

M +3 -3 kioslave/http/http.cpp

http://commits.kde.org/kdelibs/65d736dab592bced4410ccfa4699de89f78c96ca

While working on a fix for the Fedora kdelibs3 compatibility package, I noticed that your fix for 4.10 is NOT complete: There are at least 2 instances where url() (rather than prettyUrl()) is still used in error messages!

https://projects.kde.org/projects/kde/kdelibs/repository/entry/kioslave/http/http.cpp?rev=KDE%2F4.10#L1582
This one looks particularly weird: Only if the URL is NOT null, it gets replaced with the default??? I think the ! there is too much. But the main issue is that it uses url() and (later in the function) prints the thing.

https://projects.kde.org/projects/kde/kdelibs/repository/entry/kioslave/http/http.cpp?rev=KDE%2F4.10#L3467
And this one shouldn't need any explanation of why it's bad.

Created attachment 79887
kdelibs-3.5.10-CVE-2013-2074.patch

For reference (and for other distros which still ship kdelibs3), here's my tentative kdelibs3 patch. In addition to what you fixed, this patch also fixes the debugging output (fixed in 4.x by an earlier commit), the 2 spots I pointed out above, and 1 additional one I can't find in the 4.10 code (the error(ERR_ACCESS_DENIED, u.url()); line – it apparently got rewritten or removed).

(In reply to comment #9)
> While working on a fix for the Fedora kdelibs3 compatibility package, I
> noticed that your fix for 4.10 is NOT complete: There are at least 2
> instances where url() (rather than prettyUrl()) is still used in error
> messages!

Wow, thanks a lot for that.

> https://projects.kde.org/projects/kde/kdelibs/repository/entry/kioslave/http/
> http.cpp?rev=KDE%2F4.10#L1582
> This one looks particularly weird: Only if the URL is NOT null, it gets
> replaced with the default???

I think that url can be null is _url cannot be converted, so it looks legitimate. However you're right, prettyUrl() should indeed be used.

> https://projects.kde.org/projects/kde/kdelibs/repository/entry/kioslave/http/
> http.cpp?rev=KDE%2F4.10#L3467
> And this one shouldn't need any explanation of why it's bad.

Indeed.

Thanks a lot for you help, I'll fix those.

Cheers,
Grégory

Git commit 898135a59d91184692ed1bcee8bb4c6d80d6f7b9 by Grégory Oestreicher.
Committed on 15/05/2013 at 21:56.
Pushed by goestreicher into branch 'KDE/4.10'.

Continue hiding passwords in URLs displayed to the user
The fix introduced by 65d736da missed two usages of
url() instead of prettyUrl(). Thanks to Kevin Kofler
for spotting those.

M +2 -2 kioslave/http/http.cpp

http://commits.kde.org/kdelibs/898135a59d91184692ed1bcee8bb4c6d80d6f7b9

> I think that url can be null is _url cannot be converted, so it looks legitimate.

1. There's no conversion there. It's just a copy from a const QString & to a writable QString.
2. The thing there is that it checks for if (!url.isNull()) when I think it wants to actually check for if (url.isNull()) without the '!' (or maybe actually for if (url.isEmpty()), again without negation; checking strings for isNull is not recommended and rarely needed). The code looks to me like the intention was to fill in a default value if no URL was given, whereas as written now, it leaves it empty if it was empty and it overwrites it with the default value if it was not empty.