Safari sandbox logic error enables reading of arbitrary files

[GoogleCodeExporter]

The description we have so far is in the form of some raw notes that describe 
the logic error:

Goal: get UniversalFileReadSandboxExtension

This IPC will get it for us, providing we pass a few checks:
WebPageProxy::
    BackForwardGoToItem(uint64_t itemID) -> (WebKit::SandboxExtension::Handle sandboxExtensionHandle)

void WebPageProxy::backForwardGoToItem(uint64_t itemID, 
SandboxExtension::Handle& sandboxExtensionHandle)
{
    WebBackForwardListItem* item = m_process->webBackForwardItem(itemID); <-- //check 1: we need to get a file url as an itemID
    if (!item)
        return;

    bool createdExtension = maybeInitializeSandboxExtensionHandle(URL(URL(), item->url()), sandboxExtensionHandle); <-- //*** [1]
    if (createdExtension)
        m_process->willAcquireUniversalFileReadSandboxExtension();
    m_backForwardList->goToItem(item);
}

//[1]
//getting the sandbox extension looks easy; just call this function with the 
file:// URL (and if inspector is enabled, get isInspectorPage(this) to return 
false
bool WebPageProxy::maybeInitializeSandboxExtensionHandle(const URL& url, 
SandboxExtension::Handle& sandboxExtensionHandle)
{
    if (!url.isLocalFile())
        return false;

#if ENABLE(INSPECTOR)
    // Don't give the inspector full access to the file system.
    if (WebInspectorProxy::isInspectorPage(this)) <-- //this will be important later
        return false;
#endif

    SandboxExtension::createHandle("/", SandboxExtension::ReadOnly, sandboxExtensionHandle); <-- //very importantly, this is actually going to grant us read access to the WHOLE FS, not just
                                                                                                 //the file:// url we are trying to get an extension for :-)
    return true;
}

//So check 1: get a file:// url as a webBackForwardItem:

WebBackForwardListItem* WebProcessProxy::webBackForwardItem(uint64_t itemID) 
const
{
    return m_backForwardListItemMap.get(itemID);
}

//just get it inserted in that map;

// easy with this IPC
WebProcessProxy::
    AddBackForwardItem(uint64_t itemID, WTF::String originalURL, WTF::String url, WTF::String title, CoreIPC::DataReference backForwardData)

void WebProcessProxy::addBackForwardItem(uint64_t itemID, const String& 
originalURL, const String& url, const String& title, const 
CoreIPC::DataReference& backForwardData)
{
    MESSAGE_CHECK_URL(originalURL); <-- //EXCEPT, what are these checks
    MESSAGE_CHECK_URL(url);         <-- //.... ? [2]

    WebBackForwardListItemMap::AddResult result = m_backForwardListItemMap.add(itemID, nullptr);
    if (result.isNewEntry) {
        result.iterator->value = WebBackForwardListItem::create(originalURL, url, title, backForwardData.data(), backForwardData.size(), itemID);
        return;
    }

    // Update existing item.
    result.iterator->value->setOriginalURL(originalURL);
    result.iterator->value->setURL(url);
    result.iterator->value->setTitle(title);
    result.iterator->value->setBackForwardData(backForwardData.data(), backForwardData.size());
}

[2]

#define MESSAGE_CHECK_URL(url) 
MESSAGE_CHECK_BASE(m_process->checkURLReceivedFromWebProcess(url), 
m_process->connection())
//it's a runtime check
#define MESSAGE_CHECK_BASE(assertion, connection) do \
    if (!(assertion)) { \
        ASSERT(assertion); \
        (connection)->markCurrentlyDispatchedMessageAsInvalid(); \
        return; \
    } \
while (0)

//what is it actually checking? we have to be able to return true from this 
function to proceed:

