GNOME Bugzilla – Bug 697695
Avatar cache is not used
Last modified: 2014-08-13 23:07:39 UTC
EDS backend writes all avatars into a file cache, but never reads that cache. It seems to be useless work. Also there seems to be no protection against external URI avatars. If I import a VCard into EDS that have PHOTO=http://attacker.com/bigfile.png then any folks app will start loading into memory potentially gigabytes of data. If that happens on 3G the user could be have a debt for the rest of his life. That would happen in background with no notification to user and no way to stop it. ... unless there is something I did not understand in the code, of course.
(In reply to comment #0) > EDS backend writes all avatars into a file cache, but never reads that cache. > It seems to be useless work. Oh no. That’s pretty bad, and it’s all my fault: https://git.gnome.org/browse/folks/commit/?id=3862f876. Looks like load_avatar() was never used from the beginning. > Also there seems to be no protection against external URI avatars. If I import > a VCard into EDS that have PHOTO=http://attacker.com/bigfile.png then any folks > app will start loading into memory potentially gigabytes of data. That’s pretty bad. Assuming the above bug were fixed, the avatar would only be downloaded once (then cached), but that’s still bad. I guess the only solution is to check the avatar file’s size first, and only download it if it’s smaller than something reasonable (100 KiB? arbitrary figure pulled from nowhere); otherwise just print a warning and set that persona’s avatar to null. Sorry.
(In reply to comment #1) > (In reply to comment #0) > That’s pretty bad. Assuming the above bug were fixed, the avatar would only be > downloaded once (then cached), but that’s still bad. I guess the only solution > is to check the avatar file’s size first, and only download it if it’s smaller > than something reasonable (100 KiB? arbitrary figure pulled from nowhere); > otherwise just print a warning and set that persona’s avatar to null. I'm not sure you can know the size of the file before starting downloading it, or at least in a way that malicious server cannot fake. For now I would just ignore any avatar URI which does not have "file://" prefix, I think it is the safest and easiest solution for a serious security issue. Later we could think about downloading the first 100Kb and if we don't hit EOF before that then cancel the download. Of course it would need on disk cache.
As pohly pointed out, the cache has also the problem of duplicating on disk avatars which are already locally stored with file:// URI.
I suggest the KISS solution here: 1) simply drop folks' avatar cache all together 2) ignore all avatars that are not file:// URI because other URI have security issues, and inlined avatars should be cached locally by EDS itself. 3) thanks to point 2, we can drop MemoryIcon from folks, because icons will always be a local file. 4) Open a bug against Google EDS abook because they give inlined avatars instead of caching on disk. I think it's better to do the caching there than in folks, because that would avoid having all avatar data going over DBus which I'm sure makes things slower. Ok ?
(In reply to comment #4) > 1) simply drop folks' avatar cache all together Yes, as long as we can guarantee that all backends will only expose avatars which are file:// URIs. The only reason AvatarCache exists is because we have “backends where retrieving avatars is an expensive operation”. > 2) ignore all avatars that are not file:// URI because other URI have security > issues, and inlined avatars should be cached locally by EDS itself. Yes. > 3) thanks to point 2, we can drop MemoryIcon from folks, because icons will > always be a local file. Yes. > 4) Open a bug against Google EDS abook because they give inlined avatars > instead of caching on disk. I think it's better to do the caching there than in > folks, because that would avoid having all avatar data going over DBus which > I'm sure makes things slower. Yes.
*** Bug 696220 has been marked as a duplicate of this bug. ***
(In reply to comment #4) > I suggest the KISS solution here: > 1) simply drop folks' avatar cache all together Not possible, because it’s (correctly) used in the Tracker backend, and could also be added to the BlueZ backend in future to speed that up (bug #734751). > 2) ignore all avatars that are not file:// URI because other URI have security > issues, and inlined avatars should be cached locally by EDS itself. Patch coming. > 3) thanks to point 2, we can drop MemoryIcon from folks, because icons will > always be a local file. Already dropped (in favour of GLib.BytesIcon): bug #705403. > 4) Open a bug against Google EDS abook because they give inlined avatars > instead of caching on disk. I think it's better to do the caching there than in > folks, because that would avoid having all avatar data going over DBus which > I'm sure makes things slower. You already opened bug #710826! \o/
So I think this can be FIXED. commit 97474e2016882c616452095312b0922f3045da4a Author: Philip Withnall <philip@tecnocode.co.uk> Date: Wed Aug 13 23:58:41 2014 +0100 eds: Remove use of the AvatarCache For several reasons: 1) Now that only local URIs are permitted for URI-based photos, loading the photos should no longer be slow and need caching. 2) Inline photos don’t need caching (and if they do, it should be in EDS, not folks — see bug #710826). 3) The cached photos were never actually loaded again. How embarrassing. https://bugzilla.gnome.org/show_bug.cgi?id=697695 backends/eds/lib/edsf-persona.vala | 35 ++++------------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) commit 3caef48df30d25cb3445d3b45551c6ccb3656d23 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Wed Aug 13 23:57:18 2014 +0100 eds: Ignore non-local URIs for photos for security reasons We can’t entirely trust the URIs provided to us in contacts, and they could reference huge photos which we really don’t want to download (and incur huge bandwidth bills). Since non-local URIs are really rare, it seems reasonable to ignore them entirely, avoiding the security issue. https://bugzilla.gnome.org/show_bug.cgi?id=697695 backends/eds/lib/edsf-persona.vala | 13 +++++++++++++ 1 file changed, 13 insertions(+)