x509store: let X509::Store#add_file raise TypeError if nil is given

Undo special treatment of nil and simply pass the value to StringValueCStr(). nil was never a valid argument for the method; OpenSSL::X509::StoreError with an unhelpful error message "system lib" was raised in that case.
ruby · Aug 12, 2020 · fb2fcbb · fb2fcbb
1 parent f36af95
commit fb2fcbb
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 20 deletions.
diff --git a/ext/openssl/ossl_x509store.c b/ext/openssl/ossl_x509store.c
@@ -301,17 +301,15 @@ ossl_x509store_add_file(VALUE self, VALUE file)
 {
     X509_STORE *store;
     X509_LOOKUP *lookup;
-    char *path = NULL;
+    const char *path;
 
-    if(file != Qnil){
-	path = StringValueCStr(file);
-    }
     GetX509Store(self, store);
+    path = StringValueCStr(file);
     lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file());
-    if(lookup == NULL) ossl_raise(eX509StoreError, NULL);
-    if(X509_LOOKUP_load_file(lookup, path, X509_FILETYPE_PEM) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (!lookup)
+        ossl_raise(eX509StoreError, "X509_STORE_add_lookup");
+    if (X509_LOOKUP_load_file(lookup, path, X509_FILETYPE_PEM) != 1)
+        ossl_raise(eX509StoreError, "X509_LOOKUP_load_file");
 #if OPENSSL_VERSION_NUMBER < 0x10101000 || defined(LIBRESSL_VERSION_NUMBER)
     /*
      * X509_load_cert_crl_file() which is called from X509_LOOKUP_load_file()
@@ -336,17 +334,15 @@ ossl_x509store_add_path(VALUE self, VALUE dir)
 {
     X509_STORE *store;
     X509_LOOKUP *lookup;
-    char *path = NULL;
+    const char *path;
 
-    if(dir != Qnil){
-	path = StringValueCStr(dir);
-    }
     GetX509Store(self, store);
+    path = StringValueCStr(dir);
     lookup = X509_STORE_add_lookup(store, X509_LOOKUP_hash_dir());
-    if(lookup == NULL) ossl_raise(eX509StoreError, NULL);
-    if(X509_LOOKUP_add_dir(lookup, path, X509_FILETYPE_PEM) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (!lookup)
+        ossl_raise(eX509StoreError, "X509_STORE_add_lookup");
+    if (X509_LOOKUP_add_dir(lookup, path, X509_FILETYPE_PEM) != 1)
+        ossl_raise(eX509StoreError, "X509_LOOKUP_add_dir");
 
     return self;
 }

diff --git a/test/openssl/test_x509store.rb b/test/openssl/test_x509store.rb
@@ -26,25 +26,44 @@ def test_nosegv_on_cleanup
     ctx.verify
   end
 
-  def test_add_file
+  def test_add_file_path
     ca_exts = [
       ["basicConstraints", "CA:TRUE", true],
       ["keyUsage", "cRLSign,keyCertSign", true],
     ]
-    cert1 = issue_cert(@ca1, @rsa1024, 1, ca_exts, nil, nil)
-    cert2 = issue_cert(@ca2, @rsa2048, 1, ca_exts, nil, nil)
-    tmpfile = Tempfile.open { |f| f << cert1.to_pem << cert2.to_pem; f }
+    cert1_subj = OpenSSL::X509::Name.parse_rfc2253("CN=Cert 1")
+    cert1_key = Fixtures.pkey("rsa-1")
+    cert1 = issue_cert(cert1_subj, cert1_key, 1, ca_exts, nil, nil)
+    cert2_subj = OpenSSL::X509::Name.parse_rfc2253("CN=Cert 2")
+    cert2_key = Fixtures.pkey("rsa-2")
+    cert2 = issue_cert(cert2_subj, cert2_key, 1, ca_exts, nil, nil)
 
+    # X509::Store#add_file reads concatenated PEM file
+    tmpfile = Tempfile.open { |f| f << cert1.to_pem << cert2.to_pem; f }
     store = OpenSSL::X509::Store.new
     assert_equal false, store.verify(cert1)
     assert_equal false, store.verify(cert2)
     store.add_file(tmpfile.path)
     assert_equal true, store.verify(cert1)
     assert_equal true, store.verify(cert2)
 
+    # X509::Store#add_path
+    Dir.mktmpdir do |dir|
+      hash1 = "%08x.%d" % [cert1_subj.hash, 0]
+      File.write(File.join(dir, hash1), cert1.to_pem)
+      store = OpenSSL::X509::Store.new
+      store.add_path(dir)
+
+      assert_equal true, store.verify(cert1)
+      assert_equal false, store.verify(cert2)
+    end
+
     # OpenSSL < 1.1.1 leaks an error on a duplicate certificate
     assert_nothing_raised { store.add_file(tmpfile.path) }
     assert_equal [], OpenSSL.errors
+
+    # Non-String is given
+    assert_raise(TypeError) { store.add_file(nil) }
   ensure
     tmpfile and tmpfile.close!
   end