bool WebProcessProxy::checkURLReceivedFromWebProcess(const URL& url)
{
    // FIXME: Consider checking that the URL is valid. Currently, WebProcess sends invalid URLs in many cases, but it probably doesn't have good reasons to do that.

    // Any other non-file URL is OK.
    if (!url.isLocalFile())  <-- //it's okay if this isn't a file:// url, well, we want a file:// url so this doesn help
        return true;

    // Any file URL is also OK if we've loaded a file URL through API before, granting universal read access.
    if (m_mayHaveUniversalFileReadSandboxExtension) <-- //we don't have this yet so this is no help...
        return true;

    // If we loaded a string with a file base URL before, loading resources from that subdirectory is fine.
    // There are no ".." components, because all URLs received from WebProcess are parsed with URL, which removes those.
    String path = url.fileSystemPath();
    for (HashSet<String>::const_iterator iter = m_localPathsWithAssumedReadAccess.begin(); iter != m_localPathsWithAssumedReadAccess.end(); ++iter) {
        if (path.startsWith(*iter))
            return true;        //THIS looks more promising :-) if our file://XXX url is in a subdirectory in m_localPathsWithAssumedReadAccess then we can load it
                                // this doesn't sound so exciting, except that remember if we can get ANY file:// url through here then we're actually going to get
                                // a sandbox extension for '/'.... so how can we get something in this list?
    }
...
  return false;
}

ENTER INSPECTOR

//that m_localPathsWithAssumedReadAccess is only modified here:

void WebProcessProxy::assumeReadAccessToBaseURL(const String& urlString)
{
    URL url(URL(), urlString);
    if (!url.isLocalFile())
        return;

    // There's a chance that urlString does not point to a directory.
    // Get url's base URL to add to m_localPathsWithAssumedReadAccess.
    URL baseURL(URL(), url.baseAsString());

    // Client loads an alternate string. This doesn't grant universal file read, but the web process is assumed
    // to have read access to this directory already.
    m_localPathsWithAssumedReadAccess.add(baseURL.fileSystemPath());
}

//which in turn is only called here in code reachable via IPC:
IPC::
    CreateInspectorPage() -> (uint64_t inspectorPageID, WebKit::WebPageCreationParameters inspectorPageParameters)

