WebKit
[WPE][GTK] Website data leaked outside specified data/cache directories
4e14746e445b586ffada351949f43aa248b09432
[WPE][GTK] Website data leaked outside specified data/cache directories https://bugs.webkit.org/show_bug.cgi?id=239521 Reviewed by NOBODY (OOPS!). Currently whenever somebody adds a new type of website data, you have to add new public WPE/GTK API to WebKitWebsiteDataManager to allow configuring the data location. If this step is not performed, the data will leak outside the expected directory locations, which is a serious privacy issue. Sadly, this step is usually missed in practice. We released 2.36 without accounting for the new general storage directory, resulting in the general storage directory being created outside the base data directory, which is not acceptable. Similarly, we missed deviceidhashsalts, which was added *four years ago*. This case is a little weird because we did add a WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT WebKitWebsiteDataTypes flag to allow deleting the data, but unfortunately we missed that configuring a directory under the base data directory was required. This is bad. Apple developers shouldn't be expected to write Linux-specific code when making cross-platform changes. Linux developers shouldn't need to worry that oversights will lead to data being stored outside permitted locations. This commit attempts to fix this *comprehensively*. First: we need to make sure the default directories only get used when base data directory or base cache directory is NULL, and also ensure this is hard to mess up. I considered many different approaches but ultimately decided the simplest way to do it is to make all the default directories relative to the base directories. This requires some cross-platform changes in WebsiteDataStoreConfiguration, but nothing too disruptive. I've used a default null string parameter to minimize the amount of code changes required in Apple ports, and left the base directories unused for Apple. For Windows, I've gone ahead and implemented even though they are unused, because the implementation is so simple. I've also added an assert to make sure WPE/GTK always use the base directories. By pushing the base directory logic from WebKitWebsiteDataManager down to WebsiteDataStoreConfiguration, it's now hopefully quite difficult to mess up, and the burden on Apple developers is limited to just choosing whether the new directory should be data or cache. I've also renamed WillCopyPathsFromExistingConfiguration to ShouldInitializePaths, in order to better match the new semantics, and brought it inside the class since it is no longer needed by the public WebsiteDataStoreConfiguration::create methods. Next problem: the default WebsiteDataStore. For cross-platform WebKit, I suppose this can be used whenever a specific WebsiteDataStore is not configured, and therefore it ought to be a persistent data store that actually stores data on disk. This should never happen for WPE and GTK: a specific WebsiteDataStore must always be used. This is currently ensured by WebKitWebContext, which creates its own WebKitWebsiteDataStore if the application does not do so itself. The default WebsiteDataStore is still needed for prewarmed processes, but I hope for nothing else. Accordingly, we can safely make it nonpersistent for WPE and GTK, but not for other ports. This is required to comprehensively ensure there is no way to create a WebsiteDataStoreConfiguration without initializing the base data and base cache directories unless it is an ephemeral store. In particular, we can assert the WebsiteDataStoreConfiguration is either created with base directories, or ephemeral. Finally, I got a bit carried away and decided to deprecate all the non-base directory configuration APIs from WebKitWebsiteDataManager. These are of very limited utility because frankly, who cares what each particular subdirectory is named? What applications really want to do is ensure that all data gets stored only where the application expects it to be stored, a use case that can be achieved by configuring only the base data and base cache directories. By deprecating these, we remove the expectation that we should add more in the future or keep the list of directories here updated, and we can remove them in the upcoming GTK 4 API version. I've included some documentation updates to remove references to the deprecated properties and functions, and snuck in a few drive-by fixes. All of this could have been done separately, but it makes sense to do here so as to eliminate the temptation to add new APIs to configure the general storage directory and hash salts. Future consideration: in future API versions, where WebKitWebsiteDataManager's localstorage and IndexedDB directory configuration APIs are removed, we can change WebsiteDataStore::defaultShouldUseCustomStoragePaths to allow WPE and GTK to use the new general storage directory instead of the legacy custom localstorage and IndexedDB directories. Cannot do that yet because the old APIs must still be supported even if deprecated. * Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp: (webkitWebsiteDataManagerGetProperty): (webkit_website_data_manager_class_init): (webkitWebsiteDataManagerGetDataStore): * Source/WebKit/UIProcess/API/gtk/WebKitWebsiteDataManager.h: * Source/WebKit/UIProcess/API/wpe/WebKitWebsiteDataManager.h: * Source/WebKit/UIProcess/Inspector/gtk/WebInspectorUIProxyGtk.cpp: (WebKit::inspectorWebsiteDataStore): * Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm: (WebKit::WebsiteDataStore::defaultApplicationCacheDirectory): (WebKit::WebsiteDataStore::defaultCacheStorageDirectory): (WebKit::WebsiteDataStore::defaultGeneralStorageDirectory): (WebKit::WebsiteDataStore::defaultNetworkCacheDirectory): (WebKit::WebsiteDataStore::defaultAlternativeServicesDirectory): (WebKit::WebsiteDataStore::defaultMediaCacheDirectory): (WebKit::WebsiteDataStore::defaultIndexedDBDatabaseDirectory): (WebKit::WebsiteDataStore::defaultServiceWorkerRegistrationDirectory): (WebKit::WebsiteDataStore::defaultLocalStorageDirectory): (WebKit::WebsiteDataStore::defaultMediaKeysStorageDirectory): (WebKit::WebsiteDataStore::defaultDeviceIdHashSaltsStorageDirectory): (WebKit::WebsiteDataStore::defaultWebSQLDatabaseDirectory): (WebKit::WebsiteDataStore::defaultResourceLoadStatisticsDirectory): (WebKit::WebsiteDataStore::defaultJavaScriptConfigurationDirectory): (WebKit::WebsiteDataStore::defaultModelElementCacheDirectory): (WebKit::WebsiteDataStore::cacheDirectoryFileSystemRepresentation): (WebKit::WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation): * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp: (WebKit::defaultDataStoreIsPersistent): (WebKit::WebsiteDataStore::defaultDataStore): (WebKit::WebsiteDataStore::defaultMediaCacheDirectory): (WebKit::WebsiteDataStore::defaultAlternativeServicesDirectory): (WebKit::WebsiteDataStore::defaultJavaScriptConfigurationDirectory): * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h: * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.cpp: (WebKit::WebsiteDataStoreConfiguration::WebsiteDataStoreConfiguration): (WebKit::WebsiteDataStoreConfiguration::initializePaths): (WebKit::WebsiteDataStoreConfiguration::copy const): * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h: (WebKit::WebsiteDataStoreConfiguration::create): (WebKit::WebsiteDataStoreConfiguration::createWithBaseDirectories): * Source/WebKit/UIProcess/WebsiteData/playstation/WebsiteDataStorePlayStation.cpp: (WebKit::WebsiteDataStore::defaultApplicationCacheDirectory): (WebKit::WebsiteDataStore::defaultCacheStorageDirectory): (WebKit::WebsiteDataStore::defaultGeneralStorageDirectory): (WebKit::WebsiteDataStore::defaultNetworkCacheDirectory): (WebKit::WebsiteDataStore::defaultIndexedDBDatabaseDirectory): (WebKit::WebsiteDataStore::defaultServiceWorkerRegistrationDirectory): (WebKit::WebsiteDataStore::defaultLocalStorageDirectory): (WebKit::WebsiteDataStore::defaultMediaKeysStorageDirectory): (WebKit::WebsiteDataStore::defaultWebSQLDatabaseDirectory): (WebKit::WebsiteDataStore::defaultResourceLoadStatisticsDirectory): (WebKit::WebsiteDataStore::cacheDirectoryFileSystemRepresentation): (WebKit::WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation): * Source/WebKit/UIProcess/WebsiteData/win/WebsiteDataStoreWin.cpp: (WebKit::WebsiteDataStore::defaultApplicationCacheDirectory): (WebKit::WebsiteDataStore::defaultCacheStorageDirectory): (WebKit::WebsiteDataStore::defaultNetworkCacheDirectory): (WebKit::WebsiteDataStore::defaultGeneralStorageDirectory): (WebKit::WebsiteDataStore::defaultIndexedDBDatabaseDirectory): (WebKit::WebsiteDataStore::defaultServiceWorkerRegistrationDirectory): (WebKit::WebsiteDataStore::defaultLocalStorageDirectory): (WebKit::WebsiteDataStore::defaultMediaKeysStorageDirectory): (WebKit::WebsiteDataStore::defaultWebSQLDatabaseDirectory): (WebKit::WebsiteDataStore::defaultResourceLoadStatisticsDirectory): * Source/WebKit/UIProcess/glib/WebsiteDataStoreGLib.cpp: (WebKit::WebsiteDataStore::defaultApplicationCacheDirectory): (WebKit::WebsiteDataStore::defaultNetworkCacheDirectory): (WebKit::WebsiteDataStore::defaultCacheStorageDirectory): (WebKit::WebsiteDataStore::defaultHSTSDirectory): (WebKit::WebsiteDataStore::defaultGeneralStorageDirectory): (WebKit::WebsiteDataStore::defaultIndexedDBDatabaseDirectory): (WebKit::WebsiteDataStore::defaultServiceWorkerRegistrationDirectory): (WebKit::WebsiteDataStore::defaultLocalStorageDirectory): (WebKit::WebsiteDataStore::defaultMediaKeysStorageDirectory): (WebKit::WebsiteDataStore::defaultDeviceIdHashSaltsStorageDirectory): (WebKit::WebsiteDataStore::defaultWebSQLDatabaseDirectory): (WebKit::WebsiteDataStore::defaultResourceLoadStatisticsDirectory): (WebKit::programName): (WebKit::WebsiteDataStore::cacheDirectoryFileSystemRepresentation): (WebKit::WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation): * Tools/MiniBrowser/gtk/main.c: (gotWebsiteDataCallback): * Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp: (testWebsiteDataConfiguration): (testWebsiteDataEphemeral): (testWebsiteDataITP):
I'm requesting only WPE/GTK reviewers for now, because I'm not sure whether we want to land the WebKitWebsiteDataManager changes here or not, and they are a significant portion of the diff. See this comment.
Once we've figured that out and WPE/GTK reviewers are happy, then I'll request owner review next.
TODO: decide how to resolve https://bugs.webkit.org/show_bug.cgi?id=213471 before merging anything here
Deprecating these APIs and relying on relative paths sounds like a good idea to me.
We have consensus on the WPE/GTK API changes. Adding more reviewers for the cross-platform changes.
It seems we are only deprecating the getters, but I think we should deprecate the properties too. It's weird that you can still set but not get them. Since the directory name is decided by wk and unknown by the app, maybe it makes sense to still allow to get the final directories, but instead of using the properties, we could add a single getter that takes the website data type as argument.
It seems we are only deprecating the getters, but I think we should deprecate the properties too. It's weird that you can still set but not get them.
I did deprecate all the properties! Maybe you were looking at one of the non-deprecated properties, base-data-dir and base-cache-dir?
Since the directory name is decided by wk and unknown by the app, maybe it makes sense to still allow to get the final directories, but instead of using the properties, we could add a single getter that takes the website data type as argument.
Seems fine to me. I don't think it would be too hard to implement that.
I am not very familiar with the WPE/GTK API and its usage. Are you going to set different default base and cache directories for different WebsiteDataStores in the same app? If not, can you just add a global variable like sharedCacheDirectory or sharedBaseDirectory, add an API to set it and use it in cacheDirectoryInContainerOrHomeDirectory if set; so you may not need to change all functions.
For the defaultWebsiteStore, if you are not going to use it, why not create a non-persistent WebsiteDataStore: creating a new WebsiteDataStore instead of making default WebsiteDataStore non-persistent. It seems defaultDataStore will stay forever after it's created with our current implementation.
I am not very familiar with the WPE/GTK API and its usage. Are you going to set different default base and cache directories for different WebsiteDataStores in the same app?
That is supported, yes, although it's hard to think of why you might want to do so. The base directories are properties of the WebKitWebsiteDataManager and you can create as many of those as you want. Accordingly, a sharedCacheDirectory or sharedBaseDirectory won't work.
For the defaultWebsiteStore, if you are not going to use it, why not create a non-persistent WebsiteDataStore: creating a new WebsiteDataStore instead of making default WebsiteDataStore non-persistent. It seems defaultDataStore will stay forever after it's created with our current implementation.
Hm, I'd certainly rather the default data store never get created. But I think it's used for prewarmed processes?
We have this in WebProcessProxy::platformGetLaunchOptions:
if (!dataStore) {
// Prewarmed processes don't have a WebsiteDataStore yet, so use the primary WebsiteDataStore from the WebProcessPool.
// The process won't be used if current WebsiteDataStore is different than the WebProcessPool primary one.
dataStore = WebsiteDataStore::defaultDataStore().ptr();
}
You're suggesting that we could use a non-persistent data store for that instead? That might work. I wonder if anything else will create a default data store. I'll try adding a RELEASE_ASSERT_NOT_REACHED() next week and see what happens.
Although we'd have to figure out what to do about "The process won't be used if current WebsiteDataStore is different than the WebProcessPool primary one."
It seems we are only deprecating the getters, but I think we should deprecate the properties too. It's weird that you can still set but not get them.
I did deprecate all the properties! Maybe you were looking at one of the non-deprecated properties, base-data-dir and base-cache-dir?
I'm sorry, I missed all the changes in the cpp file because github decided it was too much code, I still have to get used to this...
@mcatanzaro This patch could use a rebase, when you have some time for that 😉
This patch could use a rebase, when you have some time for that wink
Indeed. Note it's not suitable for stable branches.
Although we'd have to figure out what to do about "The process won't be used if current WebsiteDataStore is different than the WebProcessPool primary one."
Returning to this... I think we probably cannot easily change this for that reason. So I'll just rebase this pull request.
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/0fa82dc49800a8d8932290e08ab0e44cbdcd4512)
| Misc | iOS, tvOS & watchOS | macOS | Linux | Windows |
|---|---|---|---|---|
| ✅ 🧪 style | ✅ 🛠 ios | ✅ 🛠 mac | ✅ 🛠 wpe | ✅ 🛠 🧪 win |
| ✅ 🧪 bindings | ✅ 🛠 ios-sim | ✅ 🛠 mac-debug | ✅ 🛠 gtk | ✅ 🛠 wincairo |
| ✅ 🧪 webkitperl | ✅ 🧪 ios-wk2 | ✅ 🛠 mac-AS-debug | ❌ 🧪 gtk-wk2 | |
 🧪 api-ios |
