Closed Bug 1002814 Opened 7 years ago Closed 7 years ago

CreateEncodedOCSPRequest checks issuerCert for having an over-long serial number instead of cert

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(2 files, 4 obsolete files)

Erwann Abalea pointed this out in the discussion on dev.tech.crypto:
(https://groups.google.com/forum/#!topic/mozilla.dev.tech.crypto/EbWse7Ryj8I - I can't figure out how to link directly to the post)
945   if (issuerCert->serialNumber.len > 127u - totalLenWithoutSerialNumberData) {
946     PR_SetError(SEC_ERROR_BAD_DATA, 0);
947     return nullptr;
948   }
949 
950   uint8_t totalLen = static_cast<uint8_t>(totalLenWithoutSerialNumberData +
951     cert->serialNumber.len);

"issuerCert" on line 945 should be "cert"
(In reply to David Keeler (:keeler) from comment #0)
> Erwann Abalea pointed this out in the discussion on dev.tech.crypto:
> (https://groups.google.com/forum/#!topic/mozilla.dev.tech.crypto/EbWse7Ryj8I
> - I can't figure out how to link directly to the post)

Direct link to post:
https://groups.google.com/d/msg/mozilla.dev.tech.crypto/EbWse7Ryj8I/uGO9UfOXSQYJ

I got it by clicking on "More message actions"/The triangle at the top right hand corner of the post, and selecting "Link" from the menu shown.
Assignee: nobody → brian
Target Milestone: --- → mozilla32
I prototyped a quick gtest for this, if you want to go ahead and use it.
(In reply to David Keeler (:keeler) from comment #2)
> I prototyped a quick gtest for this, if you want to go ahead and use it.

OK, or you can just take the bug. The reason I took it is because I had a plan for the test. But, if you already have the test...
Attached patch patch (obsolete) — Splinter Review
OK - sorry for not communicating earlier.
Assignee: brian → dkeeler
Status: NEW → ASSIGNED
Attachment #8416198 - Flags: review?(brian)
Your patch looks correct.

I don't think the way this test is written is good though. There's no way for somebody reading the test to understand what is being tested without doing a lot of work (copy/paste all the base64, de-base64, view the certs). I think that either using the stuff I wrote in bug 1005198 (preferred by me) or having the test read/write the certificates from easily-inspected files (like we do in other tests) is a much better pattern for us to establish. What do you think?
Flags: needinfo?(dkeeler)
Example of using CreateEncodedCertificate to create test certs.
Comment on attachment 8418175 [details] [diff] [review]
CreateEncodedCertificate-example.patch

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

::: security/pkix/test/lib/pkixtestutil.h
@@ +70,5 @@
>  //
>  // The return value, if non-null, is owned by the arena in the context and
>  // MUST NOT be freed.
>  SECItem* CreateEncodedCertificate(PLArenaPool* arena, long version,
>                                    SECOidTag signature, long serialNumber,

Ok - I can modify the test to use this. However, it looks like we'll have to extend how to specify the serial number so we can include a wider range of values. Any suggestions? My first instinct is to just take a SECItem* consisting of the bytes of the number. Or maybe it should be pre-DER-encoded?
Flags: needinfo?(dkeeler)
(In reply to David Keeler (:keeler) from comment #7)
> However, it looks like we'll have to
> extend how to specify the serial number so we can include a wider range of
> values. Any suggestions? My first instinct is to just take a SECItem*
> consisting of the bytes of the number. Or maybe it should be pre-DER-encoded?

I agree that that is a good idea. I suggest that it be the entire DER-encoded serial number.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8416198 - Attachment is obsolete: true
Attachment #8418175 - Attachment is obsolete: true
Attachment #8419777 - Flags: review?(brian)
Comment on attachment 8419777 [details] [diff] [review]
patch v2

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

It may also be useful to have a test that tests a serial number of length 20 (the maximum allowed by the spec).

::: security/pkix/test/gtest/pkix_ocsp_request_tests.cpp
@@ +12,5 @@
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */

Please update this with the boilerplate that Gerv wrote in bug 1007195.

@@ +36,5 @@
> +
> +class pkix_ocsp_request_tests : public ::testing::Test
> +{
> +protected:
> +  void SetUp()

1. SetUp() is called for each test case, so it should be initialing instance variables, not static variables. (Compare SetUp to SetUpTestCase to Environment::SetUp).

2. Things created in SetUp() should get destroyed in TearDown, or even better, they should be RAII using ScopedXXX.

@@ +39,5 @@
> +protected:
> +  void SetUp()
> +  {
> +    NSS_NoDB_Init(nullptr);
> +    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);

I suggest documenting this like like this:

static const uint8_t LEN = 128; // must be larger than 127
longSerialNumber = SECITEM_AllocItem(arena.get(), nulptr, 1 + 2 + LEN); // tag + length + value
...
longSerialNumber->data[1] = 0x80 + 1; // long length format (1 following byte)
longSerialNumber->data[2] = LEN;
longSerialNumber->data[3] = 0x01; // value 0x01000000...00

@@ +45,5 @@
> +    memset(longSerialNumber->data, 0, longSerialNumber->len);
> +    longSerialNumber->data[0] = der::INTEGER;
> +    longSerialNumber->data[1] = 0x81;
> +    longSerialNumber->data[2] = 0x80;
> +    longSerialNumber->data[3] = 0x01;

newline here, and ditto comments below.

@@ +58,5 @@
> +
> +  static const SECItem*
> +  ASCIIToDERName(const char* cn)
> +  {
> +    CERTName* certName = CERT_AsciiToName(cn);

Nit: ScopedPtr<CERTName, CERT_DestroyName>

@@ +69,5 @@
> +    return derName;
> +  }
> +
> +  static void MakeTwoCerts(const char* issuerCN, SECItem* issuerSerial,
> +                           ScopedCERTCertificate& issuer,

document /*out*/ for issuer and child.

@@ +91,5 @@
> +                        issuerKey.get(), SEC_OID_SHA256, childKey));
> +    ASSERT_TRUE(childDer);
> +    issuer = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), issuerCertDer,
> +                                     nullptr, false, true);
> +    ASSERT_TRUE(issuer.get());

ASSERT_TRUE(issuer) should also work due to overloaded operator bool.

@@ +94,5 @@
> +                                     nullptr, false, true);
> +    ASSERT_TRUE(issuer.get());
> +    child = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), childDer, nullptr,
> +                                    false, true);
> +    ASSERT_TRUE(child.get());

ditto.

@@ +99,5 @@
> +  }
> +
> +};
> +
> +// An issuer may have a long serial number in this situation.