void WebInspectorProxy::createInspectorPage(uint64_t& inspectorPageID, 
WebPageCreationParameters& inspectorPageParameters)
{
... //a lot of important stuff happens here; come back to that later
    m_page->process()->assumeReadAccessToBaseURL(inspectorBaseURL());  //importantly, this is getting set for the process, not for the page

//the path which is getting passed to assumeReadAccessToBaseURL
//it doesn't really matter what that path is, so long as it's actually a 
file:// url (and fixed or guessable)
String WebInspectorProxy::inspectorBaseURL() const
{
    // Call the soft link framework function to dlopen it, then [NSBundle bundleWithIdentifier:] will work.
    WebInspectorUILibrary();

    NSString *path = [[NSBundle bundleWithIdentifier:@"com.apple.WebInspectorUI"] resourcePath];
    ASSERT([path length]);

    return [[NSURL fileURLWithPath:path] absoluteString];
}

//need to debug to check but it's almost certainly under here: 
/System/Library/PrivateFrameworks/WebInspectorUI.framework/Resources
//see Main.html, Main.js etc

//need to check exactly how we route messages to a particular 
WebPageProxy/WebProcessProxy

//at this point only really have to make sure we pass the 
//WebInspectorProxy::isInspectorPage(this) check
//note that this here corrisponds to a WebPageProxy - not a WebProcessProxy:

bool WebPageProxy::maybeInitializeSandboxExtensionHandle(const URL& url, 
SandboxExtension::Handle& sandboxExtensionHandle)
{
    if (!url.isLocalFile())
        return false;

#if ENABLE(INSPECTOR)
    // Don't give the inspector full access to the file system.
    if (WebInspectorProxy::isInspectorPage(this)) //we have created an Inspector page through createInspectorPage, so we'll have to be careful getting around this
        return false;

bool WebInspectorProxy::isInspectorPage(WebPageProxy* page)
{
    return WebInspectorPageGroups::shared().isInspectorPageGroup(page->pageGroup());
}

//this WebInspectorPageGroups thing seems to be there to allow nesting of 
inspectors (since the inspector itself is just another Page)
in WebInspectorProxy.cpp

class WebInspectorPageGroups {
public:
    static WebInspectorPageGroups& shared()
    {
        //so this is just a singleton
        DEFINE_STATIC_LOCAL(WebInspectorPageGroups, instance, ());
        return instance;
    }

//will be called with page->pageGroup() as group when called by isInspectorPage
    bool isInspectorPageGroup(WebPageGroup* group)
    {
        return m_pageGroupLevel.contains(group);
    }

    typedef HashMap<WebPageGroup*, unsigned> PageGroupLevelMap;
    PageGroupLevelMap m_pageGroupLevel;

//so a page is considered an inspector if it's pageGroup pointer is a key in 
m_pageGroupLevels

//this is the only place keys are added to m_pageGroupLevel
    WebPageGroup* inspectorPageGroupForLevel(unsigned level)
    {
        // The level is the key of the HashMap, so it cannot be 0.
        ASSERT(level);

        auto iterator = m_pageGroupByLevel.find(level);
        if (iterator != m_pageGroupByLevel.end())
            return iterator->value.get();

        RefPtr<WebPageGroup> group = createInspectorPageGroup(level); //note that it's always adding a new PageGroup
        m_pageGroupByLevel.set(level, group.get());
        m_pageGroupLevel.set(group.get(), level);
        return group.get();
    }

WebPageProxy::WebPageProxy(PageClient* pageClient, PassRefPtr<WebProcessProxy> 
process, WebPageGroup* pageGroup, uint64_t pageID){
...
#if ENABLE(INSPECTOR)
    m_inspector = WebInspectorProxy::create(this);
#endif
...
}

//the constructor of the WebInspectorProxy (invoked from the WebPageProxy 
constructor)
WebInspectorProxy::WebInspectorProxy(WebPageProxy* page){
...
//does this add m_page->pageGroup to the m_pageGroupLevel map?
    m_level = WebInspectorPageGroups::shared().inspectorLevel(m_page->pageGroup()); // <-- m_level will be 1
//adds itself as a messageReceived for this process+pageID combo
    m_page->process()->addMessageReceiver(Messages::WebInspectorProxy::messageReceiverName(), m_page->pageID(), this);
}

    unsigned inspectorLevel(WebPageGroup* inspectedPageGroup)
    {
        //the first time around m_pageGroupLevel will be empty so isInspectorPageGroup must be empty -> this returns 1 (and nothing gets added to m_pageGroupLevel)
        return isInspectorPageGroup(inspectedPageGroup) ? inspectorPageGroupLevel(inspectedPageGroup) + 1 : 1;
    }

//going back to createInspectorPage we can see when that map is maipulated to 
add entries:

void WebInspectorProxy::createInspectorPage(uint64_t& inspectorPageID, 
WebPageCreationParameters& inspectorPageParameters)
{
    inspectorPageID = 0;

    if (!m_page)
        return;

    m_isAttached = shouldOpenAttached();
    m_attachmentSide = static_cast<AttachmentSide>(inspectorPageGroup() <--
...

WebPageGroup* WebInspectorProxy::inspectorPageGroup() const
{
    return WebInspectorPageGroups::shared().inspectorPageGroupForLevel(m_level);
}

//this will call createInspectorPageGroup

    static PassRefPtr<WebPageGroup> createInspectorPageGroup(unsigned level)
    {
        RefPtr<WebPageGroup> pageGroup = WebPageGroup::create(String::format("__WebInspectorPageGroupLevel%u__", level), false, false);
        ...
    }
//which creates a new WebPageGroup corrisponding to the correct WebInspector 
level

//so what was the point of all that:
class WebProcessProxy {
...
    typedef HashMap<uint64_t, WebPageProxy*> WebPageProxyMap;

    WebPageProxyMap m_pageMap; <-- // WebProcessProxies have multiple WebPageProxies - identified by pageIDs (uint64_t's)

    HashSet<String> m_localPathsWithAssumedReadAccess; <-- //this is also defined on a per process basis

    WebBackForwardListItemMap m_backForwardListItemMap; // each WebProcessProxy has a single backforwardlistitemmap
...
}

//the crux of the issue is that only the inspector pageID is prevented from 
getting '/' read access, but the inspected page isn't

Original issue reported on code.google.com by cev...@google.com on 11 Mar 2014 at 6:35

[GoogleCodeExporter]

Original comment by cev...@google.com on 11 Mar 2014 at 6:44

• Added labels: Id-604312896

[GoogleCodeExporter]

Apple advisory: http://support.apple.com/kb/HT6181
Fix: 
http://trac.webkit.org/changeset/166026/trunk/Source/WebKit2/UIProcess/WebPagePr
oxy.cpp

Original comment by ianb...@google.com on 7 Apr 2014 at 1:47

• Changed state: Fixed
• Added labels: CVE-2014-1297

[GoogleCodeExporter]

Original comment by ianb...@google.com on 30 Jul 2014 at 5:37

• Added labels: Deadline-90

[GoogleCodeExporter]

Original comment by cev...@google.com on 31 Jul 2014 at 12:17

• Removed labels: Restrict-View-Commit