389188 - Expose the principal of an image on its imgIRequest

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P4)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This will allow us to improve some security checks.  drawImage on canvas comes to mind, but possibly also others.

I'd really like to get this done for 1.9.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 273331 [details] [diff] [review]
Proposed patch

imgRequest::GetPrincipal always returns a failure; it seems you forgot a return NS_OK :-)

I'm not sure it's good to make libpr0n depend on caps, couldn't you just expose the owner and post-redirect URI instead and have the caller turn that into a principal?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Good catch on the return, yeah.

For the other, I could, I guess.  It's much more annoying for callers, and there's no guarantee that the way the principal is derived from a channel will remain invariant and will only involve the owner and final URI.  In fact, that's not even how it works now; sometimes the originalURI is used.  That's why I'd like to have a single call point for it to go through (getChannelPrincipal) so we don't run into issues with having to change it.  I do think that having to pull in xpconnect and js really sucks in this case; that really shouldn't have been done via inheritance.  Maybe we can change that.

As a compromise I would be happy handing back the channel, but I know stuart wouldn't want that.  ;)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

I would much rather expose the channel, yeah...

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 273331 [details] [diff] [review]
Proposed patch

please rerequest sr once you have review

Comment 6

[Stuart Parmenter]

Given our previous conversations and the fact that the channel is shared, will the principal of one channel in one document be right for a load of the same image in another document?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes.  This principal is all about where the image comes from, not who's loading it.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 273331 [details] [diff] [review]
Proposed patch

sr=biesi, remember the return NS_OK thing

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273331 [details] [diff] [review]
Proposed patch

Requesting approval for basic security arch patch that we need as a basis for ongoing security work.  Risk is very low.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked in, after merging for the bitrot.  It would really be nice if patches didn't have to sit around this long....  :(

Comment 11

[Dave Townsend [:mossop]]

Backed out for the time being due to potential private bytes and Tp increases on talos

Comment 12

[Robert Sayre]

This patch definitely increased the private bytes metric.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So we did some calculations on IRC.  This patch adds probably on the order of 500 bytes per imgRequest (nsPrincipal + nsStandardURL + URI string).  If it's adding something like 2-3MB (as in this case), then we're looking at something like 4000-6000 images in flight at once.  It's possible that we have that many in our image cache if they're all small.  It's also possible that my 500 byte estimate is off (see below).

The problem is that we really do need this data to be able to do reasonable security checks (e.g. bug .  There is some unfortunate data duplication that scriptability and crap CAPS APIs impose on nsPrincipal (having to clone the URI, for example; otherwise it could just hold on to the same URI that the imgRequest is already holding on to).  We could try to avoid eager allocation of the principal, but then we need to hold on to the channel for the lifetime of the imgRequest.  Right now the channel is dropped in OnStopRequest in imagelib.  I'm pretty sure that a channel is bigger than a principal.  For one thing, it owns all the request and response headers.  So unless the channel is staying alive anyway, we don't win anything.

I guess the real questions are:

1)  What is pbytes measuring?
2)  Do we care about it?
3)  Do we care enough to have security checks that don't give the right answer
    in certain edge cases (in both directions: allowing access when they should
    not, and disallowing when they should)?
4)  If the answer to #3 is no and the answer to #2 is yes, do we reland as-is,
    or do we try to eliminate the memory overhead of principals in various ways
    (reducing or removing the URI cloning, etc).  This last might be a good bit
    of work.

It might also be interesting to get hard data on what's using memory here.  For example, perhaps mAnnotations should be lazily allocated.

Had this patch landed back when approval was requested, I'd take this sort of instrumentation on, but at this point this is off my plate until after higher-priority things (non-Mozilla ones) happen.  Which is probably after 1.9 ships.  So someone else really needs to pick this up for 1.9, unless we're ok with the broken security checks.

Comment 14

[Boris Zbarsky [:bzbarsky]]

It would also have been nice if it hadn't taken a month and a half to get review to start with....  Between that and the two-week lag on approval, I've once again been reminded why I should avoid imagelib like the plague.

Comment 15

[Dave Townsend [:mossop]]

As commented this did cause the private bytes regression. The numbers are a little unclear about whether it caused a Tp regression so if this is checked in again please check the Tp on talos for a few runs afterwards.

Comment 16

[Boris Zbarsky [:bzbarsky]]

The other thing that might have happened, by the way, is that skin images don't get the system principal, so that they all got nsPrincipals hanging off of them.  Depending on how many of those we have in flight it might matter.  But I really hope we don't have thousands, or even hundreds, of those.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could we fix the problem of principals not sharing URIs? Seems like they should be able to lazily clone the uris if they need to modify them.

Is there a bug for this?

Comment 18

[Mike Schroepfer]

(In reply to comment #13)

> I guess the real questions are:
> 
> 1)  What is pbytes measuring?

It is described here:

http://technet2.microsoft.com/WindowsServer/en/Library/86b5d116-6fb3-427b-af8c-9077162125fe1033.mspx?mfr=true

"Shows the size, in bytes, that this process has allocated that cannot be shared with other processes."

Working Set is:

"Shows the size, in bytes, in the working set of this process. The working set is the set of memory pages that were touched recently by the threads in the process. If free memory in the computer is above a threshold, pages are left in the working set of a process, even if they are not in use. When free memory falls below a threshold, pages are trimmed from working sets. If the pages are needed, they will be soft-faulted back into the working set before leaving main memory."

If you are on windows you can see both of these in the task manager.  Working Set is the first one.   

> 2)  Do we care about it?

Yes.  Not in absolute - there are always memory/performance trade-offs - but as we all know memory consumption is a very common source of frustration for users. :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Jonas, the issue is that principals need to ensure that their URI will not get mutated.  If they're sharing a URI with someone else, they can't ensure that, because the someone else might mutate it.  If the URI coming in is already immutable, they could use it without cloning it (though I don't think they do), but at the moment channel URI objects are not in fact immutable, though they should be.

We could remove the cloning in principals and just go back to hoping that no one mutates a URI object that we're using elsewhere for security checks, but that seems pretty bad to me.

We could also (and should, imo) make the mCapabilities hashtable in nsPrincipal lazily allocated, since it's rarely used.  I'll file a bug on this and a few other principal-memory-reduction things.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Filed bug 397733 on some changes that will reduce the footprint of an nsPrincipal a bit (about 200 bytes pluse URI length).

Comment 21

[Stuart Parmenter]

someone who understands what not taking this breaks should comment on it blocking.  Given that we haven't had it in prior releases I'm not sure we should hold the release for it.

Comment 22

[Damon Sicore (:damons)]

Comment on attachment 273331 [details] [diff] [review]
Proposed patch

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip, with the NS_OK thing

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in.  Perf seems to be OK this time.