[tor-bugs] #9701 [Tor Browser]: Prevent TorBrowser from creating clipboardcache turds

[Tor Bug Tracker & Wiki]

#9701: Prevent TorBrowser from creating clipboardcache turds
-------------------------+-------------------------------------------------
     Reporter:           |      Owner:  mikeperry
  cypherpunks            |     Status:  needs_review
         Type:  defect   |  Milestone:
     Priority:  normal   |    Version:
    Component:  Tor      |   Keywords:  tbb-disk-leak, interview, tbb-
  Browser                |  firefox-patch, TorBrowserTeam201501R
   Resolution:           |  Parent ID:
Actual Points:           |
       Points:           |
-------------------------+-------------------------------------------------

Comment (by michael):

 Replying to [comment:30 mcs]:
 > I don't think we have to worry about non-Mozilla components calling
 DataStruct::SetData(); it looks like that method is only called from
 within nsTransferable.cpp
 > [...]
 > Is that your conclusion as well?
 >
 Yes, SetData is not defined in IDL and only nsTransferable objects call
 DataStruct::SetData(). So Arthur and I think it's okay to change the
 arguments by adding a '''PBM flag.'''

 Furthermore, with no IDL entries, there's no XPCOM transport to get
 interfaces, so my attempt to avoid foreign code crashing by '''adding an
 overloaded method''' might not help that much.
 [[br]]
 > (I am not sure why it is not declared inside nsTransferable.cpp instead
 of in the header).
 >
 ...or why '''DataStruct''' is a independent class, rather than a private
 member of '''nsTransferable?'''
 [[br]]
 > I would feel better if the old DataStruct::SetData() call that does not
 accept a aIsPrivBrowsing parameter was removed entirely (to avoid any
 chance that the wrong call could be made).
 >
 Assuming the overloaded method is a NOOP there's no tradeoff to worry
 about, so thanks for the tip.

 I've deleted the original DataStruct::SetData() definition and
 implementation not including the bool PBM flag.
 ~~DataStruct::SetData ( nsISupports*, uint32_t);~~
 DataStruct::SetData ( nsISupports*, uint32_t, bool);
 [[br]]
 >In fact, there is another call later in nsTransferable::SetTransferData()
 that should be modified to use the new method as well.
 >
 Opla, good catch. Now the second (of two) call has the relevant PBM aware
 logic as well.
 [[br]]
 > One nit:  You should be able to just access the mPrivateData member
 directly instead of bothering with a call to GetIsPrivateData().
 >
 I used the accessor so that the mPrivateData type could be internally
 changed in a future FF release (typical raison d'etre for accessors,) but
 either way is fine. If we have a convention of direct member manipulation
 I'll change the logic to match.

 Do we/should I?

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