1248628 - consider better serialization strategy for persisting nsIX509Cert security info on disk

Status: RESOLVED WONTFIX

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1247580 I discovered that the uuid of the nsIX509Cert changed in FF46.  This broke service workers for any site offlined in prior FF versions because the security_info cert could not be deserialized from disk.

I hacked around the problem with a UUID fixup in nsBinaryInputStream, but obviously that really sucks.  We should consider better ways to serialize the security info to disk such that it doesn't break when FF is changed.

Thoughts?

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Alternatively, would there be a way for service workers to fail more gracefully? For instance, if the serialized information is corrupted or changed such that the old information is simply insufficient to reconstruct the necessary information represented by the security info, shouldn't the implementation re-fetch its resources and/or somehow signal that it needs to be reinitialized? Why are we serializing this information anyway? Perhaps the service workers implementation should grab exactly the information it needs to operate and handle its own serialization/deserialization?

For context, this shortcoming is hampering efforts in other areas (notably, implementing certificate transparency - see bug 1305289 and, more generally, bug 1281469).

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

We are storing the security info with the Response in the Cache API because we need to recreate an nsIChannel when the service worker passes that Response to FetchEvent.respondWith().  We need the real security info in order for all the things that look for it on nsIChannel to work.  Think about mixed content, lock in the URL bar, security info dialog, devtools info, etc.

I don't think we can reasonably redownload all the resources when the security info structure changes.  Some of the resources may not exist any more; e.g. dynamic resources, etc.  Also, it would break the offline features we are attempting to provide with service worker.

I propose we do something like the following:

1) Security libs provide an API that takes the nsISupports security info from the nsIChannel and recreates the on-the-wire bytes.
2) If the API is only usable on the main thread, we may need to modify fetch() to set the on-the-wire bytes on the Response object instead of the nsISupports object.  If not, we can convert later off the main thread.  I assume this requires MT, though.
3) Begin storing the on-the-wire bytes in Cache API.
4) Modify service worker respondWith() code to convert from the on-the-wire bytes back into the nsISupports info when we recreate the nsIChannel.
5) Write a Cache API database migration that converts existing nsISupports serializations to the the on-the-wire bytes format.  The ability to deserialize the current version of the nsISupports security info would have to stay in the tree to support this migration for the forseeable future.  Given the way our current deserialization stuff works this might have to be a bit hacky.

This gets us to the place where we are storing the wire bytes which our libraries should be flexible enough to parse in a backward compatible way.

I'm happy to mentor and do reviews for this.

David, what do you think?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Andrew, do we have anyone who could help here if David's team needs more resources?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Note, Cache API already de-duplicates the security info.  We might also want to compress if the on-the-wire bytes are particularly verbose.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I think that sounds good, but I'm not entirely sure what all the on-the-wire bytes entails. Would that be the entire TLS handshake? I'm not sure we're set up to capture and replay that.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> I think that sounds good, but I'm not entirely sure what all the on-the-wire
> bytes entails. Would that be the entire TLS handshake? I'm not sure we're
> set up to capture and replay that.

Ok.  What does http cache store?  My impression was it was the cert as represented on the wire.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

My understanding is that the http cache stores the serialization of the security info (and if deserialization fails, I think it just treats it as a cache miss). The serialization includes the bytes of the certificate, but there's a fair bit of additional information, both that comes directly from properties of the TLS handshake (e.g. cipher suite, etc.) and that we compute (did verification succeed, is the certificate EV, etc.).

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Ok, then we need the security info to support some form of backward compatible loading.

One potential way to do this would be to version the typename of the xpcom interface.  Like nxIX509Certv2, etc.  This is ugly, but would allow the legacy values to be loaded and then converted to the new type.

Alternatively, if the nsIX509Cert deserialization could be made backward compatible itself, then we could get away with just UUID hacks like:

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd4fdadb313c

Or provide a completely different serialization approach for the security info like propose din comment 2.  It doesn't have to be the wire bytes, but some format the security team can support in a backward compatible way.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Ok - it looks like we have a way forward by re-purposing an unused byte in the nsSSLStatus serialization as a version number. Given that, we can read (or not) the new fields as appropriate.

Comment 10

[Andrew Overholt [:overholt]]

(In reply to Ben Kelly [:bkelly] from comment #3)
> Andrew, do we have anyone who could help here if David's team needs more
> resources?

I'd prefer getting the right people involved when necessary.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Given comment 9 I'm going to mark this WONTFIX.  We can open a new bug if someone feels we need to do this in the future if that solution stops working for some reason.