New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix loading more than one certificate in PEM format in the X509_load_cert_file_ex() function #22885

Closed

olszomal wants to merge 3 commits into openssl:master from olszomal:X509_load_cert_file

Contributor

olszomal commented Nov 30, 2023 •

edited

X509_load_cert_file_ex() was broken by commit ae29622 in PR #21545.

The following code shows this error.

#include <openssl/x509_vfy.h>

int main()
{
    const char *cafile = "./ca-certificates.crt";
    X509_STORE *store;
    X509_LOOKUP *lookup;
    STACK_OF(X509) *certs;
    int i;

    store = X509_STORE_new();
    lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file());
    X509_load_cert_file(lookup, cafile, X509_FILETYPE_PEM);
    certs = X509_STORE_get1_all_certs(store);
    for(i=0; i<sk_X509_num(certs); i++) {
        char *name=X509_NAME_oneline(X509_get_subject_name(sk_X509_value(certs, i)), NULL, 0);
        printf("#%d: %s\n", i, name);
        OPENSSL_free(name);
    }
    sk_X509_pop_free(certs, X509_free);
    X509_STORE_free(store);
}

The ca-certificates.crt file contains 5 sample certificates in PEM format.
This PoC shows that there is only one certificate in the store, the last one in the file.

#0: /serialNumber=G63287510/C=ES/O=ANF Autoridad de Certificacion/OU=ANF CA Raiz/CN=ANF Secure Server Root CA

My PR fixes this by freeing an X509 object before any further use.
Additionally, it allocates and initializes an X509 structure with a library context and property query for all certificates.

#0: /C=ES/O=FNMT-RCM/OU=AC RAIZ FNMT-RCM
#1: /CN=ACCVRAIZ1/OU=PKIACCV/O=ACCV/C=ES
#2: /O=mkcert development CA/OU=mo@localhost-live.home/CN=mkcert mo@localhost-live.home
#3: /C=ES/O=FNMT-RCM/OU=Ceres/organizationIdentifier=VATES-Q2826004J/CN=AC RAIZ FNMT-RCM SERVIDORES SEGUROS
#4: /serialNumber=G63287510/C=ES/O=ANF Autoridad de Certificacion/OU=ANF CA Raiz/CN=ANF Secure Server Root CA


          Fix loading more than one certificate in PEM format in the X509_load_…

09826b7

…cert_file_ex() function

t8m reviewed

View reviewed changes

Member

t8m left a comment

Would you be able to add a testcase? There are already files with multiple certs in them in test/certs - for example the leaf-chain.pem file.

t8m added branch: master triaged: bug severity: regression branch: 3.2 approval: review pending approval: otc review pending labels

olszomal force-pushed the X509_load_cert_file branch from 38f3489 to ca9a5d2 Compare

November 30, 2023 14:47

t8m requested changes

View reviewed changes

Member

t8m left a comment

Good, just nits

test/x509_load_cert_file_test.c Outdated Show resolved Hide resolved

test/recipes/60-test_x509_load_cert_file.t Outdated Show resolved Hide resolved

test/x509_load_cert_file_test.c Outdated Show resolved Hide resolved


          Add a test for X509_load_cert_file()

de4d64f

olszomal force-pushed the X509_load_cert_file branch from ca9a5d2 to de4d64f Compare

November 30, 2023 16:57

t8m approved these changes

View reviewed changes

t8m removed the approval: otc review pending label

AlexanderOMara mentioned this pull request

Revert logic change from refactor ae29622 #22897

Closed

1 task

This was referenced Nov 30, 2023

Compatibility issue with openssl@3.2.0 mtrojnar/osslsigncode#325

Closed

openssl@3.2.0 update makes osslsigncode verify fail Homebrew/homebrew-core#155744

Closed

Use X509_LOOKUP_load_file() to load certificates with a lookup using the implementation X509_LOOKUP_file() mtrojnar/osslsigncode#326

Merged

mattcaswell requested changes

