More cleaning

crypto/pkcs7/bio/cipher.c

@@ -192,9 +192,9 @@ static int enc_write(BIO *b, const char *in, int inl) {
 
 static long enc_ctrl(BIO *b, int cmd, long num, void *ptr) {
   GUARD_PTR(b);
-  EVP_CIPHER_CTX **cipher_ctx;
   long ret = 1;
   BIO_ENC_CTX *ctx = BIO_get_data(b);
+  EVP_CIPHER_CTX **cipher_ctx;
   BIO *next = BIO_next(b);
   if (ctx == NULL) {
     return 0;
@@ -238,6 +238,9 @@ static long enc_ctrl(BIO *b, int cmd, long num, void *ptr) {
       ret = BIO_ctrl(next, cmd, num, ptr);
       BIO_copy_next_retry(b);
       break;
+    case BIO_C_GET_CIPHER_STATUS:
+      ret = (long)ctx->ok;
+      break;
     case BIO_C_GET_CIPHER_CTX:
       cipher_ctx = (EVP_CIPHER_CTX **)ptr;
       if (!cipher_ctx) {
@@ -247,9 +250,6 @@ static long enc_ctrl(BIO *b, int cmd, long num, void *ptr) {
       *cipher_ctx = ctx->cipher;
       BIO_set_init(b, 1);
       break;
-    case BIO_C_GET_CIPHER_STATUS:
-      ret = (long)ctx->ok;
-      break;
     // OpenSSL implements these, but because we don't need them and cipher BIO
     // is internal, we can fail loudly if they're called. If this case is hit,
     // it likely means you're making a change that will require implementing

crypto/pkcs7/internal.h

@@ -134,7 +134,9 @@ OPENSSL_EXPORT int BIO_set_cipher(BIO *b, const EVP_CIPHER *cipher,
 // abnormalities. Data read from an unhealthy cipher should not be considered
 // authentic.
 OPENSSL_EXPORT int BIO_get_cipher_status(BIO *b);
-// TODO [childw]
+
+// pkcs7_final initializes a data BIO using |p7|, copies all of |data| into it,
+// before final finalizing |p7|. It returns 1 on success and 0 on failure.
 OPENSSL_EXPORT int pkcs7_final(PKCS7 *p7, BIO *data);
 
 #if defined(__cplusplus)

crypto/pkcs7/pkcs7.c

@@ -1092,7 +1092,7 @@ int pkcs7_final(PKCS7 *p7, BIO *data) {
   if ((p7bio = PKCS7_dataInit(p7, NULL)) == NULL) {
     OPENSSL_END_ALLOW_DEPRECATED
     OPENSSL_PUT_ERROR(PKCS7, ERR_R_PKCS7_LIB);
-    return 0;
+    goto err;
   }
 
   if (!pkcs7_bio_copy_content(data, p7bio)) {
@@ -1223,12 +1223,6 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) {
 
   switch (OBJ_obj2nid(p7->type)) {
     case NID_pkcs7_signed:
-      /*
-       * p7->d.sign->contents is a PKCS7 structure consisting of a contentType
-       * field and optional content.
-       * data_body is NULL if that structure has no (=detached) content
-       * or if the contentType is wrong (i.e., not "data").
-       */
       data_body = PKCS7_get_octet_string(p7->d.sign->contents);
       if (!PKCS7_is_detached(p7) && data_body == NULL) {
         OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_INVALID_SIGNED_DATA_TYPE);

crypto/pkcs7/pkcs7_asn1.c

@@ -176,6 +176,7 @@ ASN1_SEQUENCE(PKCS7_DIGEST) = {
     ASN1_SIMPLE(PKCS7_DIGEST, contents, PKCS7),
     ASN1_SIMPLE(PKCS7_DIGEST, digest,
                 ASN1_OCTET_STRING)} ASN1_SEQUENCE_END(PKCS7_DIGEST)
+
 IMPLEMENT_ASN1_FUNCTIONS(PKCS7_DIGEST)
 
 ASN1_SEQUENCE(PKCS7_ENVELOPE) = {

crypto/pkcs7/pkcs7_test.cc

@@ -1319,22 +1319,6 @@ hJTbHtjEDJ7BHLC/CNUhXbpyyu1y
   EXPECT_EQ(Bytes(pkcs7_bytes, pkcs7_len),
             Bytes(kExpectedOutput, sizeof(kExpectedOutput)));
 
-  // TODO [childw] modify this or drop test case?
-  // Other option combinations should fail.
-  // EXPECT_FALSE(PKCS7_sign(cert.get(), key.get(), /*certs=*/nullptr,
-  //                         // data_bio.get(),
-  //                         // PKCS7_NOATTR | PKCS7_BINARY | PKCS7_NOCERTS));
-  // EXPECT_FALSE(PKCS7_sign(cert.get(), key.get(), /*certs=*/nullptr,
-  //                         data_bio.get(),
-  //                         PKCS7_BINARY | PKCS7_NOCERTS | PKCS7_DETACHED));
-  // EXPECT_FALSE(
-  //     PKCS7_sign(cert.get(), key.get(), /*certs=*/nullptr, data_bio.get(),
-  //                PKCS7_NOATTR | PKCS7_TEXT | PKCS7_NOCERTS |
-  //                PKCS7_DETACHED));
-  // EXPECT_FALSE(PKCS7_sign(cert.get(), key.get(), /*certs=*/nullptr,
-  //                         data_bio.get(),
-  //                         PKCS7_NOATTR | PKCS7_BINARY | PKCS7_DETACHED));
-
   ERR_clear_error();
 }
 
@@ -1689,6 +1673,7 @@ TEST(PKCS7Test, DataInitFinal) {
   EXPECT_FALSE(PKCS7_dataFinal(p7.get(), bio.get()));
 }
 
+// TODO [childw] delete this, it's covered in ruby's tests.
 TEST(PKCS7Test, RubyRepro) {
   bssl::UniquePtr<PKCS7> p7;
   const uint8_t kRubyTestEnveloped[] = {

crypto/pkcs7/pkcs7_x509.c

@@ -233,19 +233,15 @@ int PKCS7_bundle_CRLs(CBB *out, const STACK_OF(X509_CRL) *crls) {
 }
 
 PKCS7 *d2i_PKCS7_bio(BIO *bio, PKCS7 **out) {
-  if (bio == NULL) {
-    OPENSSL_PUT_ERROR(PKCS7, ERR_R_PASSED_NULL_PARAMETER);
-    return NULL;
-  }
-
+  GUARD_PTR(bio);
   uint8_t *data;
   size_t len;
-  // Historically, this function did not impose a limit in OpenSSL and is used
-  // to read CRLs, so we leave this without an external bound.
+  // Read BIO contents into newly allocated buffer
   if (!BIO_read_asn1(bio, &data, &len, INT_MAX)) {
     return NULL;
   }
   const uint8_t *ptr = data;
+  // d2i_PKCS7 handles indefinite-length BER properly, so use it
   void *ret = d2i_PKCS7(out, &ptr, len);
   OPENSSL_free(data);
   return ret;

crypto/test/test_util.h

@@ -85,7 +85,9 @@ bssl::UniquePtr<RSA> RSAFromPEM(const char *pem);
 
 static const int64_t kReferenceTime = 1474934400 /* Sep 27th, 2016 */;
 
-// TODO [childw]
+// MakeTestCert creates an X509 certificate for use in testing. It is configured
+// to be valid from 1 day prior |kReferenceTime| until 1 day after
+// |kReferenceTime|.
 bssl::UniquePtr<X509> MakeTestCert(const char *issuer,
                                           const char *subject, EVP_PKEY *key,
                                           bool is_ca);