Bug 142986

Summary: [Seccomp] Canonicalize filesystem path when whitelisting it
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKit2Assignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: mcatanzaro, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 140072, 142987    
Attachments:
Description Flags
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it
none
[Linux] Improvements to SyscallPolicy
none
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it
none
Patch zan: review+

Description Michael Catanzaro 2015-03-23 14:59:46 PDT
We need to allow symlinks in our whitelist to allow whitelisting locations like /etc/localtime that could be a symlink to anywhere. Currently symlinks don't work because they're followed by the code that checks if access is permitted, so also follow them when adding the permission.

Security consequence: an attacker that has already owned your computer can give the web process additional permissions by creating a symlink from a permissible loaction to an impermissible location. (Not a problem.)
Comment 1 Michael Catanzaro 2015-03-23 15:02:39 PDT
Created attachment 249279 [details]
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it
Comment 2 Michael Catanzaro 2015-03-23 15:10:45 PDT
Created attachment 249281 [details]
[Linux] Improvements to SyscallPolicy
Comment 3 Michael Catanzaro 2015-03-23 15:18:33 PDT
Comment on attachment 249281 [details]
[Linux] Improvements to SyscallPolicy

Dammit, wrong bug again... this time it's my fault for typing the bug number wrong, though.
Comment 4 Michael Catanzaro 2015-03-23 15:23:39 PDT
Created attachment 249285 [details]
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it
Comment 5 Zan Dobersek 2015-07-22 09:17:47 PDT
Comment on attachment 249285 [details]
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it

View in context: https://bugs.webkit.org/attachment.cgi?id=249285&action=review

> Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.h:59
> +    static void addPermissionToMap(const String& path, Permission, PermissionMap&);

Why use a private static method? Why not just implement this as a normal helper method?
Comment 6 Michael Catanzaro 2015-07-22 09:21:25 PDT
I only see one good reason: to keep SyscallPolicy::PermissionMap private.
Comment 7 Zan Dobersek 2015-07-22 09:45:42 PDT
But the helper method can be private, just as the static one is now.
Comment 8 Zan Dobersek 2015-07-22 09:50:16 PDT
Comment on attachment 249285 [details]
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it

View in context: https://bugs.webkit.org/attachment.cgi?id=249285&action=review

> Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:125
> +    SyscallPolicy::addPermissionToMap(path, permission, m_filePermission);

This could still directly set the HashMap entry, only calling out a helper method (or some already existing API?) that canonicalizes the given path.
Comment 9 Michael Catanzaro 2015-07-22 10:07:06 PDT
(In reply to comment #7)
> But the helper method can be private, just as the static one is now.

Oh, the reason to implement it as a static member function instead of a non-static member function is that it doesn't require access to any member variables. I can make it non-static if you prefer that style, but that's not my preference.

(In reply to comment #8)
> This could still directly set the HashMap entry, only calling out a helper
> method (or some already existing API?) that canonicalizes the given path.

OK, I agree that would be nicer.
Comment 10 Michael Catanzaro 2015-07-22 12:35:51 PDT
Created attachment 257286 [details]
Patch
Comment 11 Michael Catanzaro 2015-07-22 15:10:54 PDT
Committed r187190: <http://trac.webkit.org/changeset/187190>