the override in CheckCertOverridesbits are not correctly cleared

RESOLVED FIXED in Firefox 16

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox15 affected, firefox16+ fixed, firefox17+ fixed, firefox18 fixed, firefox-esr10 wontfix)

Details

(Whiteboard: [qa-])

Attachments

(6 attachments, 14 obsolete attachments)

1.15 KB, patch
briansmith
: review+
mayhemer
: superreview+
Details | Diff | Splinter Review
1.15 KB, patch
briansmith
: review+
briansmith
: superreview+
Details | Diff | Splinter Review
1.15 KB, patch
Details | Diff | Splinter Review
7.61 KB, image/jpeg
Details
28.10 KB, text/plain
Details
58.27 KB, patch
mayhemer
: review+
mayhemer
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 654830 [details]
Fix to the bug

The heckCertOverridesbits function uses a substraction isntead of a bitwise maninulation for clearing overrides. This causes that when there are overrides than the currently enabled are found (such as when libpix is enabled or if the cert changes) the override service does not returned cleared even when that should be the case.

Steps to reproduce (on a fresh checkout)
1. Enable libpkix verfication in /modules/libpref/src/init/all.js 
2. recompile 
3. Run the test_sdpy.js xpcshell test case (you need node.js).
4. The test will fail.
(Assignee)

Comment 1

5 years ago
Created attachment 654831 [details] [diff] [review]
Fix to the bug
Attachment #654830 - Attachment is obsolete: true
Attachment #654831 - Flags: review+
(Assignee)

Updated

5 years ago
Assignee: nobody → cviecco
(Assignee)

Updated

5 years ago
Attachment #654831 - Flags: review+ → review?(honzab.moz)
(Assignee)

Comment 2

5 years ago
This a two byter.. already r+ by ryan smith, now asking Honza for second review so that I can land.
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

Please use r? and sr? to handle the two-review requirement in security/.
Attachment #654831 - Flags: superreview?(honzab.moz)
Attachment #654831 - Flags: review?(honzab.moz)
Attachment #654831 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 651246
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

Review of attachment 654831 [details] [diff] [review]:
-----------------------------------------------------------------

See also https://bugzilla.mozilla.org/show_bug.cgi?id=571034#c1.  That bug is effectively a dup of this one, btw.

sr=honzab.
Attachment #654831 - Flags: superreview?(honzab.moz) → superreview+
Duplicate of this bug: 571034
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a989993c992
Target Milestone: --- → mozilla18
status-firefox-esr10: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
Target Milestone: mozilla18 → ---
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is a longstanding bug.

User impact if declined: The user may encounter the "certificate error" page two or three times (over a period of time) instead of just once, even if they override the error. (This is long-standing behavior.)

Testing completed (on m-c, etc.): This just landed on inbound.

Risk to taking this patch (and alternatives if risky): No risk. This is a trivial change that is an obviously improvement.

String or UUID changes made by this patch: none.
Attachment #654831 - Flags: approval-mozilla-beta?
Attachment #654831 - Flags: approval-mozilla-aurora?

Comment 8