View reviewed changes

test/x509_load_cert_file_test.c Outdated

+                  X509_STORE *store = NULL;
+                  X509_LOOKUP *lookup = NULL;
+                  STACK_OF(X509) *certs = NULL;
+                  const char *chain = test_get_argument(n);

Member

mattcaswell Dec 1, 2023

This is a misuse of the n parameter. The argument to this function is the index of the test. The test will be repeated a given number of times and n tells you which iteration this is.

Member

t8m Dec 1, 2023

Actually this is only a partial misuse - there could be a loop going through all arguments here. On the other hand this requires all the test PEM files having 4 certificates because this is hardcoded below so I am not quite sure it is that useful to loop.

test/x509_load_cert_file_test.c Outdated

+                  if (!TEST_int_gt(n, 0))
+                      return 0;
+                  ADD_ALL_TESTS(test_load_cert_file, n);

Member

mattcaswell Dec 1, 2023

You should just use ADD_TEST here. The last argument to ADD_ALL_TESTS is the number of times that the test should be repeated. But we only want to run the test once.

t8m requested changes

View reviewed changes

test/x509_load_cert_file_test.c Outdated


		#include "testutil.h"

		const char *chain;

Member

t8m Dec 1, 2023

This must be static const char *chain;

test/x509_load_cert_file_test.c

+                      return 0;
+                  }
+                  chain = test_get_argument(0);

Member

t8m Dec 1, 2023

You should check that chain != NULL.


          Remove n parameter from x509_load_cert_file_test

877aa3f

olszomal force-pushed the X509_load_cert_file branch from 23694bc to 877aa3f Compare

December 1, 2023 11:27

mattcaswell approved these changes

View reviewed changes

t8m approved these changes

View reviewed changes

t8m added approval: done tests: present and removed approval: review pending labels

openssl-machine added approval: ready to merge and removed approval: done labels

Collaborator

openssl-machine commented Dec 2, 2023

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request


          Fix loading more than one certificate in PEM format in X509_load_cert…

20c680d

…_file_ex()

Fixes #22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22885)

openssl-machine pushed a commit that referenced this pull request


          Add a test for X509_load_cert_file()

d6961af

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22885)

Member

t8m commented Dec 4, 2023

Merged to the master and 3.2 branches. Thank you for your contribution.

t8m closed this

openssl-machine pushed a commit that referenced this pull request


          Fix loading more than one certificate in PEM format in X509_load_cert…

cafccb7

…_file_ex()

Fixes #22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22885)

(cherry picked from commit 20c680d)

openssl-machine pushed a commit that referenced this pull request


          Add a test for X509_load_cert_file()

fd105ab

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22885)

(cherry picked from commit d6961af)

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request


          Fix loading more than one certificate in PEM format in X509_load_cert…

2b07d00

…_file_ex()

Fixes #22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22885)

Signed-off-by: fly2x <fly2x@hitls.org>

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request


          Add a test for X509_load_cert_file()

6176ca1

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22885)

Signed-off-by: fly2x <fly2x@hitls.org>

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request


          Fix loading more than one certificate in PEM format in X509_load_cert…

891bd05

…_file_ex()

Fixes #22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22885)

(cherry picked from commit 20c680de9c435534be48fa85b2a975067a4e7c9d)
Signed-off-by: fly2x <fly2x@hitls.org>

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request


          Add a test for X509_load_cert_file()

8edabcc

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22885)

(cherry picked from commit d6961af1acbdf29b684f3307578bd03890a26a9c)
Signed-off-by: fly2x <fly2x@hitls.org>

wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request


          Fix loading more than one certificate in PEM format in X509_load_cert…

6b1b944

…_file_ex()

Fixes openssl#22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#22885)

wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request


          Add a test for X509_load_cert_file()

1726d3e

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#22885)

This was referenced Feb 27, 2024

A memleak introduced between 3.1.1 and 3.2 #23693

Closed

Fix a memory leak on successful load of CRL #23695

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment