Closed
Bug 1002814
Opened 10 years ago
Closed 10 years ago
CreateEncodedOCSPRequest checks issuerCert for having an over-long serial number instead of cert
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(2 files, 4 obsolete files)
10.20 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
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"
Comment 1•10 years ago
|
||
(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.
Updated•10 years ago
|
Assignee: nobody → brian
Target Milestone: --- → mozilla32
Assignee | ||
Comment 2•10 years ago
|
||
I prototyped a quick gtest for this, if you want to go ahead and use it.
Comment 3•10 years ago
|
||
(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...
Assignee | ||
Comment 4•10 years ago
|
||
OK - sorry for not communicating earlier.
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
Example of using CreateEncodedCertificate to create test certs.
Assignee | ||
Comment 7•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Attachment #8416198 -
Flags: review?(brian)
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8416198 -
Attachment is obsolete: true
Attachment #8418175 -
Attachment is obsolete: true
Attachment #8419777 -
Flags: review?(brian)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
Had to back out because something's broken on OS X: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c88afa3e9d9
Looks like this intermittently fails on linux builds as well: https://tbpl.mozilla.org/php/getParsedLog.php?id=39673212&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=39679666&tree=Mozilla-Inbound
Assignee | ||
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
I filed bug 1012786. Carrying over r+. Thanks for the reviews! https://hg.mozilla.org/integration/mozilla-inbound/rev/f99ad5c1e65e https://hg.mozilla.org/integration/mozilla-inbound/rev/1370753a3c12
Attachment #8424145 -
Attachment is obsolete: true
Attachment #8424984 -
Flags: review+
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f99ad5c1e65e https://hg.mozilla.org/mozilla-central/rev/1370753a3c12
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•