5 years ago
(In reply to Brian Smith (:bsmith) from comment #7)
> User impact if declined: The user may encounter the "certificate error" page
> two or three times (over a period of time) instead of just once, even if
> they override the error. (This is long-standing behavior.)

Is the concern here that bug 650355 will make the certificate error much more common?

Comment 9

5 years ago
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.
Attachment #654831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox17: ? → +
(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Brian Smith (:bsmith) from comment #7)
> > User impact if declined: The user may encounter the "certificate error" page
> > two or three times (over a period of time) instead of just once, even if
> > they override the error. (This is long-standing behavior.)
> 
> Is the concern here that bug 650355 will make the certificate error much
> more common?

Yes, that's right.

Updated

5 years ago
tracking-firefox16: ? → +
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Triage Comment]
Let's get bugs like this nominated sooner in the release cycle. Approving for Beta since this is a trivial fix for what may become an annoying "regression".

If you know of an md5 certificate site that QA can use to verify, please include here.
Attachment #654831 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/1a989993c992
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Wait, |!overrideBits| and not |~overrideBits|?

Updated

5 years ago
status-firefox18: affected → fixed
(In reply to :Ms2ger from comment #13)
> Wait, |!overrideBits| and not |~overrideBits|?

Camilo, please fix this ASAP.

Thanks, Ms2ger.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

5 years ago
Created attachment 665548 [details] [diff] [review]
fix-to-commited patch. Use bitwise NOT instead of logic NOT
Attachment #665548 - Flags: review?(bsmith)
(Assignee)

Comment 16

5 years ago
Created attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

Added two patches for you to choose. this one Assumes the first patch was not placed before
Attachment #665550 - Flags: review?(bsmith)
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Triage Comment]
Given the churn in this bug, setting the aurora/beta flags back to ?. This patch was meant to be very low risk, but has since been reopened.
Attachment #654831 - Flags: approval-mozilla-beta?
Attachment #654831 - Flags: approval-mozilla-beta+
Attachment #654831 - Flags: approval-mozilla-aurora?
Attachment #654831 - Flags: approval-mozilla-aurora+
Comment on attachment 665548 [details] [diff] [review]
fix-to-commited patch. Use bitwise NOT instead of logic NOT

Review of attachment 665548 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d87924d4760
https://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5
https://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039

OK, I didn't see Alex's last comment until I'd already checked in the fixed version of the patch into all three channels. Shall I back them out?

It was definitely silly that three different people missed "!" vs "~" in the patch but it's still very low risk. (Now even more risk, now that it is correct!)
Attachment #665548 - Flags: superreview+
Attachment #665548 - Flags: review?(bsmith)
Attachment #665548 - Flags: review+
Attachment #665548 - Flags: approval-mozilla-beta?
Attachment #665548 - Flags: approval-mozilla-aurora?
Attachment #665548 - Attachment description: fix-to-commited patch. Use bitwise or instead of logic or → fix-to-commited patch. Use bitwise NOT instead of logic NOT
Attachment #654831 - Flags: approval-mozilla-beta?
Attachment #654831 - Flags: approval-mozilla-aurora?
Attachment #665548 - Flags: approval-mozilla-beta?
Attachment #665548 - Flags: approval-mozilla-aurora?
Comment on attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

https://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5
https://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039

I already checked in this patch based on the previous approval for the one with the typo. See the previous comment.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding issue
User impact if declined: Under some conditions, certificate error pages may be shown more frequently than necessary and/or some certificate errors may not overridable.
Testing completed (on m-c, etc.): Manual. Just landed.
Risk to taking this patch (and alternatives if risky): Basically none (for real this time.)
String or UUID changes made by this patch: none.
Attachment #665550 - Flags: approval-mozilla-beta?
Attachment #665550 - Flags: approval-mozilla-aurora?
Comment on attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

[Triage Comment]
We're going to trust your intuition and leave it approved, to prevent a frustrating UX.

Please instruct QA on the best way to verify and find regressions.
Attachment #665550 - Flags: approval-mozilla-beta?
Attachment #665550 - Flags: approval-mozilla-beta+
Attachment #665550 - Flags: approval-mozilla-aurora?
Attachment #665550 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Keywords: qawanted, verifyme
https://hg.mozilla.org/mozilla-central/rev/5d87924d4760
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 22

5 years ago
Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5
Pushed to Beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039
status-firefox16: affected → fixed
status-firefox17: affected → fixed
(In reply to Alex Keybl [:akeybl] from comment #20)
> Please instruct QA on the best way to verify and find regressions.

Brian - still waiting on this.
Created attachment 666707 [details]
T-Shirt for PSM hackers

I think I'm gonna by one!
cviecco, please either write an automated test or respond on comment 23.

If you write an automated test, I think you can do so in a mochitest by directly calling into nsICertOverrideService to set a cert override for some invalid cert that has only one bit set (e.g. expired) for a cert that has two problems (e.g. expired + untrusted issuer), then open up an SSL connection. The test should fail without your patch and it should succeed with your patch. See toolkit/mozapps/extensions/test/browser/browser_installssl.js for an example.

For this, you need to generate a new certificate that has the two errors. This is good because I think you've already done similar work in bug 699874.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

5 years ago
Was out of the office until today.

Seems like writing a mochitest would be possible. And Yes I am going to get one of those shirts.

Shall I open a follow bug for the tests?
Just use this bug.
BTW, you might not need a cert with two problems; it might be the case that you simply need a cert that has a different problem (e.g. expired) than what the override is for.
(Assignee)

Comment 29

5 years ago
Created attachment 667619 [details] [diff] [review]
test-cases part1

This tests the override service for all certoverride bits but only for
1. expired
2. unstrusted
3. expired and mismatch
4. unstrusted and mismatch

Three cases are pending (adding mismatch only, expired and untrusted and all wrong), but those require changing the pgo cert db. This patch is just a preview.
Attachment #667619 - Flags: feedback?(bsmith)
(Assignee)

Comment 30

5 years ago
Created attachment 667687 [details] [diff] [review]
now with full testing of all combinations (certdb patched).
(Assignee)

Comment 31

5 years ago
Created attachment 667733 [details] [diff] [review]
test-certoverrides
Attachment #667619 - Attachment is obsolete: true
Attachment #667687 - Attachment is obsolete: true
Attachment #667619 - Flags: feedback?(bsmith)
Attachment #667733 - Flags: review?(bsmith)
Comment on attachment 667733 [details] [diff] [review]
test-certoverrides

Review of attachment 667733 [details] [diff] [review]:
-----------------------------------------------------------------

Great work!

Please describe what you did with pgocerts to modify the cert and key databases.

Please attach a log showing the output of this test when your patch for this bug is not applied (i.e. when we use operator- instead of operator&), to show which tests fail (or add a link to a tryserver run that shows that this test fails without your fix.)

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test certificate overrides</title>
> +  <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>        

Fix trailing whitespace highlighted by splinter everywhere in this file. Also, many parts of this file do not follow the style guide, regarding camelCase naming and whitespace conventions. Please fix that.

Also, restrict the number of blank lines between functions to one.

Remove commented-out code.

@@ +15,5 @@
> +//const Cu = Components.utils;
> +//const Cr = Components.results;
> +
> +var testHost=[];
> +testHost[1]="untrusted.example.com";

s/testHost/testHosts/

@@ +16,5 @@
> +//const Cr = Components.results;
> +
> +var testHost=[];
> +testHost[1]="untrusted.example.com";
> +//mismatch is 2

what does this comment mean?

@@ +49,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports))

Add braces around body of statement to make things easier to read.

@@ +56,5 @@
> +  },
> +  notifyCertProblem: function(socketInfo, sslStatus, targetHost) {
> +    certerrorbits =  (sslStatus.QueryInterface(Ci.nsISSLStatus).isUntrusted ) |
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isDomainMismatch <<1 ) | 
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isNotValidAtThisTime <<2 );            

QueryInterface once.

certErrorBits = 0;
if (sslStatus.isUntrusted) certErrorBits |= Ci.nsICertOverrideService.ERROR_UNTRUSTED;
...

This way, we don't have to worry about whether ERROR_UNTRUSTED is 1, etc.

@@ +77,5 @@
> +     } else {
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);

None of the above lines in this block should be in the try block. We want the test to fail if any of them fail.

@@ +78,5 @@
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);

missing ok(false, "the request should have failed");

@@ +80,5 @@
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);
> +   } catch (e) {
> +     // This will fail since the server is not trusted yet

missing ok(true, ...);

@@ +85,5 @@
> +   }
> + }
> +
> +
> +///////////////////////

remove this line and extra blank lines.

@@ +87,5 @@
> +
> +
> +///////////////////////
> +
> +function xhrMustSucceed(domain,message){

why not:

   function xhr(domain, message, mustSucceed)

and...

@@ +96,5 @@
> +   try{
> +        req.open("GET", "https://"+domain+"/",false);
> +        req.send(null);
> +        var headers = req.getAllResponseHeaders();
> +        ok(true, "Page Load success  "+message);

ok(mustSuceeded, "Page load success " + message);

and...

@@ +99,5 @@
> +        var headers = req.getAllResponseHeaders();
> +        ok(true, "Page Load success  "+message);
> +   }
> +      catch(err){
> +         ok(false, "Page failed to load it should succeed message="+message);

ok(!mustSucceed, "page load failed " + message);

and...

@@ +103,5 @@
> +         ok(false, "Page failed to load it should succeed message="+message);
> +  };
> +}
> +
> +function xhrMustFail(domain,message){

...remove this function.

@@ +119,5 @@
> +  };
> +}
> +
> +
> +function doConnectTests(overridebits){

Write a comment about what this function is supposed to do.

Instead of having this function loop through the test hosts, why not put the loop inside testCertOverrides, so that this function would take (testHost, overrideBits) as parameters? This would make it clearer that we're testing the cross product of hosts * override bits, and it would make this function simpler.

@@ +121,5 @@
> +
> +
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);

might as well make cos a global variable that is initialized in the beginning, so we don't have to repeat this in every function.

