1010594 - AIA OCSP responder URI is not parsed correctly (buffer overflow?)

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

For example, see this code in OCSPRequester.cpp:

  ScopedHTTPServerSession serverSession(
    reinterpret_cast<nsNSSHttpServerSession*>(serverSessionPtr));
  nsAutoCString path(url + pathPos, pathLen);

pathLen may be -1 to indicate that there is no path component of the URI.

This bug was found by trying to load https://bugs.webkit.org/ with security.OCSP.require=true. The AIA OCSP URI for the certificate is "http://ocsp.entrust.net" (no path component). We end up trying sending the POST to "http://ocsp.entrust.net//ocsp.entrust.net."

The documentation for nsIURLParser says:

     * Out parameters of the following methods are all optional (ie. the caller
     * may pass-in a NULL value if the corresponding results are not needed).
     * Signed out parameters may hold a value of -1 if the corresponding result
     * is not part of the string being parsed.

Thus, we need to be prepared to handle any of these being -1 (missing):

  int32_t schemeLen;
  int32_t authorityLen;
  int32_t pathLen;
  int32_t hostnameLen;
  int32_t port;

I set the rating to sec-critical to get your attention. Feel free to downgrade it upon more careful study.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, MSVC -Wall reports some warnings related to this code which should would also tipped us off about this.

Also note that there may be a different but related problem with the parsing of the port number (see the warning about the conversion of larger-sized int to uint16_t).

 0:18.65 Warning: C4365 in c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp: 'argument' : co
nversion from 'PRUint32' to 'int32_t', signed/unsigned mismatch
 0:18.65 c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp(54) : warning C4365: 'argument' :
conversion from 'PRUint32' to 'int32_t', signed/unsigned mismatch
 0:18.65 Warning: C4365 in c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp: 'argument' : co
nversion from 'int32_t' to 'nsACString_internal::size_type', signed/unsigned mismatch
 0:18.65 c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp(73) : warning C4365: 'argument' :
conversion from 'int32_t' to 'nsACString_internal::size_type', signed/unsigned mismatch
 0:18.65 Warning: C4242 in c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp: 'argument' : co
nversion from 'int32_t' to 'uint16_t', possible loss of data
 0:18.65 c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp(76) : warning C4242: 'argument' :
conversion from 'int32_t' to 'uint16_t', possible loss of data
 0:18.65 Warning: C4365 in c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp: 'argument' : co
nversion from 'int32_t' to 'uint16_t', signed/unsigned mismatch
 0:18.65 c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp(76) : warning C4365: 'argument' :
conversion from 'int32_t' to 'uint16_t', signed/unsigned mismatch
 0:18.65 Warning: C4365 in c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp: 'argument' : co
nversion from 'int32_t' to 'nsACString_internal::size_type', signed/unsigned mismatch
 0:18.65 c:\p\firefox\src\security\certverifier\OCSPRequestor.cpp(83) : warning C4365: 'argument' :
conversion from 'int32_t' to 'nsACString_internal::size_type', signed/unsigned mismatch

Comment 2

[Camilo Viecco (:cviecco)]

Hi brian, I discovered this also when trying to implement OCSP GET. The patch there will fix this
https://bugzilla.mozilla.org/attachment.cgi?id=8422830.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #2)
> Hi brian, I discovered this also when trying to implement OCSP GET. The
> patch there will fix this
> https://bugzilla.mozilla.org/attachment.cgi?id=8422830.

OK, great. I think the fix for it will need to be factored out into a separate patch because, IIRC, bug 982248 should get uplifted to Firefox 31 and thus this fix would need to be uplifted too.

Please take this bug and please add the tests for the parsing of the URL.

Comment 4

[Camilo Viecco (:cviecco)]

OK if I take this as suggested by briansmith?

Comment 5

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

Well, if you're not feeling spread too thin on other bugs, feel free.

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: check-ocsp-url

Just the patch for the issue, tests comming next. Do we only want to support http urls?

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: check-ocsp-url

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8423358 [details] [diff] [review]
check-ocsp-url

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

Please add tests. In particular test the "with path" and "without path" cases (i.e. this specific bug), as well as "with non-default port" and "without port".

Tests for malformed (no host, wrong scheme, empty value) should be easy since we don't even need a fake OCSP responder to test those (we just need to test that the correct error code is returned).

r+ with comments addressed and tests.

