296364 - [FIXr]callers of nsCrypto::GetScriptPrincipal leak the principal

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

nsCrypto::GetScriptPrincipal claims to return an nsIPrincipal* and its callers
assign this to an nsCOMPtr<nsIPrincipal>.  But GetScriptPrincipal sometimes
addrefs the principal, in which case this procedure leaks.

In fact, the only codepath inside GetScriptPrincipal that _doesn't_ addref seems
to have addrefed up until bug 240745 was checked in and I'm guessing was changed
by accident.  Brian, was there a reason for that change that I'm missing?  (This
is the "found nothing on the stack, use the global object's principal" codepath
in nsCrypto::GetScriptPrincipal.)

In any case, I have what I think should be a reasonable fix (short of doing the
deCOM job that was done on the nsScriptSecurityManager code here)...  I'm not
quite sure how to test this, though.

Also, I have to wonder whether it would be possible to reuse security manager
code here instead of duplicating it.  Otherwise changes to the security manager
may not make their way here (and that may in fact make this code insecure)
Perhaps we should expose a relevant hook on nsIScriptSecurityManager or some
other (new?) caps interface?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: SImplest fix for the leak, I think

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 185140 [details] [diff] [review]
SImplest fix for the leak, I think

I think the signature mislead me into thinking the method returned a
non-addref'd pointer and that I was actually fixing a leak.

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 185140 [details] [diff] [review]
SImplest fix for the leak, I think

r=kaie

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185140 [details] [diff] [review]
SImplest fix for the leak, I think

Requesting 1.8b3 approval.  This should be a pretty safe leak fix.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fixed