@@ +123,5 @@
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);
> +    for(var i=1;i<8;i++){
> +      if(typeof testHost[i] == 'undefined'){

Why?

@@ +138,5 @@
> +      else{
> +        xhrMustFail(host,"override host="+host+status_info);
> +      }
> +      //reset overrides
> +      cos.clearValidityOverride(host,443); 

isn't this redundant?

@@ +144,5 @@
> +
> +}
> +
> +
> +function checkHostErrorbits(hostindex) {

capitalization in name.

What is this function supposed to do, exactly? Please add a comment.

and, why not:

    function checkHostErrorBits(host)

so that this function doesn't need to know about the array at all.

@@ +148,5 @@
> +function checkHostErrorbits(hostindex) {
> +   var port =443;
> +   var bits = 15;
> +
> +   if(typeof testHost[hostindex] == 'undefined'){

Why?

@@ +167,5 @@
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);

ok(false, ...);

@@ +169,5 @@
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);
> +   } catch (e) {
> +     // This will fail since the server is not trusted yet

ok(true, ...);

@@ +174,5 @@
> +   }
> +   //now compare
> +   is(certerrorbits,hostindex,"errorbits="+certerrorbits);
> +   //and reset host the state
> +       var cos = Cc["@mozilla.org/security/certoverride;1"].

indention.

@@ +178,5 @@
> +       var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);
> +   cos.clearValidityOverride(host,443);
> + }
> +

Too many blank lines (here and elsewhere).

@@ +182,5 @@
> +
> +
> +
> +function testCertOverrides(){
> +  for (var i=1;i<8;i++){

var allBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED | 
              Ci.nsICertOverrideService.ERROR_MISMATCH | 
              Ci.nsICertOverrideService.ERROR_TIME;

// Loop through every combination of overrides
for (var i = 1; i < allBits; i++) {

@@ +194,5 @@
> +
> +function onWindowLoad()
> +{
> +/*
> +*/

:(
Attachment #667733 - Flags: review?(bsmith) → review-
Duplicate of this bug: 418563
(Assignee)

Comment 34

5 years ago
Comment on attachment 667733 [details] [diff] [review]
test-certoverrides

Review of attachment 667733 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +16,5 @@
> +//const Cr = Components.results;
> +
> +var testHost=[];
> +testHost[1]="untrusted.example.com";
> +//mismatch is 2

personal comment (what is the bit value of mismatched?) , removed.

@@ +49,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports))

done

@@ +56,5 @@
> +  },
> +  notifyCertProblem: function(socketInfo, sslStatus, targetHost) {
> +    certerrorbits =  (sslStatus.QueryInterface(Ci.nsISSLStatus).isUntrusted ) |
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isDomainMismatch <<1 ) | 
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isNotValidAtThisTime <<2 );            

We still need to worry for the hostindex, but I like this as it removes one more place to look.

@@ +77,5 @@
> +     } else {
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);

agreed.done

@@ +85,5 @@
> +   }
> + }
> +
> +
> +///////////////////////

ack, done

@@ +87,5 @@
> +
> +
> +///////////////////////
> +
> +function xhrMustSucceed(domain,message){

makes sense. Will do

@@ +119,5 @@
> +  };
> +}
> +
> +
> +function doConnectTests(overridebits){

agreed

@@ +121,5 @@
> +
> +
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);

done, now global const.

