Skip to content
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.

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

ca behaviour with -spkac and stdout changed/broke text interface in 1.1.0i #8055

Closed
twegener-embertec opened this issue Jan 22, 2019 · 13 comments

Comments

@twegener-embertec
Copy link

Since around 1.1.0i, the following openssl ca command (with -spkac option and no -out option, i.e. use stdout) now generates binary certificate details rather than text (which breaks applications that parse that output):

OPENSSL_ENABLE_MD5_VERIFY=1 openssl ca -config test.openssl.cnf -passin stdin -batch -spkac input_file -startdate 190121130654Z
Check that the SPKAC request matches the signature
Signature ok
Certificate Details:
...
        X509v3 extensions:
            X509v3 Basic Constraints: 
                CA:FALSE
            Netscape Cert Type: 
                SSL Client
...
Certificate is to be certified until Jan 19 01:58:28 2029 GMT (3650 days)

Write out database with 1 new entries
...(elided binary goop)...

This was observed in openssl-1.1.0i from Fedora 28:
openssl-1.1.0i-1.fc28.x86_64

In earlier versions it would output the PEM format Base64 encoded certificate data in a block surrounded with:

-----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----

This interface change/breakage is quite painful.

@twegener-embertec
Copy link
Author

I suspect that this may have been introduced by the following commit (which was released in 1.1.0i):
29a7148

Associated pull request for that commit:
Fix openssl ca, to correctly make output file binary when using -spkac #6050

diff --git a/apps/ca.c b/apps/ca.c
index d474a2b69a..eb093d0e0b 100644
--- a/apps/ca.c
+++ b/apps/ca.c
@@ -725,8 +725,12 @@ end_of_options:
 
     /*****************************************************************/
     if (req || gencrl) {
-        /* FIXME: Is it really always text? */
-        Sout = bio_open_default(outfile, 'w', FORMAT_TEXT);
+        if (spkac_file != NULL) {
+            output_der = 1;
+            batch = 1;
+        }
+        Sout = bio_open_default(outfile, 'w',
+                                output_der ? FORMAT_ASN1 : FORMAT_TEXT);
         if (Sout == NULL)
             goto end;
     }
@@ -872,10 +876,6 @@ end_of_options:
                     BIO_printf(bio_err, "Memory allocation failure\n");
                     goto end;
                 }
-                if (outfile) {
-                    output_der = 1;
-                    batch = 1;
-                }
             }
         }
         if (ss_cert_file != NULL) {

@twegener-embertec twegener-embertec changed the title ca behaviour with -spkac and stdout changed/broke text interface from 1.1.0h to 1.1.0i ca behaviour with -spkac and stdout changed/broke text interface in 1.1.0i Jan 22, 2019
@twegener-embertec
Copy link
Author

Breaking the interface seems like kind of a big deal. Can someone please comment on this?

@mattcaswell
Copy link
Member

Looks like a possible bug to me. This is what is in the documentation:

"When processing SPKAC format, the output is DER if the B<-out>
flag is used, but PEM format if sending to stdout or the B<-outdir>
flag is used."

Perhaps @levitte can comment.

@levitte
Copy link
Member

levitte commented Feb 28, 2019

Does the following diff fix the issue for you?

diff --git a/apps/ca.c b/apps/ca.c
index c69a2b5cdd..5b33fa6aa1 100644
--- a/apps/ca.c
+++ b/apps/ca.c
@@ -725,7 +725,7 @@ end_of_options:
 
     /*****************************************************************/
     if (req || gencrl) {
-        if (spkac_file != NULL) {
+        if (spkac_file != NULL && outfile != NULL) {
             output_der = 1;
             batch = 1;
         }

@levitte
Copy link
Member

levitte commented Feb 28, 2019

There is a PR for this now, #8368

@twegener-embertec
Copy link
Author

@levitte Thanks, that diff fixes my issue.
Will this be backported to the 1.1.0 series? (That is used by Ubuntu 18.04 LTS, so it is currently broken there.)

@mattcaswell
Copy link
Member

Will this be backported to the 1.1.0 series?

No. The 1.1.0 series is in security-fix only mode. Therefore this bug fix does not qualify.

@twegener-embertec
Copy link
Author

Ah, okay, no worries.
It looks like openssl 1.1.1 will be coming to Ubuntu 18.04 LTS anyway:
https://bugs.launchpad.net/ubuntu/bionic/+source/openssl/+bug/1797386

@twegener-embertec
Copy link
Author

Correction: Ubuntu 18.04 LTS has 1.1.0g-2ubuntu4.3, and so does not have this regression.
However, current 1.1.1 does have this regression, so when Ubuntu 18.04 brings in OpenSSL 1.1.1 via an SRU [1] it currently will introduce this regression. :-(

[1] https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1797386/

Any idea when the fix for this issue will make it into a 1.1.1 series update?

@mattcaswell
Copy link
Member

Any idea when the fix for this issue will make it into a 1.1.1 series update?

It will be in 1.1.1c which we have no date for yet. However we typically do a release every 3-4 months unless a pressing security issue requires us to do one earlier. 1.1.1b came out at the end of February.

@xnox
Copy link
Contributor

xnox commented May 8, 2019

Will this be cherrypicked into https://github.com/openssl/openssl/commits/OpenSSL_1_1_1-stable/apps/ca.c ? i see other fixes on the 1_1_1 branch since 1.1.1b but not this one.

@mattcaswell
Copy link
Member

Ah...#8368 was supposed to have been cherry-picked to 1.1.1 already, but it looks like it wasn't. I'll ping that PR.

levitte added a commit that referenced this issue Jun 10, 2019
So say the docs

Fixes #8055

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #8368)

(cherry picked from commit 69f6b3c)
@twegener-embertec
Copy link
Author

Just noting FTR that this regression fix did not make it into 1.1.1c (presumably due to the oversight mentioned above). So presumably this will be in the (yet-to-be released) 1.1.1d?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants