Closed Bug 466011 Opened 11 years ago Closed 3 years ago

nsICertOverrideService.idl doesn't make it clear how overrideBits work

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: johnath, Assigned: johnath)

Details

(Whiteboard: [has patch][psm-assigned])

Attachments

(1 file, 2 obsolete files)

Attached patch Change comment (obsolete) — Splinter Review
Mossop was confused earlier today when trying to interact with nsICertOverrideService because he expected the overridebits he supplied to be treated as a set, instead of an exact match. That is, if he specified UNTRUSTED and MISMATCH, he expected MISMATCH, UNTRUSTED, MISMATCH+UNTRUSTED to all match.

I'll attach a vaguely improved comment.
Attachment #349239 - Flags: review?(kaie)
Whiteboard: [has patch][needs review kaie]
Might also be worth adding documentation for the aTemporary argument.
Since bug 488480 is another comment-only change to this IDL, I figured I'd roll them together and get it out of my patch queue.
Attachment #349239 - Attachment is obsolete: true
Attachment #382957 - Flags: review?(kaie)
Attachment #349239 - Flags: review?(kaie)
Comment on attachment 382957 [details] [diff] [review]
Updated to current trunk, this also fixes bug 488480

I have a question on this one:


>+   *  Each override is specific to exactly the errors overridden, so
>+   *  overriding everything won't match certs at the given host:port
>+   *  which only exhibit some subset of errors.
>+   *


Consider the following scenario:

1) I add exceptions for a particular host:port combination for all the 3 errors individually in three successive calls to rememberValidityOverride(). Now from what I know all the 3 exceptions will be persisted.

2) After this, a certificate error is detected for the same host:port with two errors -- say ERROR_UNTRUSTED and ERROR_MISMATCH. Now will the 3 exceptions added previously nullify this cert error and allow continuing the load?

From the comment its clear that if I add an exception by ORing all the 3 errors in a single call to rememberValidityOverride and later only one or two errors have actually occurred with the cert, the exception will not work. Thinking on the same lines the exception should not work in the previously mentioned scenario. But, may be explicitly clarifying it will help.

Regards,
Brahmana
Kai - review poke here? It's just comments. Honza want to take a crack at it?
(In reply to comment #4)
> Kai - review poke here? It's just comments. Honza want to take a crack at it?

Johnathan, taking a look at this.
So, the service actually "works" the way it has been described in comment 0. But, it is not the service itself that says it has to be exact set, the problem is here, in the bad cert handler:

http://hg.mozilla.org/mozilla-central/annotate/c63b24afe2c0/security/manager/ssl/src/nsNSSIOLayer.cpp#l3353

We do remaining_display_errors -= overrideBits. If it were doing remaining_display_errors &= ~overrideBits it would not be an exact match but a bit mask.

According to bug 327181 comment 101, the bug where this code has been originally created, Kai claims it is just a bit manipulation optimization. It has changed between version 6 and version 7 of the patch like this, see:

https://bugzilla.mozilla.org/attachment.cgi?id=281307&action=diff#mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp_sec10, line 2451 (the unoptimized version)

and 

https://bugzilla.mozilla.org/attachment.cgi?id=281800&action=diff#mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp_sec10, line 2454 (the optimized, current version)

Patch v6 does what we would expect and what current version of the IDL describes.

Patch v7 does not, and according to the comment 101 in that bug it seems to be just because of a bit manipulation optimization.


Kai, is use of -= in the bad cert handler this way intentional?
Let me make some statements, that will hopefully simplify the discussion that has started in the previous comments.

The implementation of nsICertOverrideService is a dumb service that remembers pieces of information, and makes them persistent. That's all. 

The service does not make the decisions whether remembered override bits should result in allowing or rejecting a connection!

The actual decision will be made by the PSM's communication layer. There is no need to manifest the application behavior in the IDL of the storage service.

Because of this, the following snippet
   *  Each override is specific to exactly the errors overridden, so
   *  overriding everything won't match certs at the given host:port
   *  which only exhibit some subset of errors.
should be removed.

Note, the primary key of entries in the service is host+port.
The OverrideBits-tuple is NOT part of the key.
(In reply to comment #3)
> I have a question on this one:
> 
> 
> >+   *  Each override is specific to exactly the errors overridden, so
> >+   *  overriding everything won't match certs at the given host:port
> >+   *  which only exhibit some subset of errors.
> >+   *
> 
> 
> Consider the following scenario:
> 
> 1) I add exceptions for a particular host:port combination for all the 3 errors
> individually in three successive calls to rememberValidityOverride(). Now from
> what I know all the 3 exceptions will be persisted.
> 
> 2) After this, a certificate error is detected for the same host:port with two
> errors -- say ERROR_UNTRUSTED and ERROR_MISMATCH. Now will the 3 exceptions
> added previously nullify this cert error and allow continuing the load?


The task intended to be done in this bug report is to improve a comment related to a storage service. Your question is related to the behaviour of the communication implementation, and is outside the scope of this bug.

But let me answer your question anyway:
The service will always "replace" older exceptions. The old set of errors and cert is erased, and the new set of errors and the new cert will be stored. There is no merging of error bits.
(In reply to comment #6)
> So, the service actually "works" the way it has been described in comment 0.

It's not the service who works that way, it's the separate PSM code that uses information from the override service and the current verification result and makes a decision.


> But, it is not the service itself that says it has to be exact set, the problem
> is here, in the bad cert handler:


Correct, outside of the override-storage service.


> Kai, is use of -= in the bad cert handler this way intentional?

The use of -= is simply to derive what errors are not yet overriden and should be reported to the user, when we encounter new, additional errors. Is the code wrong, if some of the old errors are no longer applicable? If yes, that's a separate bug, and we should use the bitmask approach.
Comment on attachment 382957 [details] [diff] [review]
Updated to current trunk, this also fixes bug 488480

If you remove this block, r=kaie

>+   *  Each override is specific to exactly the errors overridden, so
>+   *  overriding everything won't match certs at the given host:port
>+   *  which only exhibit some subset of errors.
Attachment #382957 - Flags: review?(kaie) → review-
Whiteboard: [has patch][needs review kaie] → [has patch]
I came here from reading bug 488480. Now I see that the summary in this bug report is "make it clear how override bits work".

Well, as said before, I would prefer not to state it in the override service.
I would prefer to resolve this particular bug as "wontfix", and fix only bug 488480 (but I'm not picky where the patch lives).

I hope my preference is reasonable. If you would like to make sure future readers of the service won't have the same question, I'd add the following comment somewhere in the .idl:

"This is a storage service for remembered certificate errors. Making decisions which sets of error bits are equivalent is outside the scope of this service."
(In reply to comment #9)
> The use of -= is simply to derive what errors are not yet overriden and should
> be reported to the user, when we encounter new, additional errors. Is the code
> wrong, if some of the old errors are no longer applicable? If yes, that's a
> separate bug, and we should use the bitmask approach.

Reported bug 571034 on it.
No longer blocks: 488480
Keywords: checkin-needed
Whiteboard: [has patch] → [has patch][psm-assigned]
https://hg.mozilla.org/mozilla-central/rev/ea1a774d53d5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.