369566 - [FIX]nsPrincipal's nsISerializable implementation seems to be broken

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I don't see how Write writes out enough information to restore the principal, nor how Read() actually restores it.  It _could_ work if the caller then happened to do the InitFromPersistent stuff given the pref name that the principal reads out, but that's not happening anywhere that I can tell.

Even then you'd need minor things like the codebase and the cert to actually function.

It so happens that fastload only writes out and reads in system principals for now, so things don't break badly.  Bug I was about to suggest to someone that we use the serializing stuff on principals to store info along with a bookmark... and that would just have been bad.

I think we either need to fix this (not sure how unless we make certs implement nsISerializable) or not have nsPrincipal QI to nsISerializable, or at least have Read and Write throw.

Comment 1

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

Seems like the easy thing to do is to only make the system principal QI to nsISerializeable.

Not marking as blocking since this doesn't appear to actually break anything right now. But it'd be nice to fix and should be relatively easy to do so.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, this does break stuff.  I'm given to understand that places and feeds are doing security checks with URIs as the origin, and they can't do them with principals because they need to persist the object representing the originator of the action.  They can persist a URI.  But they can't persist a principal.

Sorry I forgot to set the deps when I filed...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Initial implementation

This doesn't do certificates yet; I'm waiting on feedback from the crypto guys before implementing serialization of those.  But it should do everything else.  This does fix bug 387446 over here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Somewhat better

This also does certificates, if they're serializable.  They're not yet, but I filed bug 388128 on it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272288 [details] [diff] [review]
Somewhat better

Brendan, is changing XUL_FASTLOAD_FILE_VERSION enough?  Note that changing nsPrincipal serialization can change how JSScript serializes.  Do I need to bump JSXDR_BYTECODE_VERSION as well?

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 272288 [details] [diff] [review]
Somewhat better

>+  nsTArray<nsAutoPtr<nsHashtable> > mAnnotations;

Nit: space after outer < to match mandatory > >

> NS_IMETHODIMP
> nsPrincipal::Read(nsIObjectInputStream* aStream)
> {
>-  PRUint32 annotationCount;
>-  nsresult rv = aStream->Read32(&annotationCount);
>+  mInitialized = PR_TRUE;

Should this be set true only at the end, false at the front in case of early return? Or see below for a more compact style of error handling used by XUL fastload code.

>+
>+  PRBool hasCapabilities;
>+  nsresult rv = aStream->ReadBoolean(&hasCapabilities);
>+  if (NS_SUCCEEDED(rv) && hasCapabilities) {
>+    // We want to use one of the nsHashtable constructors, but don't want to
>+    // generally have mCapabilities be a pointer... and nsHashtable has no
>+    // reasonable copy-constructor.  Placement-new to the rescue!
>+    mCapabilities.~nsHashtable();
>+    new (&mCapabilities) nsHashtable(aStream, ReadAnnotationEntry,
>+                                     FreeAnnotationEntry, &rv);
>+  }
>+
>   if (NS_FAILED(rv)) {
>     return rv;
>   }

Assuming the dtor will clean up everything, you could code 'rv |= ...' in series, and test for failure in rv at the end only. Same for the Write method.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Should this be set true only at the end

I can nix that line completely, since I call Init().  It's a relic from an early patch version.  I'll remove it.

I did consider the rv |= thing.  It requires care as to which things are or are not safe to press through on (e.g. if reading haveCert errors, we don't want to readObject the cert), and I didn't want to deal with either having to think about it myself or impose that mind-print on others.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> > Should this be set true only at the end
> 
> I can nix that line completely, since I call Init().  It's a relic from an
> early patch version.  I'll remove it.

Ok. Do you want review with that in mind, or are you gonna put up a new patch?

> I did consider the rv |= thing.  It requires care as to which things are or are
> not safe to press through on (e.g. if reading haveCert errors, we don't want to
> readObject the cert), and I didn't want to deal with either having to think
> about it myself or impose that mind-print on others.

I hear ya. I still find it acceptable for more "straight-line outermost logic level" control flow, which you have a bunch of in the patch.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

Review with that line removed in mind if you can?

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 272288 [details] [diff] [review]
Somewhat better

r+me with < ... < ... > > angle-bracket spacing symmetry nit picked.

/be

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 272288 [details] [diff] [review]
Somewhat better

r=dveditz

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

This makes the principal serialization/deserialization code actually work.  We need this for session restore and to prevent bogus principal objects appearing when non-system things are fastloaded.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fix checked in.