To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
A few words about OpenSSL

A few words about OpenSSL

Dec 17 2012
Author:

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.

On checking OpenSSL

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? :)

Strange fragment N1

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):

  • V501 There are identical sub-expressions '!object->data.key' to the left and to the right of the '||' operator. str_lib.c 616
  • V501 There are identical sub-expressions '!object->data.key' to the left and to the right of the '||' operator. str_lib.c 670
  • V501 There are identical sub-expressions '!object->data.key' to the left and to the right of the '||' operator. str_lib.c 811

Strange fragment N2

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.

Strange fragment N3

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".

Strange fragment N4

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:

  • V600 Consider inspecting the condition. The 'ecdsa_c[i]' pointer is always not equal to NULL. speed.c 1506
  • V600 Consider inspecting the condition. The 'ecdsa_c[i]' pointer is always not equal to NULL. speed.c 1523
  • V600 Consider inspecting the condition. The 'ecdsa_c[i]' pointer is always not equal to NULL. speed.c 1540
  • V600 Consider inspecting the condition. The 'ecdh_c[i]' pointer is always not equal to NULL. speed.c 1560
  • V600 Consider inspecting the condition. The 'ecdh_c[i]' pointer is always not equal to NULL. speed.c 1577
  • V600 Consider inspecting the condition. The 'ecdh_c[i]' pointer is always not equal to NULL. speed.c 1594

Non-ambiguous 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 last thing worth mentioning

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'.

Conclusion

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.

Popular related articles
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept