[tor-bugs] #14059 [Tor Browser]: Revision of existing double key cookie logic to meet requirements

[Tor Bug Tracker & Wiki]

#14059: Revision of existing double key cookie logic to meet requirements
-----------------------------+----------------------------
     Reporter:  michael      |      Owner:  michael
         Type:  defect       |     Status:  needs_revision
     Priority:  normal       |  Milestone:
    Component:  Tor Browser  |    Version:
   Resolution:               |   Keywords:
Actual Points:               |  Parent ID:  #3246
       Points:               |
-----------------------------+----------------------------

Comment (by michael):

 Replying to [comment:18 gk]:
 > 1) Please document why you use one time
 `mThirdPartyUtil->GetFirstPartyURIFromChannel` and the other time
 `mThirdPartyUtil->GetFirstPartyIsolationURI` and what that implies.
 >
 GetFirstPartyIsolationURI is a special case of GetFirstPartyURIFromChannel
 which operates on the condition of party isolation (see requirement above
 'Conditional operation') while the latter (GetFirstPartyURIFromChannel)
 passes the first party URI unconditionally.

 Since we unconditionally store both keys when writing but make decisions
 (send cookie or not?) on the condition of privacy.thirdparty.isolate and
 the private browsing channel attribute, we use GetFirstPartyURIFromChannel
 when writing (SetCookieString) and GetFirstPartyIsolationURI when reading
 (GetCookieStringCommon.)

 To clarify this difference I wrote two new comment lines in the source
 code near `nsCookieService::GetCookieStringCommon`. I think that's what
 you meant by 'document.'
 [[br]]

 > 2) You can't reuse `requireHostMatch` in `SetCookieStringInternal` as
 this would mean that the URL bar domain could influence unrelated cookies
 checks which it must not do.
 >
 Good catch, I'm still considering a solution.
 [[br]]

 > 3)
 > {{{
 > // origin matches matches
 > }}}
 >
 Thanks, I fixed that.
 [[br]]

 > 4) There are several places where you just use `baseDomain` in
 nsCookie::Create() which is especially consifusing in `GetCookieFromRow()`
 as the first comment is talks about to skip reading the baseDomain what we
 do that nevertheless. Could you add a comment on this baseDomain usage
 please?
 >
 There are two places that I've named the '''baseDomain''' variable near a
 call to `nsCookie::Create`, namely patch line '''310''' and line '''344'''
 but I haven't touched the persistent cookie code including
 `GetCookieFromRow()`. Should there be a comment near both lines '''310'''
 and '''344''' (in the patch msvb14058-283f7c6.patch) resembling
 'unconditionally write the second (baseDomain) key as well as the first
 (host) key when storing cookies.' Is that what you mean?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/14059#comment:20>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online