386654 - Implement notification for EV certs

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

As of today, the security status tracking code in PSM uses nsISecurityEventSink::onSecurityChange to notify consumers about the security state of a web page.

This function is defined as
     void onSecurityChange(in nsISupports i_Context, 
                           in unsigned long state);

The possible state values are defined nsIWebProgressListener.
Unfortunately nsIWebProgressListener is a frozen interface!

How should PSM communicate the additional EV status to the consumers?

Ideally the notification of all status information should be done with a single function call. It simplifies the life of event consumers if they receive information "secure page" and "uses EV cert" at the same time.


In the past I had considered to extend the definition of parameter "state" with additional bits. But this approach has problems. While nsISecurityEventSink is NOT frozen, the definition of states in nsIWebProgressListener is frozen. The implementation of event consumers in Firefox and SeaMonkey doesn't look for bits, they look for absolute values. It's likely that other implementations do the same. By adding new bits we confuse the existing implementation, with the risk that noone notices.


Because nsISecurityEventSink is NOT frozen, we could change the existing API to use an additional parameter and force all implementations to update their code. 


Or we could implement a new nsISecurityEventSink2 interface, with an extended onSecurityChange method. PSM could attempt to QueryInterface the listener to the latter interface, which includes notification about EV, and possibly fall back to the older API.


What should we do?

Comment 1

[Kai Engert (:KaiE:)]

Besides the decision about notification API to external consumers, there is more work that I want to do in the scope of this bug.


It is an expensive operation to make the decision whether a certificate is to be treated as EV or not.

In this initial implementation I limit the cert analysis to the cert used by the top level document of the page.

As only the document loading tracker is able to know whether a given SSL socket is the top level document of a web page, this is the place where I'm requesting the EV analysis.

In order to enable this late analysis, I'm adding a reference to the cert to each channel's security status object.

Comment 2

[Kai Engert (:KaiE:)]

Hmm, the issue is more complicated than I thought.

The security notification is not sent directly from producer (PSM) to end consumers (like browser UI). In the middle sits nsDocLoader which makes a translation to another interface

PSM calls
  nsISecurityEventSink {
    void onSecurityChange(in nsISupports i_Context, in unsigned long state);
  }

uriloader listens for that, iterates over its list of all registered web progress listeners and calls each with
  nsIWebProgressListener {
    void onSecurityChange(in nsIWebProgress aWebProgress,
                          in nsIRequest aRequest,
                          in unsigned long aState);
  }


If we were to introduce a new interface nsISecurityEventSink2, we'd have to change nsDocLoader to implement that as well.
In addition we'd have to extend nsIWebProgressListener2 with a new onSecurityChange method that carries EV state.
The new nsDocLoader::OnSecurityChange would attempt to QI each consumer to nsIWebProgressListener2 to make a decision whether to call the old or new function...

Comment 3

[Kai Engert (:KaiE:)]

I'm taking a step back to my original simple approach.

I've been told that we added new bits to frozen interfaces in the past, too.

In addition, the comment for parameter state of nsIWebProgressListener::onSecurityChange explicitly says:

  "Any undefined bits are reserved for future use."


I'm therefore going to declare it a bug if existing consumers check for absolute values (instead for bits).

I'll attach a patch that implements the simple approach.

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 5

[Kai Engert (:KaiE:)]

Attachment: ui hack for testing purposes

This patch is NOT meant for check in, I only use it for testing.
It is an example how browser could catch the new event and bring up better UI for it.

It also changes handling of existing states to look for bits set (instead of looking for absolute values).

Comment 6

[Kai Engert (:KaiE:)]

Adding Benjamin to cc list because the proposed patch adds a new state flag to nsIWebProgressListener(2) in uriloader.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: stub for nsNSSSocketInfo::GetIsExtendedValidation

I'm attaching this little helper patch that allows you to get Patch v1 compiled.

Patch v1 requires the implementation of nsNSSSocketInfo::GetIsExtendedValidation.

At this time, no working implementation is available. We will focus on getting that done in bug 374336. So for the time being and while testing interfaces, feel free to use this stub.

Comment 8

[Kai Engert (:KaiE:)]

(In reply to comment #3)
> I'm therefore going to declare it a bug if existing consumers check for
> absolute values (instead for bits).

I filed bug 386681 to get that fixed in the places I found.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 270665 [details] [diff] [review]
Patch v1

Bob, can you please review?

Boris, can you please r/sr the change in uriloader?

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 270668 [details] [diff] [review]
stub for nsNSSSocketInfo::GetIsExtendedValidation

Bob, can you please review this stub as well?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Why are we putting the bit on nsIWebProgressListener2 if we're going to send the notification to implementations of nsIWebProgressListener?

Comment 12

[Kai Engert (:KaiE:)]

(In reply to comment #11)
> Why are we putting the bit on nsIWebProgressListener2 if we're going to send
> the notification to implementations of nsIWebProgressListener?

Because nsIWebProgressListener is frozen. However, the API mentions the possibility to add more flags in the future.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Frozen doesn't mean you can't add flag bits to it if the API provides for such.  We do that all the time.  In particular, see rev 1.17 of nsIWebProgressListener.idl.

I'd prefer that we put the flag on that interface, and if desired added a comment that that flag only appears in Gecko 1.9 and later.

Comment 14

[Robert Relyea]

Comment on attachment 270668 [details] [diff] [review]
stub for nsNSSSocketInfo::GetIsExtendedValidation

r+ = rrelyea.

Comment 15

[Robert Relyea]

Comment on attachment 270665 [details] [diff] [review]
Patch v1

r+ on the psm part of this patch, assuming the stub is checked in first. and bzb's interface issues are addressed.

Comment 16

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Differences from Patch v1:

- moved flag to existing idl file
- renamed flag to make it more obvious this is related to identity
- now includes the stub, which I had attached separately.

Carrying forward r=rrelyea

Boris, does this look ok?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275445 [details] [diff] [review]
Patch v2

sr=bzbarsky

Comment 18

[Kai Engert (:KaiE:)]

Attachment: Patch v2 with merging conflicts fixed

I'm in merging hell. This patch slightly collides with the work I'm doing in bug 327181.

This is a new patch that still works correctly, has code just slightly moved, but is free of conflict with my patch from bug 327181.

I'd appreciate being able to land this on trunk soon, thanks in advance for considering to approve :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 276552 [details] [diff] [review]
Patch v2 with merging conflicts fixed

a=bzbarsky.  Yay finally having PSM in the approval queries!

Comment 20

[Kai Engert (:KaiE:)]

fixed on trunk