Bug 207404 - Ephemeral session data leaks between processes
Summary: Ephemeral session data leaks between processes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: All macOS 10.15
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-07 13:55 PST by Pavel Feldman
Modified: 2020-02-19 16:40 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.94 KB, patch)
2020-02-07 15:49 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2020-02-13 16:18 PST, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2020-02-07 13:55:29 PST
When new network session is created, sessionID is used as an identifierBase:

WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:
networkProcess.ensureSession(sessionID, parameters.networkSessionParameters.shouldUseTestingNetworkSession, makeString(base, '.', sessionID.toUInt64()), WTFMove(uiProcessCookieStorage));

WebKit/NetworkProcess/NetworkProcess.cpp:
storageSession = adoptCF(createPrivateStorageSession(cfIdentifier.get()));

As a result, when there is more than one instance of the same application, sessions with the same sessionIDs use the same private store. SessionID's ephemeral session id is a simple counter from 0x8000000000000001 and on. So they will always be clashing between the processes.

I can follow up with the fix and am asking for your preference. One way would be to make SessionIDs cryptographically unique:

SessionID SessionID::generateEphemeralSessionID()
{
    ASSERT(isMainThread());
    RELEASE_ASSERT(!generationProtectionEnabled);

    uint64_t sessionId;
    cryptographicallyRandomValues(&sessionId, sizeof(sessionId));
    sessionId = sessionId | SessionConstants::EphemeralSessionMask;
    return SessionID(sessionId);
}

Another way would be blending proccess ID into the identifier base in RemoteNetworkingContext.mm.

I like the random sessionID because it would solve similar clashes once and for all, but I'm looking for your advice.
Comment 1 Pavel Feldman 2020-02-07 15:49:47 PST
Created attachment 390140 [details]
Patch
Comment 2 Alexey Proskuryakov 2020-02-08 12:58:01 PST
Comment on attachment 390140 [details]
Patch

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

> Source/WebCore/PAL/ChangeLog:7
> +        Before the change, session ids were sequential and the data was leaking
> +        between ephemeral sessions of different processes.

Have you observed the symptom? I just opened two instances of STP, opened a private browsing window in each, logged in to Bugzilla in one, and verified that I wasn't logged in in another.
Comment 3 Pavel Feldman 2020-02-09 15:28:18 PST
Yes, that's why I was looking into it. Steps to reproduce are:

