Bug 142986 – [Seccomp] Canonicalize filesystem path when whitelisting it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 249279 [details]
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it

[Michael Catanzaro]

Created attachment 249281 [details]
[Linux] Improvements to SyscallPolicy

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 249285 [details]
[Linux] Seccomp Filters: Canonicalize filesystem path when whitelisting it

[Zan Dobersek]

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?

[Michael Catanzaro]

I only see one good reason: to keep SyscallPolicy::PermissionMap private.

[Zan Dobersek]

But the helper method can be private, just as the static one is now.

[Zan Dobersek]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 257286 [details]
Patch

[Michael Catanzaro]

Committed r187190: <http://trac.webkit.org/changeset/187190>