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

[rhenium]

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)

[ioquatix]

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

[rhenium]

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.

[ioquatix]

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.

[rhenium]

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.

[ioquatix]

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

[rhenium]

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.