Boring article about checking OpenSSL

Not so long ago, a vulnerability was discovered in OpenSSL, which only a lazy one does not speak about. I know that PVS-Studio is not able to find an error that leads to this vulnerability. So I decided that there is no reason to write any article about OpenSSL. In recent days, and so has become too many articles on this topic. However, I received a flurry of letters asking me to tell if PVS-Studio can find this error. I gave up and wrote this article.

OpenSSL Verification

I think everyone already knows that a serious vulnerability has been discovered in OpenSSL . If, after all, someone missed it or wants to know more details, I suggest to get acquainted with several articles on this topic:

• The Heartbleed Bug .
• Existential type crisis: Diagnosis of the OpenSSL Heartbleed Bug .
• Wikipedia. Heartbleed
• Heartbleed
• Answers the Critical Question of Keys Using Heartbleed?
• NSA Said to Exploit Heartbleed Bug for Intelligence for Years .
• Man who introduced serious 'Heartbleed' security flaw denies he inserted it deliberately .
• The Heartbleed Hit List .
• Open Source software is for all of the others .

To be brief, the vulnerability allowing access to other people's data existed in the code for about 2 years. During this time, no code analyzer found it, although this library was not tested only by the lazy one.
')
We tested OpenSSL too. Here is a note on this topic: " A little about OpenSSL ". Something we found, but it seems nothing serious. Now these errors are fixed. So no wonder checked.

I did not specify if we checked OpenSSL when the Heartbleed bug was already there or not. In any case, I know that PVS-Studio cannot detect such a bug. It is generally difficult to detect. The OpenSSL project was tested and verified by various tools, and none of them found this error. For example, I did not find an error, the leader among the analyzers of the Coverity Scan code. Notes about this: " Heartbleed and Static Analysis ", " Heartbleed and static analysis (2) ".

The fact is that such an error is very difficult to find using static analysis. The code is too confusing. It is necessary to take into account the values ​​stored in memory, you need to understand what lies behind the obvious type conversions and so on. Even a person is hard to understand what the problem is. Static analyzers here pass. This is not a flaw in the static analysis methodology. It's just that the mistake is really complicated. Probably, there is no tool that can find such a defect, if it is not previously trained to search for such structures.

It should be noted that there are still known and unknown static analysis tools designed to search for software bookmarks. Probably, similar tools could find vulnerability, but I strongly doubt. If found, they would use it as an advertisement for their analyzer. There is, of course, the option that some tool developed within the special services finds this vulnerability, but they do not specifically talk about it. However, conspirology begins here, so let's not talk about it.

My personal opinion. This is just a mistake, but not a bookmark. Static analysis tools do not know how to detect it, since it is complex. That's all.

At this point it was possible to finish the article, but it was not at all interesting. Therefore, I once again checked OpenSSL with PVS-Studio . I didn’t find anything special, but still let's look at some parts of the code.

Why is there so little? Yes, because OpenSSL is a quality project. The fact that a serious vulnerability was found in it does not mean at all that the code is terrible. I think that in many applications there are many big holes, they just are not needed by anyone. Plus, the OpenSSL project is being tested with many tools.

Analysis results

Once again I want to repeat that I did not find any special errors. It would be better if we consider the text below not as a description of errors, but as comments on the code, which seemed to me to be inaccurate. I do not want to see comments later that I am inflating an elephant from a fly.

Suspicious comparison

typedef struct ok_struct { .... size_t buf_len_save; size_t buf_off_save; .... } BIO_OK_CTX; static int ok_read(BIO *b, char *out, int outl) { .... BIO_OK_CTX *ctx; .... /* copy start of the next block into proper place */ if(ctx->buf_len_save - ctx->buf_off_save > 0) .... } 

PVS-Studio warning: V555 The expression of the 'A - B> 0' kind will work as 'A! = B'. bio_ok.c 243

The expression (ctx-> buf_len_save - ctx-> buf_off_save> 0) does not work as it might seem at first glance.

It seems that here they want to check the condition (ctx-> buf_len_save> ctx-> buf_off_save). This is not true. The point is that the compared variables are of unsigned type. Subtracting one unsigned variable from another unsigned variable gives the result of an unsigned type.

The condition (ctx-> buf_len_save - ctx-> buf_off_save> 0) is always met if the variables do not match. In other words, the following two expressions are equivalent:

• (ctx-> buf_len_save - ctx-> buf_off_save> 0)
• (ctx-> buf_len_save! = ctx-> buf_off_save)

Explanation for those who are not very familiar with the C language. Skilled developers can not read.

Suppose we have two 32-bit variables of unsigned type:

unsigned A = 10;

unsigned B = 20;

Check whether the condition (A - B> 0) is fulfilled.

The result of the subtraction (A - B) is 10u - 20u = 0xFFFFFFF6u = 4294967286u.

Then we compare the unsigned number 4294967286u with zero. Zero also leads to unsigned type, but it does not matter.