@@ +123,5 @@
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);
> +    for(var i=1;i<8;i++){
> +      if(typeof testHost[i] == 'undefined'){

testHost was sparse at one moment.

@@ +138,5 @@
> +      else{
> +        xhrMustFail(host,"override host="+host+status_info);
> +      }
> +      //reset overrides
> +      cos.clearValidityOverride(host,443); 

no, actually the first one would be the reduntant. as I assume that none of the hosts have overrides to begin with.

@@ +144,5 @@
> +
> +}
> +
> +
> +function checkHostErrorbits(hostindex) {

The function would then need the expected errors (they match the host index) and the signarue would be
 function checkHostErrors(host,expectedErrors).

@@ +148,5 @@
> +function checkHostErrorbits(hostindex) {
> +   var port =443;
> +   var bits = 15;
> +
> +   if(typeof testHost[hostindex] == 'undefined'){

because initial versions of the hostindex was not filled completelly (missing tesing cases) and one more extra check seemed innocuous.

@@ +167,5 @@
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);

Yep missed that one

@@ +169,5 @@
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);
> +   } catch (e) {
> +     // This will fail since the server is not trusted yet

I can add it but is seems redundant.

@@ +182,5 @@
> +
> +
> +
> +function testCertOverrides(){
> +  for (var i=1;i<8;i++){

ok for (var i = 1; i <= allBits; i++)  (or 'for (var i = 1; i < allBits+1; i++)' )

@@ +194,5 @@
> +
> +function onWindowLoad()
> +{
> +/*
> +*/

So sad...... Yes I agree
Comment on attachment 667733 [details] [diff] [review]
test-certoverrides

Review of attachment 667733 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +138,5 @@
> +      else{
> +        xhrMustFail(host,"override host="+host+status_info);
> +      }
> +      //reset overrides
> +      cos.clearValidityOverride(host,443); 

Tests should always assume that the previous test messed up and didn't clean up after itself (because some tests are buggy and don't clean up properly). So, you should clear all the overrides at the very start of the test as well.

@@ +182,5 @@
> +
> +
> +
> +function testCertOverrides(){
> +  for (var i=1;i<8;i++){

Good catch. (I prefer <=, but I am not one to nit-pick.)
(Assignee)

Comment 36

5 years ago
Created attachment 668156 [details] [diff] [review]
test-certoverrides v2
(Assignee)

Comment 37

5 years ago
Created attachment 668166 [details] [diff] [review]
test-certoverrides v2.1
Attachment #667733 - Attachment is obsolete: true
Attachment #668156 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
Created attachment 668173 [details]
output of tests with original values (failed tests)

This is the output of running the test with: 
'TEST_PATH=security/ssl/bugs/test_certificate_overrides.html make -C obj-ff-dbg/ mochitest-chrome > /tmp/badout.txt'

the things to notice are the expected fail readings, notice 12 test fail.
(Assignee)

Comment 39

5 years ago
Created attachment 668195 [details] [diff] [review]
test-certoverrides v2.2
Attachment #668166 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #668195 - Flags: review?(bsmith)
Comment on attachment 668195 [details] [diff] [review]
test-certoverrides v2.2

Review of attachment 668195 [details] [diff] [review]:
-----------------------------------------------------------------

Great work. r+ with comments addressed.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +24,5 @@
> +testHost[      em | eu ] = "mismatch.untrusted.example.com";
> +testHost[ et           ] = "expired.example.com";
> +testHost[ et      | eu ] = "untrusted-expired.example.com";
> +testHost[ et | em      ] = "mismatch.expired.example.com";
> +testHost[ et | em | eu ] = "mismatch.untrusted-expired.example.com";

You make testing for cert overrides so pretty!

@@ +96,5 @@
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +             .createInstance(Ci.nsIXMLHttpRequest);
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);

spaces around "+" (two places).

@@ +98,5 @@
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);
> +    req.send(null);
> +    var headers = req.getAllResponseHeaders();

why are you grabbing the headers?

@@ +106,5 @@
> +  }
> +}
> +
> +function checkHostConnect(hostIndex, overrideBits) {
> +  if (typeof testHost[hostIndex] == 'undefined') {

can this happen?

@@ +115,5 @@
> +  var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits;
> +
> +  addCertOverride(host, 443, overrideBits);
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);

whitespace.

@@ +118,5 @@
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);
> +  }
> +  else {
> +    xhrConnect(host,"override host=" + host + statusMessage,false);

whitespace.

@@ +145,5 @@
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits,expectedErrorBits,"errorbits=" +certErrorBits + " host=" + host);

spaces.

@@ +150,5 @@
> +  //and reset host the state
> +  cos.clearValidityOverride(host, port);
> +}
> +
> +function testCertOverrides(){

() {
Attachment #668195 - Flags: superreview?(honzab.moz)
Attachment #668195 - Flags: review?(bsmith)
Attachment #668195 - Flags: review+
(Assignee)

Comment 41

5 years ago
Comment on attachment 668195 [details] [diff] [review]
test-certoverrides v2.2

Review of attachment 668195 [details] [diff] [review]:
-----------------------------------------------------------------

Should I submit a new version with fixed or wait for Honza's sr?

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +96,5 @@
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +             .createInstance(Ci.nsIXMLHttpRequest);
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);

Ack. Done

@@ +98,5 @@
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);
> +    req.send(null);
> +    var headers = req.getAllResponseHeaders();

It was for the failure, but it should be removed.

