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.

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

x509, ssl, pkcs7: try to parse as DER-encoding first #442

Merged
merged 1 commit into from
May 25, 2021
Merged

x509, ssl, pkcs7: try to parse as DER-encoding first #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions ext/openssl/ossl_pkcs7.c
View file
Open in desktop
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
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For security sensitive code, it's generally a good idea to avoid uninitialised variables.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean it should stay as SSL_SESSION *ctx = NULL;?

IMO this is a positive change. ctx is correctly set in both paths before referenced and I'd expect -Wuninitialized/-Wmaybe-uninitialized to warn me if it's not the case. If it happens to fail, AddressSanitizer or Valgrind will still be able to point it out on runtime.

By unconditionally initializing it with NULL, I'd rather worry about creating a bug where I forget to reassign to the variable correctly. Not in this case of ctx, but that could also be the source of a security bug - and those tools will be useless for finding such a bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, depending on the compiler may be an acceptable risk. My understanding is that for secure code, we should initialise all variables.

http://cwe.mitre.org/data/definitions/457.html

Once code has been compromised by buffer overflow, uninitialised variables can form part of the attack surface. Therefore, I strongly advise all variables should be initialised. One way to reduce the noise associated with this is to use C99 style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true in case there is an out-of-bounds memory access bug nearby, but zero'ing a single variable isn't an effective measure against such an attack since stack space not used for variables will still remain uninitialized.

The issue of CWE-457 itself should be mostly covered by the mentioned methods. Initializing unconditionally actually makes debugging the same kind of issue harder - junk just becomes NULL and we cannot use tools.

From #441 (comment):

https://www.blackhat.com/presentations/bh-europe-06/bh-eu-06-Flake.pdf

Valgrind will easily catch the "Compiler doesn't warn" example in the slide.

I would personally follow an established guideline for secure C programming.

Would you mind giving me a pointer? I've not heard of the rule. OpenSSL does not enforce that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only takes one mistake, even some which look like legitimate code (e.g. heartbleed). C has many areas where it is easy to unknowingly invoke undefined behaviour.

For secure default, we should avoid undefined behaviour. In the best case it's okay, but in the worst case it is security bug, program crash, strange optimisation, etc. I don't believe we should rely only on tools for secure code, although if this is part of CI, that's really great. IMHO, The code itself needs to be as safe as possible.

For a standard to follow, I'd recommend https://wiki.sei.cmu.edu/confluence/display/c/EXP33-C.+Do+not+read+uninitialized+memory

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the guideline is suggesting we initialize variables in this case. In my option, doing that here will only decrease readability. My eyes would try to find a code path that ends up with leaving ctx == NULL whenever I see such an initialization, which doesn't exist...

In general, I find pre-initialization will not necessarily make code safer. It can reduce the damage (dereferencing indeterminate or exposing potentially sensitive data) caused by a logic error, but that at the same time makes it harder to find the logic error itself in an early stage - it can then only be spotted by writing proper test code. I can't choose one always.


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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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