Regression in 0.10.48 SubjectAlternativeName

[k0nserv]

Hello,

I've discovered a regression in 0.10.48 where the SAN extension generated by rust-openssl doesn't seem to be correct when encoded as DER. In particular the resulting SAN extension no longer parses correctly with Go's ParseCertificateRequest.

The code in question:

fn create_csr(pkey: &PKey<pkey::Private>, domains: &[&str]) -> X509Req {
    //
    // the csr builder
    let mut req_bld = X509ReqBuilder::new().expect("X509ReqBuilder");

    // set private/public key in builder
    req_bld.set_pubkey(pkey).expect("set_pubkey");

    // set all domains as alt names
    let mut stack = Stack::new().expect("Stack::new");
    let ctx = req_bld.x509v3_context(None);
    let as_lst = domains
        .iter()
        .map(|&e| format!("DNS:{}", e))
        .collect::<Vec<_>>()
        .join(",");
    let as_lst = as_lst[4..].to_string();
    let mut an = SubjectAlternativeName::new();
    an.dns(&as_lst);
    let ext = an.build(&ctx).expect("SubjectAlternativeName::build");
    stack.push(ext).expect("Stack::push");
    req_bld.add_extensions(&stack).expect("add_extensions");

    // sign it
    req_bld
        .sign(pkey, MessageDigest::sha256())
        .expect("csr_sign");
    // the csr
    req_bld.build()
}

Under 0.10.47 creating a CSR with create_csr(pkey, &["*.example.com"‚ "example.com"]) produces a CSR, that when parsed from Go results in a CertificateRequest with len(req.DNSNames) == 2. With 0.10.48 it instead parses as a CertificateRequest with len(req.DNSNames) == 1 and the sole DNSName is *.example.com,DNS:example.com.

I've created two branches to showcase this:

• https://github.com/k0nserv/rust-openssl/tree/working-on-0.10.47
• https://github.com/k0nserv/rust-openssl/tree/broken-on-0.10.48

Both include a new regression test that can be run as cargo test -- x509_extension_to_der_full. This test saves a DER to openssl/csr.der.

I've used the following Go program to parse the resulting DER

main.go

package main

import (
	"crypto/x509"
	"flag"
	"fmt"
	"io/ioutil"
	"log"
	"os"
)

func main() {
	// Define a command-line flag for the CSR input path
	inputPath := flag.String("input", "", "Path to the DER formatted CSR file")
	flag.Parse()

	// Check if the input path flag is provided
	if *inputPath == "" {
		fmt.Println("Please provide the path to the CSR file using the --input flag.")
		os.Exit(1)
	}
	// Read the DER certificate request file
	csrBytes, err := ioutil.ReadFile(*inputPath)
	if err != nil {
		log.Fatal(err)
	}

	// Parse the DER-encoded certificate request
	csr, err := x509.ParseCertificateRequest(csrBytes)
	if err != nil {
		log.Fatal(err)
	}

	// Access the fields of the certificate request
	fmt.Println("Certificate Request Info:")
	fmt.Printf("Subject: %s\n", csr.Subject.String())
	fmt.Printf("Signature Algorithm: %s\n", csr.SignatureAlgorithm.String())

	// You can access more fields of the certificate request if needed

	// Verify the signature of the certificate request
	err = csr.CheckSignature()
	if err != nil {
		log.Fatal("Certificate request signature verification failed:", err)
	}
	fmt.Println("Certificate request signature verified successfully!")

	// Access the DNS names in the subject alternative names extension
	if len(csr.DNSNames) > 0 {
		fmt.Println("Found", len(csr.DNSNames), "DNS names")
		fmt.Println("DNS Names:")
		for _, dnsName := range csr.DNSNames {
			fmt.Println(dnsName)
		}
	} else {
		fmt.Println("No DNS names found in the certificate request.")
	}
}

On the 0.10.47 branch the output of this program is:

Certificate Request Info:
Subject:
Signature Algorithm: ECDSA-SHA256
Certificate request signature verified successfully!
Found 2 DNS names
DNS Names:
*.example.com
example.com

On the the 0.10.48 branch the output of this program is:

Certificate Request Info:
Subject:
Signature Algorithm: ECDSA-SHA256
Certificate request signature verified successfully!
Found 1  DNS names
DNS Names:
*.example.com,DNS:example.com

See this screenshot:

Cause

It seems quite likely that this is a side-effect of #1854. I'm not sure if the syntax used in the example is supposed to work after #1854 or not.

Workaround

There's a trivial workaround:

diff --git a/openssl/src/regression_test.rs b/openssl/src/regression_test.rs
index a9047a57..51dc23dc 100644
--- a/openssl/src/regression_test.rs
+++ b/openssl/src/regression_test.rs
@@ -27,14 +27,10 @@ fn create_csr(pkey: &PKey<pkey::Private>, domains: &[&str]) -> X509Req {
     // set all domains as alt names
     let mut stack = Stack::new().expect("Stack::new");
     let ctx = req_bld.x509v3_context(None);
-    let as_lst = domains
-        .iter()
-        .map(|&e| format!("DNS:{}", e))
-        .collect::<Vec<_>>()
-        .join(",");
-    let as_lst = as_lst[4..].to_string();
     let mut an = SubjectAlternativeName::new();
-    an.dns(&as_lst);
+    for d in domains {
+        an.dns(d);
+    }
     let ext = an.build(&ctx).expect("SubjectAlternativeName::build");
     stack.push(ext).expect("Stack::push");
     req_bld.add_extensions(&stack).expect("add_extensions");

[alex]

You're correct that there was a behavior change here, this was necessary to fix an injection vulnerability: https://rustsec.org/advisories/RUSTSEC-2023-0023.html

We did not realize anyone was intentionally "exploiting" this issue.

[k0nserv]

Cool, I figured that'd be it. Anyway the workaround works fine so no problems.

Thanks for the response.

[alex]

Sorry for the inconvenience. Glad to hear the resolution was simple.

…

On Fri, May 12, 2023 at 8:33 AM Hugo Tunius ***@***.***> wrote: Closed #1923 <#1923> as completed. — Reply to this email directly, view it on GitHub <#1923 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBCLDTLFY4O45OJ573DXFYUZPANCNFSM6AAAAAAX7NC7Y4> . You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.