@@ +106,5 @@
> +  }
> +}
> +
> +function checkHostConnect(hostIndex, overrideBits) {
> +  if (typeof testHost[hostIndex] == 'undefined') {

it should never happen, is just a fail safe in case we ever try to use an invalid hostIndex. I can remove if you wish.

@@ +115,5 @@
> +  var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits;
> +
> +  addCertOverride(host, 443, overrideBits);
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);

ACK. Done

@@ +118,5 @@
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);
> +  }
> +  else {
> +    xhrConnect(host,"override host=" + host + statusMessage,false);

ACK. Done

@@ +145,5 @@
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits,expectedErrorBits,"errorbits=" +certErrorBits + " host=" + host);

ACK. (grrrr, what is that utility that you mentioned to help me catch these?)

@@ +150,5 @@
> +  //and reset host the state
> +  cos.clearValidityOverride(host, port);
> +}
> +
> +function testCertOverrides(){

ACK, Done
(Assignee)

Comment 42

5 years ago
Created attachment 670126 [details] [diff] [review]
test-certoverrides v3
Attachment #668195 - Attachment is obsolete: true
Attachment #668195 - Flags: superreview?(honzab.moz)
Attachment #670126 - Flags: superreview?(honzab.moz)
Attachment #670126 - Flags: review?(bsmith)
(Assignee)

Comment 43

5 years ago
Comment on attachment 670126 [details] [diff] [review]
test-certoverrides v3

one typeo removing review requests
Attachment #670126 - Flags: superreview?(honzab.moz)
Attachment #670126 - Flags: review?(bsmith)
(Assignee)

Comment 44

5 years ago
Created attachment 670405 [details] [diff] [review]
test-certoverrides v3.1
Attachment #670126 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #670405 - Flags: superreview?(honzab.moz)
Attachment #670405 - Flags: review?(bsmith)
Camilo, please don't obsolete the current patch, you would destroy my splinter comments.  Thanks.
(Assignee)

Comment 46

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #45)
> Camilo, please don't obsolete the current patch, you would destroy my
> splinter comments.  Thanks.

OK. Thank you
Comment on attachment 670405 [details] [diff] [review]
test-certoverrides v3.1

Review of attachment 670405 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this is a very good piece of work!

Please regenerate the certificates (where applicable) to expire not sooner then in 100 years.

Many formatting nits, please strictly follow this formatting, mainly missing or unnecessary white spaces and CRs:

gGlobalVariable;

function name()
{
  // Comment has to be a sentence.
  variable = 0;
  for (var i = 0; i < max; ++i) {
  }
}

Please provide certutil (or any other) commands you used to generate the certs, bug comment is enough.

::: build/pgo/server-locations.txt
@@ +97,5 @@
>  https://expired.example.com:443        privileged,cert=expired
>  https://requestclientcert.example.com:443         privileged,clientauth=request
>  https://requireclientcert.example.com:443         privileged,clientauth=require
> +https://mismatch.expired.example.com:443	privileged,cert=expired
> +https://mismatch.untrusted.example.com:443	privileged,cert=untrusted

You would be OK with the self signed cert we already have, but up to you.

@@ +98,5 @@
>  https://requestclientcert.example.com:443         privileged,clientauth=request
>  https://requireclientcert.example.com:443         privileged,clientauth=require
> +https://mismatch.expired.example.com:443	privileged,cert=expired
> +https://mismatch.untrusted.example.com:443	privileged,cert=untrusted
> +https://mismatch.example.com:443		privileged,cert=staticname

I think you are OK with using nocert.example.com host here.  Again, no need for a new cert here.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +26,5 @@
> +testHost[ et      | eu ] = "untrusted-expired.example.com";
> +testHost[ et | em      ] = "mismatch.expired.example.com";
> +testHost[ et | em | eu ] = "mismatch.untrusted-expired.example.com";
> +
> +var certErrorBits;

call this gCertErrorBits.

@@ +100,5 @@
> +    req.open("GET", "https://" + domain + "/", false);
> +    req.send(null);
> +    ok(expectedSuccess, "Page Load success  " + message + " expected=" + expectedSuccess);
> +  } catch (err) {
> +    ok(!expectedSuccess, "Page failed to load" + message + " expected=" + expectedSuccess);

Check spaces in logs...

@@ +113,5 @@
> +  var host = testHost[hostIndex];
> +  var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits;
> +
> +  addCertOverride(host, 443, overrideBits);
> +  if (0 == (hostIndex & ~overrideBits)) {

You can just put this condition to the last arg of xhrConnect.

@@ +141,5 @@
> +    req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +    req.send(null);
> +    ok(false, "Connection to host" + host + "succeeded when it should have failed");
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet

"Failure here is expected" is a bit better comment, but up to you.

@@ +144,5 @@
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits, expectedErrorBits, "errorbits=" +certErrorBits + " host=" + host);

I think you can do this check in checkHostConnect() when hostIndex == overrideBits after you call addCertOverride().  Then (g)certErrorBits must be equal to hostIndex, right?

This code is mostly a copy of what addCertOverride does.  Since this test makes a lot of requests (7*7+7=56) we may save those 7 at least.

@@ +145,5 @@
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits, expectedErrorBits, "errorbits=" +certErrorBits + " host=" + host);
> +  //and reset host the state