1) Run Minibrowser #1
2) Run Minibrowser #2
3) In Minibrowser #1 pick "New WebKit2 Private Window" menu item
4) Log into bugs.webkit.org
5) In Minibrowser #2 pick "New WebKit2 Private Window" menu item
6) You are logged in
Comment 4 Pavel Feldman 2020-02-09 15:31:51 PST
I can repro with Safari Version 12.1.1 (14607.2.6.1.1) as well, same steps.
Comment 5 Pavel Feldman 2020-02-09 15:36:11 PST
(you'd need to run Safari as "open -n -a Safari" to make sure there are indeed two instances running)
Comment 6 Alex Christensen 2020-02-10 08:02:20 PST
Comment on attachment 390140 [details]
Patch

I don't think random session ids will solve this.  Does it fix the symptom for you?
Does this already-being-logged-in symptom reproduce for non-bugzilla sites?
This sounds like it's getting something out of the keychain, but I'd have to look into it more.
Comment 7 Pavel Feldman 2020-02-10 09:32:03 PST
It fixes the symptoms, check out the exact lines of code that I pointed to in the first comment - they explain why this happens.

There are also options for fixing, so it does not need to be random ids option. But random ones prevents similar problems from happening in the future.

It is not localized to bugs.webkit.org, it is about all cookies in general that end up in a single store for seemingly isolated private sessions.
Comment 8 Alexey Proskuryakov 2020-02-10 13:00:39 PST
I can reproduce with Safari Tech Preview and steps in comment 3. I think that I had steps 4 and 5 reversed the last time. It's definitely not the keychain.

I think that ultimately this is a CFNetwork issue - it probably should isolate ephemeral sessions even between processes with same bundle ID. But the patch seems like an improvement, because it at the very least avoids accidental leaks. Appending a PID of Networking or WebContent process may be reasonable too.

Alex, what do you think?
Comment 9 Pavel Feldman 2020-02-13 12:14:16 PST
As per comment above, restoring r? for it to get another look.
Comment 10 Alex Christensen 2020-02-13 15:50:24 PST
Comment on attachment 390140 [details]
Patch

Not only does this not avoid collision with SessionID::LegacyPrivateSessionID, but it does not solve the real issue, which is probably something having to do with the cookie storage in NetworkSessionCocoa::NetworkSessionCocoa
Comment 11 Alex Christensen 2020-02-13 15:50:51 PST
We do not give CFNetwork this identifier
Comment 12 Alex Christensen 2020-02-13 16:14:49 PST
We implicitly give this to CFNetwork in _CFURLStorageSessionCreate
Comment 13 Alex Christensen 2020-02-13 16:18:25 PST
Created attachment 390702 [details]
Patch
Comment 14 Alex Christensen 2020-02-13 16:22:11 PST
Pavel, using the PID would also not be as secure as we would like because PIDs are reused and there could be race conditions that still leak data.

This solution is very similar to Pavel's suggested solution.  With your permission I would like to list you as a coauthor of this patch.
Comment 15 Pavel Feldman 2020-02-14 09:23:03 PST
> Not only does this not avoid collision with SessionID::LegacyPrivateSessionID

I believe it does. Collision here implies that the crypto-secure random is equal to en exact number, which is far less likely than the hardware DRAM error.

> But it does not solve the real issue, which is probably something having to do with the cookie storage in NetworkSessionCocoa::NetworkSessionCocoa

I believe it does since the solution you are proposing is addressing it via modifying the exact same token.

> We do not give CFNetwork this identifier

I believe we do, in the line you change |identifierBase| is built from the sessionID value. I pointed to the line in RemoteNetworkingContext.mm where we do it in my first comment to the bug.

> This solution is very similar to Pavel's suggested solution.  With your permission I would like to list you as a coauthor of this patch.

Sure thing. But could you elaborate on why this could not be in a form of the review comment that I would address?
Comment 16 Pavel Feldman 2020-02-14 10:55:11 PST
Btw, I'm currently "inactive" reviewer, so I can't r+ your patch. I'm in the process of collecting patches to re-gain the status. All in all it looks good to me. I'd still prefer to randomize sessoinID so that it did not bite us in other places, but it addresses the issue in hand as is, so all is good.
Comment 17 Darin Adler 2020-02-16 17:09:34 PST
Comment on attachment 390702 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:514
> +    RetainPtr<CFStringRef> cfIdentifier = makeString(identifierBase, ".PrivateBrowsing.", createCanonicalUUIDString()).createCFString();

Why not just use a UUID? Is it important to include the identifier and the word “PrivateBrowsing”? Is that used in debugging or something?

> Source/WebKitLegacy/WebCoreSupport/NetworkStorageSessionMap.cpp:89
> +    RetainPtr<CFStringRef> cfIdentifier = makeString(identifierBase, ".PrivateBrowsing.", createCanonicalUUIDString()).createCFString();

Ditto.
Comment 18 Pavel Feldman 2020-02-17 13:31:02 PST
I don't think it is used anywhere, so using just the UUID should also work.
Comment 19 Alex Christensen 2020-02-17 17:17:39 PST
My claim "We do not give CFNetwork this identifier" was what we intend to be correct, but it is incorrect, hence the need for this change.
Another reason I am opposed to randomizing session IDs is because the sessionID is a very useful debugging tool, and right now it is easy to see that this is the first ephemeral session, the second ephemeral session, etc.  If we randomized it that would go away.
We use a string that contains the bundle ID and "PrivateBrowsing" and now a UUID to help with debugging.  I'd prefer to keep that rather than just use a UUID.
Comment 20 Alex Christensen 2020-02-17 17:19:58 PST
http://trac.webkit.org/r256792
Comment 21 Radar WebKit Bug Importer 2020-02-17 17:20:15 PST
<rdar://problem/59533067>
Comment 22 Darin Adler 2020-02-18 09:23:22 PST
(In reply to Alex Christensen from comment #19)
> right now it is easy to see
> that this is the first ephemeral session, the second ephemeral session, etc.
> If we randomized it that would go away.

I want to understand, but I don’t yet. I was looking and I don’t see a number that says "this is first, this is second", just a UUID. How is this not randomized?
Comment 23 Alex Christensen 2020-02-18 11:05:54 PST
(In reply to Darin Adler from comment #22)
> (In reply to Alex Christensen from comment #19)
> > right now it is easy to see
> > that this is the first ephemeral session, the second ephemeral session, etc.
> > If we randomized it that would go away.
> 
> I want to understand, but I don’t yet. I was looking and I don’t see a
> number that says "this is first, this is second", just a UUID. How is this
> not randomized?

PAL::SessionID::toUInt64 returns a value that returns such an integer.  In other use of PAL::SessionID that was not changed in this patch, I often use this value in logging to debug things.
Comment 24 Darin Adler 2020-02-18 20:17:29 PST
(In reply to Alex Christensen from comment #23)
> PAL::SessionID::toUInt64 returns a value that returns such an integer.  In
> other use of PAL::SessionID that was not changed in this patch, I often use
> this value in logging to debug things.

OK, but what does PAL::SessionID have to do with the storage session identifier string?
Comment 25 Pavel Feldman 2020-02-19 16:40:59 PST
I believe the first comment to this bug contains the answer to your question.