Should this comment be "test that the large length of the issuer serial number doesn't cause CreateEncodedOCSPRequest to fail for the child certificate"?

@@ +117,5 @@
> +  SECItem* request = CreateEncodedOCSPRequest(arena.get(), child.get(),
> +                                              issuer.get());
> +  PRErrorCode error = PR_GetError();
> +  ASSERT_TRUE(request);
> +  ASSERT_EQ(error, 0);

ASSERT_TRUE(CreateEncodedOCSPRequest(...)) and ASSERT_EQ(0, PR_GetERror() is also OK (IMO better), because of the convenient way that GTest formats its output when the assertion fails.

Note that GTest's convention is that the expected value is the first parameter: i.e. ASSERT_EQ(0, error) not ASSERT_EQ(error, 0). In its output, GTest labels the first parameter the "expected" value and the second parameter the "actual" value.

@@ +138,5 @@
> +  SECItem* request = CreateEncodedOCSPRequest(arena.get(), child.get(),
> +                                              issuer.get());
> +  PRErrorCode error = PR_GetError();
> +  ASSERT_FALSE(request);
> +  ASSERT_EQ(error, SEC_ERROR_BAD_DATA);

Ditto the nits here.
Attachment #8419777 - Flags: review?(brian) → review+
Attached patch patch v2.1Splinter Review
Thanks, Brian. I addressed your comments and added a test for the longest serial number we're required to support.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc04fd0a41f
Attachment #8419777 - Attachment is obsolete: true
Attachment #8422556 - Flags: review+
What I'm seeing is that RSA_NewKey is occasionally failing with SEC_ERROR_NEED_RANDOM (which gets unhelpfully translated to CKR_FUNCTION_FAILED and then SEC_ERROR_PKCS11_FUNCTION_FAILED by the time PK11_GenerateKeyPair reports the error to GenerateKeyPair in pkixtestutil.cpp). My plan is to modify GenerateKeyPair to attempt to add some entropy and then retry when PK11_GenerateKeyPair fails.
How does this look?
Let me know if you want more background info or to handle this in a separate bug.
Attachment #8424145 - Flags: review?(brian)
Comment on attachment 8424145 [details] [diff] [review]
patch to retry PK11_GenerateKeyPair

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

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +601,5 @@
> +  // PK11_GenerateKeyPair can fail if there is insufficient entropy to
> +  // generate a random key. Attempting to add some entropy and retrying
> +  // appears to solve this issue.
> +  const uint32_t MAX_RETRIES = 3;
> +  for (uint32_t retries = 0; retries < MAX_RETRIES; retries++) {

IMO, there's no need for the MAX_RETRIES variable. I also suggest you might want to try more than 3 times (e.g. 10 or 100)?

@@ +622,5 @@
> +      return SECFailure;
> +    }
> +
> +    PRTime now = PR_Now();
> +    if (PK11_RandomUpdate(&now, sizeof(PRTime)) != SECSuccess) {

I have run into this before but I didn't have time to investigate it.

This is a bug in NSS that will affect WebCrypto and other things. Please add an XXX comment referencing the NSS bug.
Attachment #8424145 - Flags: review?(brian) → review+
https://hg.mozilla.org/mozilla-central/rev/f99ad5c1e65e
https://hg.mozilla.org/mozilla-central/rev/1370753a3c12
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.