The expression (4294967286u> 0u) is true.

The expression (A - B> 0) will be false only in one case, when A == B.

Is this comparison a mistake? I do not know, because I am not familiar with the project device. I think that there is no error.

Most likely, this is the case. The variable 'buf_len_save' is usually larger than the variable '' buf_off_save '. Only sometimes the value of the variable 'buf_off_save' reaches the value stored in 'buf_len_save'. In this case, when the variables match and this check is needed. The case when (buf_len_save <buf_off_save) is probably impossible.

Uninitialized variable that does not cause trouble

There is a place where an uninitialized variable can be used. But it will not lead to any bad consequences. Here is the code:

 int PEM_do_header(....) { int i,j,o,klen; .... if (o) o = EVP_DecryptUpdate(&ctx,data,&i,data,j); if (o) o = EVP_DecryptFinal_ex(&ctx,&(data[i]),&j); .... j+=i; if (!o) { PEMerr(PEM_F_PEM_DO_HEADER,PEM_R_BAD_DECRYPT); return(0); } .... } 

PVS-Studio Warning: V614 Potentially uninitialized variable 'i' used. pem_lib.c 480

The variable 'i' may be uninitialized if (o == false). As a result, it is not clear what will be added to 'j'. This is not a problem, since if (o == false), the error handler is triggered, and the function stops its work.

The code is correct, but not accurate. It is better to first check the variable 'o', and only then use the 'i':

 if (!o) { PEMerr(PEM_F_PEM_DO_HEADER,PEM_R_BAD_DECRYPT); return(0); } j+=i; 

Strange assignments

 #define SSL_TLSEXT_ERR_ALERT_FATAL 2 int ssl3_accept(SSL *s) { .... if (ret != SSL_ERROR_NONE) { ssl3_send_alert(s,SSL3_AL_FATAL,al); if (al != TLS1_AD_UNKNOWN_PSK_IDENTITY) SSLerr(SSL_F_SSL3_ACCEPT,SSL_R_CLIENTHELLO_TLSEXT); ret = SSL_TLSEXT_ERR_ALERT_FATAL; ret= -1; goto end; } .... } 

PVS-Studio warning: V519 The 'ret' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 376, 377. s3_srvr.c 377

At the beginning of the variable 'ret' is assigned the value 2, and then -1. Probably the first assignment is superfluous and remained in the code by accident.

Another case:

 int dtls1_retransmit_message(....) { .... /* save current state */ saved_state.enc_write_ctx = s->enc_write_ctx; saved_state.write_hash = s->write_hash; saved_state.compress = s->compress; saved_state.session = s->session; saved_state.epoch = s->d1->w_epoch; saved_state.epoch = s->d1->w_epoch; .... } 

PVS-Studio warning: V519 The 'saved_state.epoch' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1277, 1278. d1_both.c 1278

Potential null pointer dereference

The most common bloop in programs ( in my experience ) is dereferencing a pointer before it is checked. It is not always a mistake. Often the pointer will never be null. However, this is a potentially dangerous code. especially when the project is changing rapidly.

There are some bloopers in OpenSSL:

 int SSL_shutdown(SSL *s) { if (s->handshake_func == 0) { SSLerr(SSL_F_SSL_SHUTDOWN, SSL_R_UNINITIALIZED); return -1; } if ((s != NULL) && !SSL_in_init(s)) return(s->method->ssl_shutdown(s)); else return(1); } .... } 

PVS-Studio warning: V595 The pointer 's pointer was used before it was verified against nullptr. Check lines: 1013, 1019. ssl_lib.c 1013

At the beginning, the 's' pointer is used: (s-> handshake_func == 0).

And only then it is checked: (s! = NULL).

Another more difficult case:

 #define bn_wexpand(a,words) \ (((words) <= (a)->dmax)?(a):bn_expand2((a),(words))) static int ubsec_dh_generate_key(DH *dh) { .... if(bn_wexpand(pub_key, dh->p->top) == NULL) goto err; if(pub_key == NULL) goto err; .... } 

PVS-Studio warning: V595 The 'pub_key' pointer was used before it was verified against nullptr. Check lines: 951, 952. e_ubsec.c 951

To understand where the error is, you need to open the macros. Then we get this code:

 if((((dh->p->top) <= (pub_key)->dmax)? (pub_key):bn_expand2((pub_key), (dh->p->top))) == ((void *)0)) goto err; if(pub_key == ((void *)0)) goto err; 

Note the pointer 'pub_key'.

Initially, it is dereferenced: (pub_key) -> dmax.

Below it is checked for equality to zero: (pub_key == ((void *) 0)).

Extra checks

There are several code snippets where a variable is compared twice with the same value. I think this is not a mistake. It seems that just the second check was written randomly and simply superfluous. It can be removed.

