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

[Ingo Klöcker]

> 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.

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.


- Ingo


-----------------------------------------------------------
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/20100809/829b6628/attachment.htm>