Bug 207404 – Ephemeral session data leaks between processes

• Component: WebKit2
• Status: RESOLVED FIXED

[Pavel Feldman]

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.

[Pavel Feldman]

Created attachment 390140 [details]
Patch

[Alexey Proskuryakov]

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.

[Pavel Feldman]

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

[Pavel Feldman]

I can repro with Safari Version 12.1.1 (14607.2.6.1.1) as well, same steps.

[Pavel Feldman]

(you'd need to run Safari as "open -n -a Safari" to make sure there are indeed two instances running)

[Alex Christensen]

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.

[Pavel Feldman]

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.

[Alexey Proskuryakov]

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?

[Pavel Feldman]

As per comment above, restoring r? for it to get another look.

[Alex Christensen]

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

[Alex Christensen]

We do not give CFNetwork this identifier

[Alex Christensen]

We implicitly give this to CFNetwork in _CFURLStorageSessionCreate

[Alex Christensen]

Created attachment 390702 [details]
Patch

[Alex Christensen]

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.

[Pavel Feldman]

> 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?

[Pavel Feldman]

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.

[Darin Adler]

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.

[Pavel Feldman]

I don't think it is used anywhere, so using just the UUID should also work.

[Alex Christensen]

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.

[Alex Christensen]

http://trac.webkit.org/r256792

[Radar WebKit Bug Importer]

<rdar://problem/59533067>

[Darin Adler]

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

[Alex Christensen]

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

[Darin Adler]

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

[Pavel Feldman]

I believe the first comment to this bug contains the answer to your question.