Skip to content

Commit

Permalink
x509, ssl, pkcs7: try to parse as DER-encoding first
Browse files Browse the repository at this point in the history
Methods that take both PEM-encoding and DER-encoding have not been
consistent in the order in which encoding to attempt to parse.

A DER-encoding may contain a valid PEM block ("\n-----BEGIN ..-----" to
"-----END ...-----") embedded within it. Also, the PEM-encoding parser
allows arbitrary data around the PEM block and silently skips it. As a
result, attempting to parse data in DER-encoding as PEM-encoding first
can incorrectly finds the embedded PEM block instead.

This commit ensures that DER encoding will always be attempted before
PEM encoding. OpenSSL::X509::Certificate is one of the updated classes.
With this, the following will always be true:

    # obj is an OpenSSL::X509::Certificate
    obj == OpenSSL::X509::Certificate.new(obj.to_der)
    obj == OpenSSL::X509::Certificate.new(obj.to_pem)
  • Loading branch information
rhenium committed May 19, 2021
1 parent 964d836 commit b280eb1
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 61 deletions.
20 changes: 9 additions & 11 deletions ext/openssl/ossl_pkcs7.c
Original file line number Diff line number Diff line change
Expand Up @@ -330,27 +330,25 @@ ossl_pkcs7_alloc(VALUE klass)
static VALUE
ossl_pkcs7_initialize(int argc, VALUE *argv, VALUE self)
{
PKCS7 *p7, *pkcs = DATA_PTR(self);
PKCS7 *p7, *p7_orig = RTYPEDDATA_DATA(self);
BIO *in;
VALUE arg;

if(rb_scan_args(argc, argv, "01", &arg) == 0)
return self;
arg = ossl_to_der_if_possible(arg);
in = ossl_obj2bio(&arg);
p7 = PEM_read_bio_PKCS7(in, &pkcs, NULL, NULL);
p7 = d2i_PKCS7_bio(in, NULL);
if (!p7) {
OSSL_BIO_reset(in);
p7 = d2i_PKCS7_bio(in, &pkcs);
if (!p7) {
BIO_free(in);
PKCS7_free(pkcs);
DATA_PTR(self) = NULL;
ossl_raise(rb_eArgError, "Could not parse the PKCS7");
}
OSSL_BIO_reset(in);
p7 = PEM_read_bio_PKCS7(in, NULL, NULL, NULL);
}
DATA_PTR(self) = pkcs;
BIO_free(in);
if (!p7)
ossl_raise(rb_eArgError, "Could not parse the PKCS7");

RTYPEDDATA_DATA(self) = p7;
PKCS7_free(p7_orig);
ossl_pkcs7_set_data(self, Qnil);
ossl_pkcs7_set_err_string(self, Qnil);

Expand Down
53 changes: 24 additions & 29 deletions ext/openssl/ossl_ssl_session.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,43 +34,38 @@ static VALUE ossl_ssl_session_alloc(VALUE klass)
* Creates a new Session object from an instance of SSLSocket or DER/PEM encoded
* String.
*/
static VALUE ossl_ssl_session_initialize(VALUE self, VALUE arg1)
static VALUE
ossl_ssl_session_initialize(VALUE self, VALUE arg1)
{
SSL_SESSION *ctx = NULL;

if (RDATA(self)->data)
ossl_raise(eSSLSession, "SSL Session already initialized");

if (rb_obj_is_instance_of(arg1, cSSLSocket)) {
SSL *ssl;

GetSSL(arg1, ssl);

if ((ctx = SSL_get1_session(ssl)) == NULL)
ossl_raise(eSSLSession, "no session available");
} else {
BIO *in = ossl_obj2bio(&arg1);
SSL_SESSION *ctx;

ctx = PEM_read_bio_SSL_SESSION(in, NULL, NULL, NULL);
if (RTYPEDDATA_DATA(self))
ossl_raise(eSSLSession, "SSL Session already initialized");

if (!ctx) {
OSSL_BIO_reset(in);
ctx = d2i_SSL_SESSION_bio(in, NULL);
}
if (rb_obj_is_instance_of(arg1, cSSLSocket)) {
SSL *ssl;

BIO_free(in);
GetSSL(arg1, ssl);

if (!ctx)
ossl_raise(rb_eArgError, "unknown type");
}
if ((ctx = SSL_get1_session(ssl)) == NULL)
ossl_raise(eSSLSession, "no session available");
}
else {
BIO *in = ossl_obj2bio(&arg1);

/* should not happen */
if (ctx == NULL)
ossl_raise(eSSLSession, "ctx not set - internal error");
ctx = d2i_SSL_SESSION_bio(in, NULL);
if (!ctx) {
OSSL_BIO_reset(in);
ctx = PEM_read_bio_SSL_SESSION(in, NULL, NULL, NULL);
}
BIO_free(in);
if (!ctx)
ossl_raise(rb_eArgError, "unknown type");
}

RDATA(self)->data = ctx;
RTYPEDDATA_DATA(self) = ctx;

return self;
return self;
}

static VALUE
Expand Down
17 changes: 10 additions & 7 deletions ext/openssl/ossl_x509cert.c
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,27 @@ static VALUE
ossl_x509_initialize(int argc, VALUE *argv, VALUE self)
{
BIO *in;
X509 *x509, *x = DATA_PTR(self);
X509 *x509, *x509_orig = RTYPEDDATA_DATA(self);
VALUE arg;

rb_check_frozen(self);
if (rb_scan_args(argc, argv, "01", &arg) == 0) {
/* create just empty X509Cert */
return self;
}
arg = ossl_to_der_if_possible(arg);
in = ossl_obj2bio(&arg);
x509 = PEM_read_bio_X509(in, &x, NULL, NULL);
DATA_PTR(self) = x;
x509 = d2i_X509_bio(in, NULL);
if (!x509) {
OSSL_BIO_reset(in);
x509 = d2i_X509_bio(in, &x);
DATA_PTR(self) = x;
OSSL_BIO_reset(in);
x509 = PEM_read_bio_X509(in, NULL, NULL, NULL);
}
BIO_free(in);
if (!x509) ossl_raise(eX509CertError, NULL);
if (!x509)
ossl_raise(eX509CertError, "PEM_read_bio_X509");

RTYPEDDATA_DATA(self) = x509;
X509_free(x509_orig);

return self;
}
Expand Down
17 changes: 10 additions & 7 deletions ext/openssl/ossl_x509crl.c
Original file line number Diff line number Diff line change
Expand Up @@ -93,23 +93,26 @@ static VALUE
ossl_x509crl_initialize(int argc, VALUE *argv, VALUE self)
{
BIO *in;
X509_CRL *crl, *x = DATA_PTR(self);
X509_CRL *crl, *crl_orig = RTYPEDDATA_DATA(self);
VALUE arg;

rb_check_frozen(self);
if (rb_scan_args(argc, argv, "01", &arg) == 0) {
return self;
}
arg = ossl_to_der_if_possible(arg);
in = ossl_obj2bio(&arg);
crl = PEM_read_bio_X509_CRL(in, &x, NULL, NULL);
DATA_PTR(self) = x;
crl = d2i_X509_CRL_bio(in, NULL);
if (!crl) {
OSSL_BIO_reset(in);
crl = d2i_X509_CRL_bio(in, &x);
DATA_PTR(self) = x;
OSSL_BIO_reset(in);
crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
}
BIO_free(in);
if (!crl) ossl_raise(eX509CRLError, NULL);
if (!crl)
ossl_raise(eX509CRLError, "PEM_read_bio_X509_CRL");

RTYPEDDATA_DATA(self) = crl;
X509_CRL_free(crl_orig);

return self;
}
Expand Down
17 changes: 10 additions & 7 deletions ext/openssl/ossl_x509req.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,26 @@ static VALUE
ossl_x509req_initialize(int argc, VALUE *argv, VALUE self)
{
BIO *in;
X509_REQ *req, *x = DATA_PTR(self);
X509_REQ *req, *req_orig = RTYPEDDATA_DATA(self);
VALUE arg;

rb_check_frozen(self);
if (rb_scan_args(argc, argv, "01", &arg) == 0) {
return self;
}
arg = ossl_to_der_if_possible(arg);
in = ossl_obj2bio(&arg);
req = PEM_read_bio_X509_REQ(in, &x, NULL, NULL);
DATA_PTR(self) = x;
req = d2i_X509_REQ_bio(in, NULL);
if (!req) {
OSSL_BIO_reset(in);
req = d2i_X509_REQ_bio(in, &x);
DATA_PTR(self) = x;
OSSL_BIO_reset(in);
req = PEM_read_bio_X509_REQ(in, NULL, NULL, NULL);
}
BIO_free(in);
if (!req) ossl_raise(eX509ReqError, NULL);
if (!req)
ossl_raise(eX509ReqError, "PEM_read_bio_X509_REQ");

RTYPEDDATA_DATA(self) = req;
X509_REQ_free(req_orig);

return self;
}
Expand Down
12 changes: 12 additions & 0 deletions test/openssl/test_x509cert.rb
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,18 @@ def test_read_from_file
}
end

def test_read_der_then_pem
cert1 = issue_cert(@ca, @rsa2048, 1, [], nil, nil)
exts = [
# A new line before PEM block
["nsComment", "Another certificate:\n" + cert1.to_pem],
]
cert2 = issue_cert(@ca, @rsa2048, 2, exts, nil, nil)

assert_equal cert2, OpenSSL::X509::Certificate.new(cert2.to_der)
assert_equal cert2, OpenSSL::X509::Certificate.new(cert2.to_pem)
end

def test_eq
now = Time.now
cacert = issue_cert(@ca, @rsa1024, 1, [], nil, nil,
Expand Down

0 comments on commit b280eb1

Please sign in to comment.