Review Request: New Kwallet scheme for Khtml user-password form saving (enabling multiple accounts per site)

Tue Aug 10 17:25:22 BST 2010

> On 2010-08-05 17:42:51, Ingo Klöcker wrote:
> > > All account usernames on the site are stored as PASSWORD value in the FormData
> > > folder of Network KWallet with the key:
> > >   accounts_SITE
> > > where SITE stands for host part of the URL.
> > 
> > I think this is a potential security problem. Let's say there are two completely different websites hosted on the same host like geocities.yahoo.com or as institutions (universities, institutes, etc.) offering personal websites for their members (like example.org/~joeuser). Let's further say both websites have a login form. If you have different usernames for both websites then you always have to double check that you do not select the wrong username because otherwise you would try to login to website A with username _and_ password of website B. Ouch! Moreover, if you use the same username for both websites then you are completely screwed because AFAIU your patch this is not supported resp. without noticing Konqueror might send the password of website A to website B.
> > 
> > Either I misunderstood what your patch does or your patch is IMHO unacceptable because of the above.
> 
> Carewolf wrote:
>     Are you objecting to the sharing of username/password across multiple pages within the same host? I can see the potential usability issue if the default isn't remembered separate for each page, but I am not so sure of the security issue. If I remember correctly, when webpages are sharing the same host, domain and IP-adress, they are not protected from cross-scripting either and can also access the same cookies. They are for most purposes the same security domain. Though https will have to be treated differently.
> 
> Ingo Klöcker wrote:
>     Yes, that's what I'm objecting to. IMO it is a security issue, but it's true that the patch wouldn't really make it much worse because it's already bad enough due to the possibility of cross-site-scripting, cookies and what not. Ignore security still leaves us with a potential usability issue as you correctly point out.
> 
> Dawit Alemayehu wrote:
>     For the record, there is no problem with cookies when hosting sites offer their members websites that have the same host but different path, e.g. "foo.bar/johndoe" and "foo.bar/janedoe". That is because the path is also taken into consideration when storing/retrieving cookies...
> 
> Michael Leupold wrote:
>     The new Secret Service API will feature an attribute-based mechanism to find secrets instead of the name-based approach KWallet uses, eg.
>     "account_www.kde.org_lemma" would instead be {"host" => "www.kde.org", "user" => "lemma"} or similar. To make migration easier I'd suggest replacing the underscore ('_') to separate the parts of the keys by something that can't be part of a QUrl, eg. the pipe character ('|').
>     
>     Apart from that while I honor convenience aspects with regards to reusing passwords on multiple pages of a site, I second Ingo's security concerns, especially if passwords are filled in automatically when the site is loaded. Mozilla had a related security issue which contains some discussion and some ideas on how to solve it: https://bugzilla.mozilla.org/show_bug.cgi?id=360493 (note that the bug starts out with a form calling an action on another site, yet many of the concerns on that bug are valid here as well IMO).

I just thought about the redundant storage of usernames in the accounts_SITE list as well as in the key of account_SITE_USERNAME. You can avoid that by using the regexp feature of Wallet::readPasswordList() aka Wildcard support. It would work like this:
- accounts_SITE is just an empty entry that exists as long as account_SITE_USERNAME entries are available.
- Konqueror checks the availability of accounts_SITE to see if there are logins available and proceeds to open the wallet if there are.
- Konqueror can retrieve all passwords using readPasswordList("account_SITE_*", resultMap);

I don't recall seeing this feature being used anywhere but George Staikos seems to have implemented it for that very purpose.

- Michael

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4907/#review6824
-----------------------------------------------------------

On 2010-08-05 12:33:43, Filip Brcic wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4907/
> -----------------------------------------------------------
> 
> (Updated 2010-08-05 12:33:43)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch adds support for storing multiple web-site accounts in KWallet when using Khtml-based browser (=Konqueror).
> 
> Originally Khtml saved whole forms (containing at least one password field) in KWallet as a map. Support for that behavior is still included, but new approach takes precedence.
> 
> Key features of this new approach are the following:
> 
> Login page is identified by having 2 form input fields, one that is plain text
> field, and the other that is the password field. All other form types are
> left to be handled by the original map-based approach.
> 
> Login on one site is universal on that site. That means that if you can login using
> multiple URLs (like on bugs.kde.org), the login data will be stored only once
> and used on all login pages.
> 
> All account usernames on the site are stored as PASSWORD value in the FormData
> folder of Network KWallet with the key:
> 
> accounts_SITE
> 
> where SITE stands for host part of the URL.
> 
> All passwords are stored as PASSWORD value in the FormData folder of the
> Network KWallet with the key:
> 
> account_SITE_USERNAME
> 
> When you come to the SITE's login page, the system offers you to autocomplete
> username. When you select some username from the autocomplete list, the system
> autofills the correct password.
> 
> No username if prefilled, but that could be easily done (for example, by
> selecting the first username from the accounts list or by keeping record which
> username was last used).
> 
> 
> This addresses bug 72317.
>     https://bugs.kde.org/show_bug.cgi?id=72317
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/khtml/html/html_formimpl.h 1159459 
>   /trunk/KDE/kdelibs/khtml/html/html_formimpl.cpp 1159459 
>   /trunk/KDE/kdelibs/khtml/khtml_part.h 1159459 
>   /trunk/KDE/kdelibs/khtml/khtml_part.cpp 1159459 
>   /trunk/KDE/kdelibs/khtml/khtml_wallet_p.h 1159459 
>   /trunk/KDE/kdelibs/khtml/rendering/render_form.cpp 1159459 
>   /trunk/KDE/kdelibs/khtml/ui/passwordbar/storepassbar.h 1159459 
>   /trunk/KDE/kdelibs/khtml/ui/passwordbar/storepassbar.cpp 1159459 
> 
> Diff: http://reviewboard.kde.org/r/4907/diff
> 
> 
> Testing
> -------
> 
> I made this patch from the current kdelibs trunk checkout, yet I tested it also
> on kdelibs 4.3.3 and it applies correctly (actually it doesn't since the
> copyright comments don't apply correctly, but if you ignore those few errors as
> they only relate to the comments, not the code, the patch does apply). Therefore, it is safe to
> assume this patch works on all versions of kdelibs from 4.3.3 to trunk
> (possibly with some older versions, but I didn't check that).
> 
> Also, I ran the patched kdelibs for two weeks and it works well.
> 
> 
> Screenshots
> -----------
> 
> Here's how it looks like in KWallet
>   http://reviewboard.kde.org/r/4907/s/471/
> 
> 
> Thanks,
> 
> Filip
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100810/c97a5bbc/attachment.htm>