Bug 319428 - notifications about errors contain password
Summary: notifications about errors contain password
Status: RESOLVED FIXED
Alias: None
Product: kio
Classification: Frameworks and Libraries
Component: http (show other bugs)
Version: 4.10.3
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-06 20:06 UTC by m.wege
Modified: 2013-05-15 22:01 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
kdelibs-3.5.10-CVE-2013-2074.patch (6.04 KB, patch)
2013-05-14 16:20 UTC, Kevin Kofler
Details

Note You need to log in before you can comment on or make changes to this bug.
Description m.wege 2013-05-06 20:06:29 UTC
I just received a notification from the ressource which read "internal server error" and the url https://username:password@serveradress.com/remote.php.carddav...

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

Reproducible: Always
Comment 1 Allen Winter 2013-05-07 14:02:45 UTC
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.
Comment 2 Laurent Montel 2013-05-07 17:10:42 UTC
We need a screenshot or exact error message to find it.
Comment 3 m.wege 2013-05-07 21:19:16 UTC
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.
Comment 4 Grégory Oestreicher 2013-05-08 20:01:57 UTC
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.
Comment 5 Grégory Oestreicher 2013-05-08 20:14:48 UTC
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?
Comment 6 Allen Winter 2013-05-08 20:57:40 UTC
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.
Comment 7 Grégory Oestreicher 2013-05-08 21:13:39 UTC
(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().
Comment 8 Grégory Oestreicher 2013-05-08 21:38:57 UTC
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
Comment 9 Kevin Kofler 2013-05-14 15:51:29 UTC
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.
Comment 10 Kevin Kofler 2013-05-14 16:20:30 UTC
Created attachment 79887 [details]
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).
Comment 11 Grégory Oestreicher 2013-05-15 19:54:43 UTC
(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
Comment 12 Grégory Oestreicher 2013-05-15 19:58:18 UTC
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
Comment 13 Kevin Kofler 2013-05-15 22:01:21 UTC
> 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.