::: security/certverifier/OCSPRequestor.cpp
@@ +60,5 @@
> +    PR_SetError(SEC_ERROR_CERT_BAD_ACCESS_LOCATION, 0);
> +    return nullptr;
> +  }
> +  nsAutoCString scheme(url + schemePos, schemeLen);
> +  if (!scheme.LowerCaseEqualsLiteral("http")) {

This is a good addition. Please add a TODO(bug 92923) that we currently don't support https:// to void loops.

@@ +70,5 @@
>    int32_t hostnameLen;
>    int32_t port;
>    rv = urlParser->ParseAuthority(url + authorityPos, authorityLen,
>                                   nullptr, nullptr, nullptr, nullptr,
>                                   &hostnamePos, &hostnameLen, &port);

It looks like we've decided to skip any user@pass portion of the URL. Do you think we should reject URLs that have the uesr@pass portion?

Regardless of the answer, we should document our policy on user@pass in a comment here.

@@ +95,5 @@
>    ScopedHTTPServerSession serverSession(
>      reinterpret_cast<nsNSSHttpServerSession*>(serverSessionPtr));
> +  nsAutoCString path;
> +  if (pathLen > 0) {
> +    path.Assign(url + pathPos, pathLen);

1. Is it the case that url[pathPos] == '/'?
2. Do we need to normalize this? e.g. convert '\\' to '/'?

@@ +97,5 @@
> +  nsAutoCString path;
> +  if (pathLen > 0) {
> +    path.Assign(url + pathPos, pathLen);
> +  }
> +  else{

whitespace.

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: add-ocsp-tests

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: add-ocsp-tests (v1.1)

Comment 11

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

Comment on attachment 8424153 [details] [diff] [review]
add-ocsp-tests (v1.1)

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

Looks pretty good. There's a lot of copied code that can be refactored. Also, we don't need to keep around the database files after the certificates have been exported, right? Another concern I have is we have python utilities for generating certificates in CertUtils.py, but this generate.py doesn't make use of them.

::: security/manager/ssl/tests/unit/test_ocsp_url.js
@@ +2,5 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +"use strict";

Let's document what this test is supposed to exercise and how it's doing it.

@@ +8,5 @@
> +do_get_profile(); // must be called before getting nsIX509CertDB
> +const certdb = Cc["@mozilla.org/security/x509certdb;1"]
> +                 .getService(Ci.nsIX509CertDB);
> +
> +function load_ca(ca_name) {

This test only calls load_ca once - I'm not sure it's worth it to have a separate function to do this.

@@ +13,5 @@
> +  var ca_filename = ca_name + ".der";
> +  addCertFromFile(certdb, "test_ocsp_url/" + ca_filename, 'CTu,CTu,CTu');
> +}
> +
> +function cert_from_file(filename) {

Use constructCertFromFile from head_psm.js

@@ +20,5 @@
> +}
> +
> +const SERVER_PORT = 8888;
> +
> +function failingOCSPResponder() {

This is identical to what's in test_ev_certs.js, right? Let's factor out the common code to head_psm.js.

@@ +30,5 @@
> +  httpServer.start(SERVER_PORT);
> +  return httpServer;
> +}
> +
> +function start_ocsp_responder(expectedCertNames) {

This is also common to test_ev_certs.js, right? Again, factoring out common code is good.

@@ +63,5 @@
> +    }
> +  };
> +}
> +
> +function check_cert_err(cert_name, expected_error) {

Factor out common code.

@@ +75,5 @@
> +
> +function run_test() {
> +  addCertFromFile(certdb, "test_ocsp_url/int.der", ',,');
> +  load_ca("ca");
> +  Services.prefs.setBoolPref("security.OCSP.require", true);

Document why we need to set this pref (I'm assuming it's because soft fail won't report the misconfigured OCSP AIAs, right?)

@@ +77,5 @@
> +  addCertFromFile(certdb, "test_ocsp_url/int.der", ',,');
> +  load_ca("ca");
> +  Services.prefs.setBoolPref("security.OCSP.require", true);
> +
> +  // setup ocsp responder

This comment isn't quite accurate - setting this pref makes it so traffic to "www.example.com" goes to localhost, which is where the responder is.

@@ +79,5 @@
> +  Services.prefs.setBoolPref("security.OCSP.require", true);
> +
> +  // setup ocsp responder
> +  Services.prefs.setCharPref("network.dns.localDomains",
> +                             'www.example.com');

Let's just stick to double quotes instead of single quotes for long strings.

@@ +88,5 @@
> +}
> +
> +function add_tests_in_mode(useMozillaPKIX)
> +{
> +  add_test(function () {

nit: be consistent about "function()" vs. "function ()" (personally, I prefer the former)

@@ +124,5 @@
> +
> +  add_test(function() {
> +    clearOCSPCache();
> +    let ocspResponder = failingOCSPResponder();
> +    // XXX parser accepts :8888 as hostname

File a bug. Is this a problem with the library function or the OCSP requesting code?

::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
@@ +2,5 @@
> +
> +import tempfile, os, sys
> +import random
> +import pexpect
> +import subprocess

Doesn't look like this is used.

@@ +3,5 @@
> +import tempfile, os, sys
> +import random
> +import pexpect
> +import subprocess
> +import shutil

Doesn't look like this is used.

@@ +8,5 @@
> +import time
> +
> +libpath = os.path.abspath('../psm_common_py')
> +sys.path.append(libpath)
> +import CertUtils

This file doesn't seem to even use CertUtils - let's either remove the reference here or actually use it.

@@ +13,5 @@
> +
> +srcdir = os.getcwd()
> +db = tempfile.mkdtemp()
> +
> +def init_nss_db(db_dir):

It seems like other generate.py files do similar things - can we factor this code out?

@@ +18,5 @@
> +    nss_db_files = ["cert9.db", "key4.db", "pkcs11.txt"]
> +    for file in nss_db_files:
> +        if os.path.isfile(file):
> +            os.remove(file)
> +    #create noise file

nit: always have a space after a comment marker

@@ +29,5 @@
> +    pf = open(pwd_file, 'w')
> +    pf.write("\n")
> +    pf.close()
> +    #create nss db
> +    os.system("certutil -d sql:" + db_dir + " -N -f "+ pwd_file);

Do we need the certificate db after we've generated and exported the certificates? If so, we don't need to use the sql version. We should also be sure to remove those temporary files.

@@ +33,5 @@
> +    os.system("certutil -d sql:" + db_dir + " -N -f "+ pwd_file);
> +    return [noise_file, pwd_file]
> +
> +
> +def generate_ca_cert(db_dir, dest_dir, noise_file, name):

Again, it seems like this can be factored out.

@@ +36,5 @@
> +
> +def generate_ca_cert(db_dir, dest_dir, noise_file, name):
> +    out_name = dest_dir + "/" + name + ".der"
> +    base_exec_string = ("certutil -S -z " + noise_file + " -g 2048 -d sql:"
> +                        + db_dir +"/ -n " + name +" -v 120 -s 'CN=" + name

nit: spaces around operators

@@ +59,5 @@
> +    base_exec_string = ("certutil -S -z " + noise_file + " -g 2048 -d sql:"
> +                        + db_dir +"/ -n " + name +" -v 120 -m "
> +                        + str(random.randint(100,40000000)) +" -s 'CN=" + name
> +                        + ",O=PSM Testing,L=Mountain View,ST=California,C=US'"
> +                        + " -t ',,' -c "+ ca_nick + " -2 --extAIA")

I think if you use "-8" instead of "--extAIA", you can add the url in the command line instead of using the text interface (which gets rid of a lot of the child.expect/child.sendLine below)

@@ +92,5 @@
> +def generate_certs():
> +    [noise_file, pwd_file] = init_nss_db(srcdir)
> +    generate_ca_cert(srcdir, srcdir, noise_file, 'ca')
> +    generate_child_cert(srcdir, srcdir, noise_file, 'int', 'ca', False,
> +                        "http://www.example.com:8888/int")

Why do we need this intermediate? If it is necessary, does it need to have an OCSP AIA? It seems like that would unnecessarily complicate testing (if we decided to test with EV for example).

@@ +93,5 @@
> +    [noise_file, pwd_file] = init_nss_db(srcdir)
> +    generate_ca_cert(srcdir, srcdir, noise_file, 'ca')
> +    generate_child_cert(srcdir, srcdir, noise_file, 'int', 'ca', False,
> +                        "http://www.example.com:8888/int")
> +    nick_baseurl = { 'no-path-url':'http://www.example.com:8888',

nit: space after ":"

@@ +97,5 @@
> +    nick_baseurl = { 'no-path-url':'http://www.example.com:8888',
> +                     'ftp-url':"ftp://www.example.com:8888/",
> +                     'no-scheme-url': "www.example.com:8888/",
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/"}

We should also test with an https:// url, right?
Also, how about things like "://www.example.com", "//www.example.com", "/www.example.com", "http://www.example.com:/", "ttp://www.example.com", "http://www.example.com:-1", "/", etc.

@@ +100,5 @@
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/"}
> +    for nick, url in nick_baseurl.iteritems():
> +        if (nick != "no-path-url"):
> +            url = url + nick

How about, instead of defaulting to appending the nickname to the url, we just have nick_baseurl be a list of (nickname, complete OCSP AIA url) pairs? I would find it less confusing to read.

::: security/manager/ssl/tests/unit/test_ocsp_url/pkcs11.txt
@@ +1,1 @@
> +library=

We don't need to keep this or the other db files around, right?

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +77,5 @@
>  # Bug 1009158: this test times out on Android
>  skip-if = os == "android"
> +[test_ocsp_url.js]
> +# Bug 1009158: this test times out on Android
> +fail-if = os == "android"

fail or skip? The others time out the entire test run, so they can't be run at all.

@@ +78,5 @@
>  skip-if = os == "android"
> +[test_ocsp_url.js]
> +# Bug 1009158: this test times out on Android
> +fail-if = os == "android"
> +

nit: probably don't need this trailing blank line

Comment 12

[Camilo Viecco (:cviecco)]

Comment on attachment 8424153 [details] [diff] [review]
add-ocsp-tests (v1.1)

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

Thanks for the review. Can you reply here to the potential breakage of all generators on windows if I factor out some of generate.py? Alo on the remove or not the intermediate.

::: security/manager/ssl/tests/unit/test_ocsp_url.js
@@ +20,5 @@
> +}
> +
> +const SERVER_PORT = 8888;
> +
> +function failingOCSPResponder() {

almost identical. but yes I can make somthing similar.

@@ +30,5 @@
> +  httpServer.start(SERVER_PORT);
> +  return httpServer;
> +}
> +
> +function start_ocsp_responder(expectedCertNames) {

Again is close but not the same. But yes I can create a generic function and wrappers in each function so that we dont have to do extra arguments.

::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
@@ +29,5 @@
> +    pf = open(pwd_file, 'w')
> +    pf.write("\n")
> +    pf.close()
> +    #create nss db
> +    os.system("certutil -d sql:" + db_dir + " -N -f "+ pwd_file);

Yes, we need it so that the ocspresponder can generate a valid response( And openssl is not capable of generating ocsp urls that do not end in '/').

@@ +33,5 @@
> +    os.system("certutil -d sql:" + db_dir + " -N -f "+ pwd_file);
> +    return [noise_file, pwd_file]
> +
> +
> +def generate_ca_cert(db_dir, dest_dir, noise_file, name):

Mostly, I will create a generic function and then wrappers so that local callers no not have to care about the parameters they dont need (for example certificate version)

@@ +59,5 @@
> +    base_exec_string = ("certutil -S -z " + noise_file + " -g 2048 -d sql:"
> +                        + db_dir +"/ -n " + name +" -v 120 -m "
> +                        + str(random.randint(100,40000000)) +" -s 'CN=" + name
> +                        + ",O=PSM Testing,L=Mountain View,ST=California,C=US'"
> +                        + " -t ',,' -c "+ ca_nick + " -2 --extAIA")

option 8 is for the DNS subject alt name extension :(
Also I cannot factor this out to CertUtils unless you are willing to accept that none of the generators that depend on certutils will be able to run in windows. pexpect does not come with windows' python by default. Bsmith was very emphatic that we should limit the number of scripts that cannot run on windows. Are you OK with breaking all generators on windows by default?

@@ +92,5 @@
> +def generate_certs():
> +    [noise_file, pwd_file] = init_nss_db(srcdir)
> +    generate_ca_cert(srcdir, srcdir, noise_file, 'ca')
> +    generate_child_cert(srcdir, srcdir, noise_file, 'int', 'ca', False,
> +                        "http://www.example.com:8888/int")

I am confused here
what I agree: the intermediate is not technically necessary if we stick to DV validation.
If we go for EV, EVs are required to have intermediates (we are not enforcing that right now).

I prefer to have an intermediate (as it is closer to real world deployments), but if you want it gone please state so.

@@ +97,5 @@
> +    nick_baseurl = { 'no-path-url':'http://www.example.com:8888',
> +                     'ftp-url':"ftp://www.example.com:8888/",
> +                     'no-scheme-url': "www.example.com:8888/",
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/"}

sure I will add those and some other.

@@ +100,5 @@
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/"}
> +    for nick, url in nick_baseurl.iteritems():
> +        if (nick != "no-path-url"):
> +            url = url + nick

OK.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +77,5 @@
>  # Bug 1009158: this test times out on Android
>  skip-if = os == "android"
> +[test_ocsp_url.js]
> +# Bug 1009158: this test times out on Android
> +fail-if = os == "android"

Interesting, text_ev_certs uses fail-if (was my model). I will change to skip.

Comment 13

[Camilo Viecco (:cviecco)]

Can you reply yo comment 12?

Comment 14

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

(In reply to Camilo Viecco (:cviecco) from comment #12)
> Comment on attachment 8424153 [details] [diff] [review]
> add-ocsp-tests (v1.1)
> 
> Review of attachment 8424153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the review. Can you reply here to the potential breakage of all
> generators on windows if I factor out some of generate.py? Alo on the remove
> or not the intermediate.
> 
> ::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
> @@ +29,5 @@
> > +    pf = open(pwd_file, 'w')
> > +    pf.write("\n")
> > +    pf.close()
> > +    #create nss db
> > +    os.system("certutil -d sql:" + db_dir + " -N -f "+ pwd_file);
> 
> Yes, we need it so that the ocspresponder can generate a valid response( And
> openssl is not capable of generating ocsp urls that do not end in '/').

Ok - I kept forgetting that this test needed to generate successful responses.
(Although, actually, there would be a way to get around this: have the responder throw a 500 Server Error or something, and the test would expect a SEC_ERROR_OCSP_SERVER_ERROR in those cases. But it's probably not necessary to be that clever just to save space by not keeping these files around.)

> @@ +59,5 @@
> > +    base_exec_string = ("certutil -S -z " + noise_file + " -g 2048 -d sql:"
> > +                        + db_dir +"/ -n " + name +" -v 120 -m "
> > +                        + str(random.randint(100,40000000)) +" -s 'CN=" + name
> > +                        + ",O=PSM Testing,L=Mountain View,ST=California,C=US'"
> > +                        + " -t ',,' -c "+ ca_nick + " -2 --extAIA")
> 
> option 8 is for the DNS subject alt name extension :(
> Also I cannot factor this out to CertUtils unless you are willing to accept
> that none of the generators that depend on certutils will be able to run in
> windows. pexpect does not come with windows' python by default. Bsmith was
> very emphatic that we should limit the number of scripts that cannot run on
> windows. Are you OK with breaking all generators on windows by default?

Well, either way we're getting into some technical debt here. At some point we should make sure all of the generators work on first-tier platforms and that we don't have unnecessary copies of code. I think this is fine for now.

> @@ +92,5 @@
> > +def generate_certs():
> > +    [noise_file, pwd_file] = init_nss_db(srcdir)
> > +    generate_ca_cert(srcdir, srcdir, noise_file, 'ca')
> > +    generate_child_cert(srcdir, srcdir, noise_file, 'int', 'ca', False,
> > +                        "http://www.example.com:8888/int")
> 
> I am confused here
> what I agree: the intermediate is not technically necessary if we stick to
> DV validation.
> If we go for EV, EVs are required to have intermediates (we are not
> enforcing that right now).
> 
> I prefer to have an intermediate (as it is closer to real world
> deployments), but if you want it gone please state so.

Let's have an intermediate without an OCSP AIA.

> ::: security/manager/ssl/tests/unit/xpcshell.ini
> @@ +77,5 @@
> >  # Bug 1009158: this test times out on Android
> >  skip-if = os == "android"
> > +[test_ocsp_url.js]
> > +# Bug 1009158: this test times out on Android
> > +fail-if = os == "android"
> 
> Interesting, text_ev_certs uses fail-if (was my model). I will change to
> skip.

Well, I guess it would be good to try it out either way.

Comment 15

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

Camilo - looks like bug 982248 (i.e. the code that introduced this bug) just landed on aurora. We should land this and uplift as soon as possible.

Comment 16

[Camilo Viecco (:cviecco)]

Attachment: add-ocsp-tests (v2)

Comment 17

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

Comment on attachment 8425768 [details] [diff] [review]
add-ocsp-tests (v2)

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

The refactoring made everything a lot cleaner, I think. I still have some concerns - see comments.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +409,5 @@
>    }
>    return retArray;
>  }
> +
> +// Returns an http responder that will issue an alert for any connection

nit: how about "Starts and returns an http responder that will cause a test failure if it is queried"
Otherwise good documentation.

@@ +418,5 @@
> +  httpServer.registerPrefixHandler("/", function(request, response) {
> +    do_check_true(false);
> +  });
> +  httpServer.identity.setPrimary("http", serverIdentities.shift(), serverPort);
> +  for (let i = 0; i< serverIdentities.length; i++) {

nit: use serverIdentities.forEach

@@ +425,5 @@
> +  httpServer.start(serverPort);
> +  return httpServer;
> +}
> +
> +// returns a http OCSP responder that serves good OCSP responses to

Hmmm - but it doesn't, really, does it? It returns an object with a method "stop" (which is fine - it should just be documented).

@@ +429,5 @@
> +// returns a http OCSP responder that serves good OCSP responses to
> +// the nicknames indicated by expectedCertNames. Requires an NSS database
> +// that has the cert, the issuer and the issuer's private key.
> +function startOCSPResponder(serverPort, identity, invalidIdentities,
> +                            nssDBLocation, expectedCertNames, matchNames) {

A bit more documentation about what each of these arguments are, please. For one thing, it seems like expectedCertNames and matchNames do the same thing - why are both necessary?

@@ +432,5 @@
> +function startOCSPResponder(serverPort, identity, invalidIdentities,
> +                            nssDBLocation, expectedCertNames, matchNames) {
> +  let httpServer = new HttpServer();
> +  httpServer.registerPrefixHandler("/",
> +      function handleServerCallback(aRequest, aResponse) {

nit: this whole block only needs to be indented two additional spaces from the previous scope, rather than 4

@@ +433,5 @@
> +                            nssDBLocation, expectedCertNames, matchNames) {
> +  let httpServer = new HttpServer();
> +  httpServer.registerPrefixHandler("/",
> +      function handleServerCallback(aRequest, aResponse) {
> +        for (let i = 0; i< invalidIdentities.length; i++) {

nit: use !invalidIdentities.some(...)

@@ +436,5 @@
> +      function handleServerCallback(aRequest, aResponse) {
> +        for (let i = 0; i< invalidIdentities.length; i++) {
> +          do_check_neq(aRequest.host, invalidIdentities[i]);
> +        }
> +        let cert_nick = aRequest.path.slice(1, aRequest.path.length - 1);

nit: consistent naming style, please (I know xpcshell uses underscores, but the rest of the test code here doesn't)

@@ +444,5 @@
> +        if (matchNames.length < 1) {
> +          do_check_eq(cert_nick, expected_nick);
> +        } else {
> +          let test_name = matchNames.shift();
> +          do_check_eq(cert_nick, test_name);

This whole little bit is confusing - what's going on here?

@@ +456,5 @@
> +        let responseBody = retArray[0];
> +        aResponse.bodyOutputStream.write(responseBody, responseBody.length);
> +      });
> +  httpServer.identity.setPrimary("http", identity, serverPort);
> +  for (let i = 0; i< invalidIdentities.length; i++) {

use invalidIdentities.forEach

@@ +470,5 @@
> +}
> +
> +
> +
> +

4 blank lines really aren't necessary here.

::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py
@@ +204,5 @@
> +    pf = open(pwd_file, 'w')
> +    pf.write("\n")
> +    pf.close()
> +    # create nss db
> +    os.system("certutil -d sql:" + db_dir + " -N -f "+ pwd_file);

nit: " +

@@ +263,5 @@
> +      do_bc      -- if the certificate should include the basic constraints
> +                    (valid ca's should be true)
> +      is_ee      -- is this and End Entity cert? false means intermediate
> +      ocsp_url   -- optional location of the ocsp responder for this certificate
> +                    this is included only of do_bc is set to True

"only if do_bc..."

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +44,3 @@
>  }
>  
> +

nit: unnecessary blank line

::: security/manager/ssl/tests/unit/test_ocsp_url.js
@@ +37,5 @@
> +  // Enabled so that we can force ocsp failure responses.
> +  Services.prefs.setBoolPref("security.OCSP.require", true);
> +
> +  Services.prefs.setCharPref("network.dns.localDomains",
> +                             'www.example.com');

nit: use double-quotes

@@ +88,5 @@
> +                                  : SEC_ERROR_OCSP_MALFORMED_REQUEST);
> +    ocspResponder.stop(run_next_test);
> +  });
> +
> +  add_test(function () {

Again, be consistent about "function()" vs. "function ()" (personally, I prefer the former).

@@ +90,5 @@
> +  });
> +
> +  add_test(function () {
> +    clearOCSPCache();
> +    let ocspResponder = start_ocsp_responder(["hTTp-url"], ["hTTp-ur"]);

Why "hTTp-ur"? Where does this come from?

@@ +107,5 @@
> +
> +  add_test(function() {
> +    clearOCSPCache();
> +    let ocspResponder = failingOCSPResponder();
> +    // XXX parser accepts :8888 as hostname

Again, is this a library bug or a bug in DoOCSPRequest? In either case, did you file a bug? What is the bug number?

::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
@@ +2,5 @@
> +
> +import tempfile, os, sys
> +import random
> +import pexpect
> +import time

nit: random, pexpect, and time all aren't used anymore

@@ +12,5 @@
> +srcdir = os.getcwd()
> +db = tempfile.mkdtemp()
> +
> +def generate_ca_cert(db_dir, dest_dir, noise_file, name):
> +    return  CertUtils.generate_ca_cert(db_dir, dest_dir, noise_file, name,

nit: two spaces after return

@@ +29,5 @@
> +                     'ftp-url': "ftp://www.example.com:8888/",
> +                     'no-scheme-url': "www.example.com:8888/",
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/",
> +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",

Why "hTTp"?

@@ +31,5 @@
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/",
> +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",
> +                     'https-url': "https://www.example.com:8888/https-url",
> +                     'empty-scheme': "://www.example.com",

What's the difference between this one and empty-scheme-url (other than the port and trailing slash - as in, why is it important that we have both of these)?

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +76,5 @@
>  [test_pinning.js]
>  run-sequentially = hardcoded ports
>  # Bug 1009158: this test times out on Android
>  skip-if = os == "android"
> +[test_ocsp_url.js]

I would call this "test_ocsp_aias.js" or "test_ocsp_aia_urls.js", but it's not a big deal

Comment 18

[Camilo Viecco (:cviecco)]

Comment on attachment 8425768 [details] [diff] [review]
add-ocsp-tests (v2)

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

Thanks for the fast review. Will address your issues.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +429,5 @@
> +// returns a http OCSP responder that serves good OCSP responses to
> +// the nicknames indicated by expectedCertNames. Requires an NSS database
> +// that has the cert, the issuer and the issuer's private key.
> +function startOCSPResponder(serverPort, identity, invalidIdentities,
> +                            nssDBLocation, expectedCertNames, matchNames) {

They do not (the confing part you noticed),expectedCertNames has the nicknames of the ocsp responses to match. ExpectedCertNames contains the base url paths to match (this is what triggered this bug, the url reported did not matched the expected url).
I think a rename would be useful

::: security/manager/ssl/tests/unit/test_ocsp_url.js
@@ +88,5 @@
> +                                  : SEC_ERROR_OCSP_MALFORMED_REQUEST);
> +    ocspResponder.stop(run_next_test);
> +  });
> +
> +  add_test(function () {

I could have sworn that my vi now included checking for this. Sorry.

@@ +90,5 @@
> +  });
> +
> +  add_test(function () {
> +    clearOCSPCache();
> +    let ocspResponder = start_ocsp_responder(["hTTp-url"], ["hTTp-ur"]);

There are some issues in the matcher part of the ocsp responder part. But since it only affected this I preaturally did not fixed in the right place. Will fix in the right place

@@ +107,5 @@
> +
> +  add_test(function() {
> +    clearOCSPCache();
> +    let ocspResponder = failingOCSPResponder();
> +    // XXX parser accepts :8888 as hostname

Just did bug 1013615

::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
@@ +29,5 @@
> +                     'ftp-url': "ftp://www.example.com:8888/",
> +                     'no-scheme-url': "www.example.com:8888/",
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/",
> +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",

schemes are case insensitive. we should not reject this one.

@@ +31,5 @@
> +                     'empty-scheme-url': "://www.example.com:8888/",
> +                     'no-host-url': "http://:8888/",
> +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",
> +                     'https-url': "https://www.example.com:8888/https-url",
> +                     'empty-scheme': "://www.example.com",

Not really, this one was suggested by you on the previous comment. I would delete it too.

Comment 19

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

(In reply to Camilo Viecco (:cviecco) from comment #18)
> ::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
> @@ +29,5 @@
> > +                     'ftp-url': "ftp://www.example.com:8888/",
> > +                     'no-scheme-url': "www.example.com:8888/",
> > +                     'empty-scheme-url': "://www.example.com:8888/",
> > +                     'no-host-url': "http://:8888/",
> > +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",
> 
> schemes are case insensitive. we should not reject this one.

... but the scheme is lowercase "http" here, unless I'm misunderstanding?

> @@ +31,5 @@
> > +                     'empty-scheme-url': "://www.example.com:8888/",
> > +                     'no-host-url': "http://:8888/",
> > +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",
> > +                     'https-url': "https://www.example.com:8888/https-url",
> > +                     'empty-scheme': "://www.example.com",
> 
> Not really, this one was suggested by you on the previous comment. I would
> delete it too.

Heh - my bad.

Comment 20

[Camilo Viecco (:cviecco)]

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #19)
> (In reply to Camilo Viecco (:cviecco) from comment #18)
> > ::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
> > @@ +29,5 @@
> > > +                     'ftp-url': "ftp://www.example.com:8888/",
> > > +                     'no-scheme-url': "www.example.com:8888/",
> > > +                     'empty-scheme-url': "://www.example.com:8888/",
> > > +                     'no-host-url': "http://:8888/",
> > > +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",
> > 
> > schemes are case insensitive. we should not reject this one.
> 
> ... but the scheme is lowercase "http" here, unless I'm misunderstanding?

 I messed up and wrote a wrong url
> 
> > @@ +31,5 @@
> > > +                     'empty-scheme-url': "://www.example.com:8888/",
> > > +                     'no-host-url': "http://:8888/",
> > > +                     'hTTp-url': "http://www.example.com:8888/hTTp-url",
> > > +                     'https-url': "https://www.example.com:8888/https-url",
> > > +                     'empty-scheme': "://www.example.com",
> > 
> > Not really, this one was suggested by you on the previous comment. I would
> > delete it too.
> 
> Heh - my bad.

Comment 21

[Camilo Viecco (:cviecco)]

Attachment: add-ocsp-tests (v3)

Comment 22

[Camilo Viecco (:cviecco)]

Attachment: add-ocsp-tests (v3.1)

Comment 23

[Camilo Viecco (:cviecco)]

Attachment: add-ocsp-tests (v3.2)

Comment 24

[Camilo Viecco (:cviecco)]

Comment on attachment 8426401 [details] [diff] [review]
add-ocsp-tests (v3.2)

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

The only thing that I knowingly did different is that I used forEach instead of some for the iteration over the invalid identities.

Comment 25

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

Comment on attachment 8426401 [details] [diff] [review]
add-ocsp-tests (v3.2)

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

Great!

::: security/manager/ssl/tests/unit/head_psm.js
@@ +410,5 @@
>    return retArray;
>  }
> +
> +// Starts and returns an http responder that will cause a test failure if it is
> +// queried.  The server identities are given by a non-empty array

nit: unnecessary double-space after "."

@@ +425,5 @@
> +  httpServer.start(serverPort);
> +  return httpServer;
> +}
> +
> +// Starts a http OCSP responder that serves good OCSP responses and

nit: "an http"

@@ +429,5 @@
> +// Starts a http OCSP responder that serves good OCSP responses and
> +// returns an object with a method stop that should be called to stop
> +// the http server.
> +//
> +// serverPort is the port if the http OCSP responder

nit: "port of the"

@@ +431,5 @@
> +// the http server.
> +//
> +// serverPort is the port if the http OCSP responder
> +// identity is the http hostname that will answer the OCSP requests
> +// invalidIdentities is an array of identities that if used an

nit: s/used an/used/
Specifying both valid and invalid identities seems unnecessary - the server shouldn't respond to identities it isn't registered to (oh, I see - we do register them. This seems weird.). I think this is cruft from another test, so please file a follow-up bug to figure out the best thing to do here (the resolution may be to document this better).

@@ +433,5 @@
> +// serverPort is the port if the http OCSP responder
> +// identity is the http hostname that will answer the OCSP requests
> +// invalidIdentities is an array of identities that if used an
> +//   will cause a test failure
> +// nssDBlocaion is the location of the NSS where from where the OCSP

nit: "NSS database from where"

@@ +434,5 @@
> +// identity is the http hostname that will answer the OCSP requests
> +// invalidIdentities is an array of identities that if used an
> +//   will cause a test failure
> +// nssDBlocaion is the location of the NSS where from where the OCSP
> +//   responses will be generates (assumes appropiate keys are present)

nit: "generated"

::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py
@@ +206,5 @@
> +    pf.close()
> +    # create nss db
> +    os.system("certutil -d sql:" + db_dir + " -N -f " + pwd_file);
> +    return [noise_file, pwd_file]
> +

nit: unnecessary blank line

@@ +244,5 @@
> +        os.system(base_exec_string)
> +    os.system("certutil -d sql:" + db_dir + "/ -L -n " + name + " -r > " +
> +              out_name)
> +    return out_name
> +

nit: unnecessary blank line

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +5,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import tempfile, os, sys, pexpect

nit: pexpect isn't used here anymore

@@ +6,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import tempfile, os, sys, pexpect
>  import random

same with random

@@ +7,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import tempfile, os, sys, pexpect
>  import random
>  import time

same with time

::: security/manager/ssl/tests/unit/test_ocsp_url.js
@@ +18,5 @@
> +function failingOCSPResponder() {
> +  return getFailingHttpServer(SERVER_PORT, ["www.example.com"]);
> +}
> +
> +function start_ocsp_responder(expectedCertNames, doNickMatching) {

nit: "doNickMatching" isn't descriptive - maybe "expectedPaths"?

::: security/manager/ssl/tests/unit/test_ocsp_url/generate.py
@@ +32,5 @@
> +                     'bad-scheme': "/www.example.com",
> +                     'empty-port': "http://www.example.com:/",
> +                     'unknown-scheme': "ttp://www.example.com",
> +                     'negative-port': "http://www.example.com:-1",
> +                     'no-scheme-host-port': "/"}

nit: add a space before the final "}"

Comment 26

[Camilo Viecco (:cviecco)]

Comment on attachment 8423358 [details] [diff] [review]
check-ocsp-url

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

Thanks for the review. Agreed to all your suggestions

::: security/certverifier/OCSPRequestor.cpp
@@ +95,5 @@
>    ScopedHTTPServerSession serverSession(
>      reinterpret_cast<nsNSSHttpServerSession*>(serverSessionPtr));
> +  nsAutoCString path;
> +  if (pathLen > 0) {
> +    path.Assign(url + pathPos, pathLen);

For 1 yes for pathlen > 0
For 2. We do not need to normalize.

Comment 27

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=914dd94af4b

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8a853d1a4dc
https://hg.mozilla.org/mozilla-central/rev/863a0fc463fa

Comment 29

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

Comment on attachment 8423358 [details] [diff] [review]
check-ocsp-url

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 982248
User impact if declined: potential out of bounds reads, crashes, etc.
Testing completed (on m-c, etc.): landed on m-c, has tests (separate patch)
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db89c11281ba
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd4377f6b508

BTW, this shouldn't have landed without sec-approval since it's a sec-crit that affects more than trunk.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 31

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

Well, what happened was I asked for uplift approval for bug 982248, which is what introduced the bug. I shouldn't have started that process until this was finished.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7fdfab684f17

https://tbpl.mozilla.org/php/getParsedLog.php?id=40280998&tree=Mozilla-Aurora

I think at this point, I'm just going to stay away from these OCSP uplifts going forward. You can do the uplifts when you get whatever approvals you need and local testing looks good.

Comment 33

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

Sorry, Ryan - this has definitely been more trouble than necessary :(
Camilo - I think we need to prepare a version of this patch that uses the non-sql db:

08:48:27     INFO -  Failed to initialize NSS: SEC_ERROR_LEGACY_DATABASE

(I guess the other option is to uplift the sql-ifying patch to aurora.)

Comment 34

[Camilo Viecco (:cviecco)]

I will write a new non-sql test patch. It should not be that hard (TM).

Comment 35

[Camilo Viecco (:cviecco)]

 https://tbpl.mozilla.org/?tree=Try&rev=4a6488bedd0e

Comment 37

[Ryan VanderMeulen [:RyanVM]]

This was pushed to Aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/096aa8386dd5
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1c175cb55c6

Comment 38

[Matt Wobensmith [:mwobensmith][:matt:]]

I can repro this issue on 2014-05-14 Fx32. However, it does not reproduce on Fx31 from the same date.

I have verified this to be fixed on Fx32, latest build.

If anyone disagrees about the observed behavior in Fx31, please advise. Thanks.