wording
Attachment #670405 - Flags: superreview?(honzab.moz)
(Assignee)

Comment 48

5 years ago
Created attachment 670859 [details] [diff] [review]
test-certoverrides v4

Addresses Comments by Honza


To generate the new cert:
cd build/pgo/certs/

../../../obj-ff-dbg/dist/bin/certutil -S -n "untrustedandexpired" -s "CN=untrusted-expired.example.com" -c "Unknown CA" -t "P,," -k rsa -g 1024 -m 98743 -v 0 -d .
(Assignee)

Comment 49

5 years ago
Created attachment 670862 [details] [diff] [review]
test-certoverrides v4.1
Attachment #670859 - Attachment is obsolete: true
(Assignee)

Comment 50

5 years ago
Created attachment 670872 [details] [diff] [review]
test-certoverrides v4.2
Attachment #670862 - Attachment is obsolete: true
(Assignee)

Comment 51

5 years ago
Comment on attachment 670872 [details] [diff] [review]
test-certoverrides v4.2

Addressed review of 3.1. 

-Renamed the certerror bits into  gCertErrorBits.
-Integrated the checks into a single function. (and this is why I am calling this a review) (removed 7 connections)
-Changed style.
-Fixed spaces in two of the log output strings.
-Remove one unnecessary cert from the database. (I am still using 'the untrusted cert' instead of the self-signed for the untrusted cert test).
-Added an extra test for the self-signed case.
Attachment #670872 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
Attachment #670405 - Flags: review?(bsmith)
Given there is an automated test for this, is there any necessity for QA to do a verification? If so, please advise on what we can check for in terms of regressions.
Keywords: qawanted
Comment on attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

This was fixed on all channels already using the other patch.
Attachment #665550 - Flags: review?(bsmith)
Comment on attachment 670872 [details] [diff] [review]
test-certoverrides v4.2

Review of attachment 670872 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping r, still not for to be landed.

You didn't regenerate the 'untrusted' and 'Unknown CA' certs.  Those will expire in 2019.

Please fix the last formatting issues before landing please.  

I personally liked some blank lines you have removed, but that is up to you.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +14,5 @@
> +              getService(Ci.nsICertOverrideService);
> +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
> +const em = Ci.nsICertOverrideService.ERROR_MISMATCH;
> +const et = Ci.nsICertOverrideService.ERROR_TIME;
> +//note:  the host index matches the expected error.

// Note: the host index matches the expected error.