✅ 🧪 api-mac | ✅ 🧪 api-gtk | ||
| ✅ 🛠 tv | ✅ 🧪 mac-wk1 | |||
| ✅ 🛠 tv-sim | ✅ 🧪 mac-wk2 | |||
| ✅ 🛠 watch | ❌ 🧪 mac-AS-debug-wk2 | |||
| ✅ 🛠 watch-sim | ✅ 🧪 mac-wk2-stress |
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/f1f085ea7c2b989e1437bad054e3bcae78006ff0)
Tests are crashing now.
The problem was the safety check I added to WebsiteDataStoreConfiguration's constructor to crash if a persistent configuration is created without base directories. The TestController was hitting this check because it used WKWebsiteDataStoreConfigurationCreate. I've fixed this function to set null base directories, which explicitly indicates that it's OK to save data in default locations. The point of the safety check is to avoid doing this accidentally, not to prevent it altogether.
BTW this commit contains Deprecated: 2.38 annotations with the expectation that it will be backported to the webkitglib/2.38 branch. Adrian, Carlos, does that sound OK? If not, I can change it to 2.40. (The website data leaking outside the expected locations is quite unfortunate, but it's been ongoing for years, so I don't see this change as particularly urgent.)
OK, EWS looks green. I will replace Since: 2.38 with Since: 2.40 and then land.
https://github.com/WebKit/WebKit/commit/6209144d0f0d10c5ee5999731ce46a087f0b5236
Committed 253824@main (38ffac2857ee): https://commits.webkit.org/253824@main
Reviewed commits have been landed. Closing PR #2147 and removing active labels.
