This is a small note on the results of checking the OpenSSL project with the PVS-Studio analyzer. I analyzed the openssl-0.9.8-stable-SNAP-20121208 version.
I have recently written the post "Security, security! But do you test it?" about checking the TOR project. I mentioned the OpenSSL library together with it, as it is used in the TOR project.
The article provoked active discussions on some programmer resources. Programmers appear to be very much concerned about the quality of the OpenSSL library. But I wasn't able to answer some questions regarding the library. I was also reproached for not having informed the OpenSSL developers about all the unsafe fragments.
So I'd like to comment on this. You see, I didn't plan to check the OpenSSL library and study the results of its analysis when being involved in analyzing TOR. This library just happened to be around. The TOR project included some version of OpenSSL, that's why it was checked by the PVS-Studio analyzer along with the main project.
Then I decided to improve the situation and downloaded and checked the latest version of the OpenSSL library.
To be honest, there's not much to tell. Almost nothing strange was found. Those errors described in that earlier article are fixed by now. OpenSSL is a quality project; the library has been already checked by many tools (Clang, Cppcheck, Coverity, DoubleCheck, Coccinelle, Klocwork, etc.). So, the library is cleaned out. It would be a feat to find even one error there.
Ok, I will tell you about some suspicious things I've found in the code of the OpenSSL library. They are most likely just insignificant slip-ups rather than serious errors. But I have to write at least something, right? :)
EVP_PKEY *STORE_get_private_key(....)
{
STORE_OBJECT *object;
....
if (!object || !object->data.key || !object->data.key)
{
STOREerr(STORE_F_STORE_GET_PRIVATE_KEY,
STORE_R_FAILED_GETTING_KEY);
return 0;
}
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions '!object->data.key' to the left and to the right of the '||' operator. str_lib.c 475
The "!object->data.key" condition is repeated twice. The second check must be just odd, and there's nothing dangerous about it. But if it turns out that the programmer wanted to check another class member, this is certainly a trouble.
This strange check can be seen in three other fragments of the str_lib.c file (seems like Copy-Paste):
There are several fragments where the pointer is first used and then checked for being a null pointer. But only one fragment looks really strange to me:
int OBJ_obj2txt(char *buf, int buf_len,
const ASN1_OBJECT *a, int no_name)
{
....
if ((a == NULL) || (a->data == NULL)) {
buf[0]='\0';
return(0);
}
....
if (buf)
....
}PVS-Studio's diagnostic message: V595 The 'buf' pointer was utilized before it was verified against nullptr. Check lines: 448, 461. obj_dat.c 448
It is first checked that 'a' or 'a->data' equal zero. If it is so, the 'buf' pointer is used. But the 'buf' pointer itself may equal zero too. The check "if (buf)" a bit farther in the code hints at that.
The PVS-Studio analyzer seems to have found a true error in the following code fragment.
int ssl3_get_cert_verify(SSL *s)
{
int type=0,i,j;
....
if ((peer != NULL) && (type | EVP_PKT_SIGN))
{
al=SSL_AD_UNEXPECTED_MESSAGE;
SSLerr(SSL_F_SSL3_GET_CERT_VERIFY,
SSL_R_MISSING_VERIFY_MESSAGE);
goto f_err;
}
....
}PVS-Studio's diagnostic message: V617 Consider inspecting the condition. The '0x0010' argument of the '|' bitwise operation contains a non-zero value. s3_srvr.c 2394
The expression "(type | EVP_PKT_SIGN)" is always true. Perhaps the following code should be written here: "type & EVP_PKT_SIGN".
There are several senseless checks like the following one:
int MAIN(int argc, char **argv)
{
....
long dsa_c[DSA_NUM][2];
....
if (dsa_c[i] == 0)
{
dsa_c[i][0]=1;
dsa_c[i][1]=1;
}
....
}PVS-Studio's diagnostic message: V600 Consider inspecting the condition. The 'dsa_c[i]' pointer is always not equal to NULL. speed.c 1486
'dsa_c' here is a two-dimensional array. That's why the expression "dsa_c[i] == 0" is always true and therefore meaningless. There is a code fragment nearby:
if (rsa_c[i][0] == 0)
{
rsa_c[i][0]=1;
rsa_c[i][1]=20;
}Maybe the 'dsa_c' array should be handled in the same way. In this case the code should look like this:
if (dsa_c[i][0] == 0)
{
dsa_c[i][0]=1;
dsa_c[i][1]=1;
}This strange check can be found in several other fragments:
There are a few small slip-ups. They are definitely not errors - just excessive code. Here's an example of excessive code in a condition:
int ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
int c;
....
c= *(s++);
if (!( ((c >= 'a') && (c <= 'z')) ||
((c >= 'A') && (c <= 'Z')) ||
(c == ' ') || // <=
((c >= '0') && (c <= '9')) ||
(c == ' ') || (c == '\'') || // <=
(c == '(') || (c == ')') ||
(c == '+') || (c == ',') ||
(c == '-') || (c == '.') ||
(c == '/') || (c == ':') ||
(c == '=') || (c == '?')))
ia5=1;
....
}PVS-Studio's diagnostic message: V501 There are identical sub-expressions '(c == ' ')' to the left and to the right of the '||' operator. a_print.c 76
The check that the character is a space is repeated twice: the programmer wrote it just from inattention. Nothing dangerous. I saw a couple of such things in other fragments, but they are not interesting to mention.
The following thing I found difficult to figure out: in some fragments, the analyzer had detected a conversion of a memsize-type to a 32-bit type and then back to memsize. This is one of these places:
int ec_GFp_simple_points_make_affine(const EC_GROUP *group,
size_t num, EC_POINT *points[], BN_CTX *ctx)
{
BIGNUM **heap = NULL;
size_t pow2 = 0;
....
heap = OPENSSL_malloc(pow2 * sizeof heap[0]);
....
}PVS-Studio's diagnostic message: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'pow2'. ecp_smpl.c 1576
It appears that the OPENSSL_malloc macro is poorly written.
void *CRYPTO_malloc(int num, const char *file, int line);
#define OPENSSL_malloc(num) CRYPTO_malloc((int)num,__FILE__,__LINE__)As a result, we get the following code after preprocessing:
heap = CRYPTO_malloc ((int)pow2 * sizeof heap[0], __FILE__,__LINE__);This leads to crazy type conversions. The 'pow2' variable of the size_t type is explicitly cast to the 'int' type at first. Then, after being multiplied by 'sizeof()', the expression's type changes to size_t again. After that it is again cast to the 'int' type when calling the CRYPTO_malloc() function, the conversion being implicit this time.
Well, this type conversion is absolutely meaningless - just an occasion to make a mistake. For instance, one might write something like this:
int *p1, *p2;
int x, y;
....
p = OPENSSL_malloc(p1 == p2 ? x : y);The 'p1' pointer will lose the high-order bits on a 64-bit system, and the comparison result will be incorrect.
This is of course an artificial example, but one still shouldn't create macros like this. It should be at least rewritten in the following way:
#define OPENSSL_malloc(num) CRYPTO_malloc((int)(num),
__FILE__,__LINE__)The best thing, however, is not to use the 'int' type here at all. The allocated memory size should be passed in a memsize-type variable. For example, 'size_t'.
Thank you all for your attention. I will be glad if this note helps to somehow improve the OpenSSL library. As usually, I recommend that the library's authors don't stop at the strange fragments mentioned here but check the library once again and study the report themselves. We provide developers of open-source libraries with a free registration key for some time. Perhaps you will notice some dangerous fragments I disregarded.