@@ +46,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports)){

aIID.equals(Ci.nsISupports)) {

Space before {

@@ +82,5 @@
> +  req.open("GET", url, false);
> +  req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +  try {
> +    req.send(null);
> +    ok(false, "Connection to host" + host + "succeeded when it should have failed");

"Connection to host " + host + " succeeded when it should have failed"

@@ +103,5 @@
> +}
> +
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;

Why is this variable needed?

@@ +105,5 @@
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {

if (overridesMustEqualError) {

Space after if.

@@ +107,5 @@
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {
> +    is(gCertErrorBits, overrideBits, "Reported Error match: errorbits=" + gCertErrorBits + " host=" + host);
> +  }

Does it make any sense to check here that gCertErrorBits & ~overrideBits == 0 in an else branch?

@@ +124,5 @@
> +    for (var j = 1; j < testHost.length; ++j){
> +      var expectedError = j;
> +      var successExpected = (0 == (expectedError & ~overrideBits));
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch);

pass overrideBits instead of i?  and maybe testHost[j], but up to you.

@@ +127,5 @@
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch);
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.

// Now we test the self-signed. Must return an overridable untrusted error.

@@ +129,5 @@
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.
> +  cos.clearValidityOverride("self-signed.example.com", 443);
> +  checkHostConnect("self-signed.example.com", eu , successExpected, true);

checkHostConnect("self-signed.example.com", eu, successExpected, true);

Unnecessary white space after eu.
Attachment #670872 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #54)
> Please fix the last formatting issues before landing please.  

I thought "before resubmitting" of course.
status-firefox-esr10: affected → wontfix
(Assignee)

Comment 56

5 years ago
Comment on attachment 670872 [details] [diff] [review]
test-certoverrides v4.2

Review of attachment 670872 [details] [diff] [review]:
-----------------------------------------------------------------

I disagree with the regeneration of both  'untrusted' and 'Unknown CA' certs as these
where in the database before this change. If we desire to update the expiration of certs this should
be in a separate bug.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +14,5 @@
> +              getService(Ci.nsICertOverrideService);
> +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
> +const em = Ci.nsICertOverrideService.ERROR_MISMATCH;
> +const et = Ci.nsICertOverrideService.ERROR_TIME;
> +//note:  the host index matches the expected error.

OK. Agreed. fixed

@@ +46,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports)){

Agreed Done.

@@ +82,5 @@
> +  req.open("GET", url, false);
> +  req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +  try {
> +    req.send(null);
> +    ok(false, "Connection to host" + host + "succeeded when it should have failed");

Fixed

@@ +103,5 @@
> +}
> +
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;

Is not really needed. Just added it for readability. Can remove if you wish.

@@ +105,5 @@
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {

Done

@@ +107,5 @@
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {
> +    is(gCertErrorBits, overrideBits, "Reported Error match: errorbits=" + gCertErrorBits + " host=" + host);
> +  }

I think that is unncecesary.

@@ +127,5 @@
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch);
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.

Fixed.

@@ +129,5 @@
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.
> +  cos.clearValidityOverride("self-signed.example.com", 443);
> +  checkHostConnect("self-signed.example.com", eu , successExpected, true);

Done fixed.
(Assignee)

Comment 57

5 years ago
Created attachment 676198 [details]
test-certoverrides v4.3

Solves issues on JS of version 4.2. Does NOT update the db from version 4.2 (ie does not update the certs already in the db).
(Assignee)

Comment 58

5 years ago
Created attachment 676209 [details] [diff] [review]
test-certoverrides v5

Addresses comments for 4.2. Does not change the certdb from 4.2 (does not update certs already in the cert db).
Attachment #676198 - Attachment is obsolete: true
Attachment #676209 - Flags: review?(honzab.moz)
(In reply to Camilo Viecco (:cviecco) from comment #56)
> Comment on attachment 670872 [details] [diff] [review]
> test-certoverrides v4.2
> 
> Review of attachment 670872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I disagree with the regeneration of both  'untrusted' and 'Unknown CA' certs
> as these
> where in the database before this change. 

Ah, I wasn't aware.  You had to enlighten me the last time already :)
Comment on attachment 676209 [details] [diff] [review]
test-certoverrides v5

Review of attachment 676209 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with few last little nits.

Thanks for all the updates and this pretty good test!

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +16,5 @@
> +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
> +const em = Ci.nsICertOverrideService.ERROR_MISMATCH;
> +const et = Ci.nsICertOverrideService.ERROR_TIME;
> +
> +//Note:  the host index matches the expected error.

Space between // and Note (it is coding convention to have a space after //.

@@ +106,5 @@
> +}
> +
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;

For me this var was more confusing, but up to you to leave or remove.

@@ +130,5 @@
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[j], overrideBits, successExpected, errorMustMatch);
> +    }
> +  }
> +  //Now we test the self-signed. Must return an overridable untrusted error.

Same here.
Attachment #676209 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 61

5 years ago
Created attachment 677044 [details] [diff] [review]
test-certoverrides v6

Fixed nits from v5

Thank you Honza and Brian. This now has r+ from both of you. I am now pushing it to try and once it passes I will mark it for chekin.
(Assignee)

Updated

5 years ago
Attachment #677044 - Flags: checkin?(honzab.moz)
Comment on attachment 677044 [details] [diff] [review]
test-certoverrides v6

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb35fde527d
Attachment #677044 - Flags: review+
Attachment #677044 - Flags: checkin?(honzab.moz)
Attachment #677044 - Flags: checkin+
Attachment #670872 - Attachment is obsolete: true
Attachment #670405 - Attachment is obsolete: true
Attachment #676209 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2fb35fde527d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Is there anything specific that we can do here to verify this? See comment 52.

Comment 65

5 years ago
Marking as [qa-] since there are still no answers for comment 52, and this bug fix does have an automated test verifying it.
Keywords: verifyme
Whiteboard: [qa-]

Updated

5 years ago
Depends on: 820757
You need to log in before you can comment on or make changes to this bug.