Excess check N1

 int ASN1_PRINTABLE_type(const unsigned char *s, int len) { .... 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 warning: V501 There are identical sub-expressions '(c ==' ')' operator. a_print.c 76

I selected identical checks using "<<<< ====". I wrote about this duplicate check in the previous article, but it is not fixed. So this is definitely not a problem.

Excess check N2, N3

 int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) { .... if ((type && (type != SSL3_RT_APPLICATION_DATA) && (type != SSL3_RT_HANDSHAKE) && type) || (peek && (type != SSL3_RT_APPLICATION_DATA))) .... } 

PVS-Studio warning: V501 operator. s3_pkt.c 952

It is checked twice that the 'type' variable has a nonzero value.

The above code snippet was copied to another file, so there is also a superfluous comparison: d1_pkt.c 760.

Incorrect row length

It is not good to set the length of lines using magic constants. It is very easy to make a mistake. In OpenSSL, the PVS-Studio analyzer noticed three such places.

The first failed magic number

To demonstrate that this is a mistake, let's look at a few examples of calling the BIO_write function:

• BIO_write (bp, “Error in encoding \ n”, 18)
• BIO_write (bp, "\ n", 1)
• BIO_write (bp, ":", 1)
• BIO_write (bp, ": BAD OBJECT", 11)
• BIO_write (bp, “Bad boolean \ n”, 12)

As you can see from these examples, the last number specifies the length of the string.

And now, the incorrect code:

 static int asn1_parse2(....) { .... if (BIO_write(bp,"BAD ENUMERATED",11) <= 0) goto end; .... } 

PVS-Studio warning: V666 Consider the third argument of the function 'BIO_write'. This is not the case. asn1_par.c 378

The length of the string “BAD ENUMERATED” is not 11, but 14 characters.

The second failed magic number

 static int www_body(char *hostname, int s, unsigned char *context) { .... if ( ((www == 1) && (strncmp("GET ",buf,4) == 0)) || ((www == 2) && (strncmp("GET /stats ",buf,10) == 0))) .... } 

PVS-Studio warning: V666 Consider the third argument of the function 'strncmp'. It is possible that it’s not passed on. s_server.c 2703

The length of the string “GET / stats” is not 10, but 11 characters. The last space is not taken into account. Minor bug, but still a bug.

The third failed magic number

 static int asn1_cb(const char *elem, int len, void *bitstr) { .... if (!strncmp(vstart, "ASCII", 5)) arg->format = ASN1_GEN_FORMAT_ASCII; else if (!strncmp(vstart, "UTF8", 4)) arg->format = ASN1_GEN_FORMAT_UTF8; else if (!strncmp(vstart, "HEX", 3)) arg->format = ASN1_GEN_FORMAT_HEX; else if (!strncmp(vstart, "BITLIST", 3)) arg->format = ASN1_GEN_FORMAT_BITLIST; else .... } 

PVS-Studio warning: V666 Consider the third argument of the function 'strncmp'. This is not the case. asn1_gen.c 371

The trouble is here:

 if (!strncmp(vstart, "BITLIST", 3)) 

The length of the string "BITLIST" is 7 characters.

I digress a little. The reader may have a question about how PVS-Studio finds such errors. I will explain. It collects information about function calls, in this case about strncmp (), and builds a data matrix:

• vstart, "ASCII", 5
• vstart, "UTF8", 4
• vstart, "HEX", 3
• vstart, "BITLIST", 3

The function has a string argument and a numeric one. Basically the length of the string is the same as the number. So this argument is the length of the string. In one place this is not the case and it means that a warning V666 should be issued.

Not very good

It is not very good to print out the pointer value using "% 08lX". For this, there is a "% p".

 typedef struct mem_st { void *addr; .... } MEM; static void print_leak_doall_arg(const MEM *m, MEM_LEAK *l) { .... BIO_snprintf(bufp, BUF_REMAIN, "number=%d, address=%08lX\n", m->num,(unsigned long)m->addr); .... } 

It is not a pointer that is passed to the function, but a value of type (unsigned long). Therefore, the compiler and some analyzers will keep silent.

PVS-Studio noticed this flaw indirectly. He does not like that the pointer explicitly lead to type (unsigned long). It is not right. No one guaranteed that the pointer would fit in the 'long' type. For example, this can not be done in Win64.

Correct and shorter code:

 BIO_snprintf(bufp, BUF_REMAIN, "number=%d, address=%p\n", m->num, m->addr); 

There are three places where the pointer value is incorrectly printed:

• mem_dbg.c 699
• bio_cb.c 78
• asn1_lib.c 467

Conclusion

Although static analyzers did not find this error, and it existed for a very long time, I still urge everyone to use static analysis in their daily work. Just do not look for a silver bullet that will solve all the problems and completely relieve the program code from errors. The best result is achieved in an integrated approach: unit tests, static and dynamic analysis , regression tests, and so on. Static analysis will eliminate a lot of typos and stupid errors at the coding stage and this will save time on other useful things, such as new functionality or writing more thorough tests.

Try our code analyzer PVS-Studio .

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. A Boring Article About the OpenSSL Project .