2017-10-03 Note: I originally posted this blog entry on the Aspect Security blog around 2015-02-13. I am mirroring it here with only formatting changes. Introduction The spat of SSL and TLS issues over the last year have caused concern about the quality of the encrypted tunnel in Internet communications. The various creatively named BEAST, CRIME, & POODLE attacks against SSLv3 have effectively killed the entire SSLv3 protocol. Bugs in different encryption libraries have created additional means of exploit, such as with OpenSSL’s HeartBleed affecting TLS and Apple’s GotoFail SSL and (partial) TLS attack. In these cases, the TLS protocol itself is safe, but the implementation of TLS reduced the overall level of protection. On 19 January 2015, the PolarSSL team announced an issue that allows for a heap break with possible remote code execution. As PolarSSL is used by some VPN packages, this could allow for compromise of a company’s gateway server and thus access to the internal network. Again, like HeartBleed and GotoFail, this comes down to a coding bug. The Security Advisory has the full details. The PolarSSL bug involves the uninitialized use of memory during the processing of a certificate. When it is time to free that memory, the exploit can be invoked. The solution is to initialize the memory with zeros. The discoverers of the bug, Certified Secure, have a full explanation. The erroneous code uses a modified version of the classic C linked-list structure containing data and a pointer to the next entry in the list. If the next entry happens to be NULL, then this indicates the current entry is last one in the list. Here is a simplified example of the classic design: C struct node { struct data_type *data; struct node *next; }; void delete_list(struct node* list) { struct node *temp = NULL; while (*list) { temp = list; list = list->next; // set the new "head" pointer for the loop delete_data(temp->data); // delete the data free(temp); // delete the node } } void load_list(struct node *head) { struct node *current = head, *rec = NULL, *temp = NULL; while (rec = load_next_record() ) { temp = (struct node *)malloc(sizeof(struct node)); // get linked list memory temp->data = malloc(sizeof(data_type)); // get memory for data temp->next = NULL; // set to end-of-list copy_record_into_node(rec, temp->data); // copy data into list current->next = temp; // append new record to list current = temp; // set new "tail" pointer for the loop } } 1234567891011121314151617181920212223242526 struct node { struct data_type *data; struct node *next; }; void delete_list(struct node* list) { struct node *temp = NULL; while (*list) { temp = list; list = list->next; // set the new "head" pointer for the loop delete_data(temp->data); // delete the data free(temp); // delete the node } } void load_list(struct node *head) { struct node *current = head, *rec = NULL, *temp = NULL; while (rec = load_next_record() ) { temp = (struct node *)malloc(sizeof(struct node)); // get linked list memory temp->data = malloc(sizeof(data_type)); // get memory for data temp->next = NULL; // set to end-of-list copy_record_into_node(rec, temp->data); // copy data into list current->next = temp; // append new record to list current = temp; // set new "tail" pointer for the loop } } The PolarSSL equivalent of load_next_record() is a bit more complicated, due to the manner in which the ASN1 data structure is read. The system allocates memory before it is needed as opposed to creating the memory when it is needed. Thus, the last record in the linked list contains an empty record and the corresponding next pointer is NULL. In pictorial form, here are the tail nodes of both types of linked lists. The orange arrows represent the “next” pointer inside each node. Linked List Visualizations As can be seen in both implementations, the final pointer is aimed at NULL. Let’s look more in depth at the PolarSSL code which loads the ASN1 sequence: C while( *p < end ) { buf = &(cur->buf); buf->tag = **p; if( ( ret = asn1_get_tag( p, end, &buf->len, tag ) ) != 0 ) return( ret ); buf->p = *p; *p += buf->len; 123456789 while( *p < end ) { buf = &(cur->buf); buf->tag = **p; if( ( ret = asn1_get_tag( p, end, &buf->len, tag ) ) != 0 ) return( ret ); buf->p = *p; *p += buf->len; During the penultimate iteration of the while loop, the buf and buf->tag are saved before the call to asn1_get_tag(). Then asn1_get_tag() will read a value of that variety of a tag and will set the length read into buf->len. If a successful read of asn1_get_tag() occurs, ret will be zero, and the while loop will continue. The head pointer p will be incremented by the length. The next part of the loop contains this if statement: C /* Allocate and assign next pointer */ if( *p < end ) { /* Get memory to help the "next" asn1 sequence */ cur->next = (asn1_sequence *) polarssl_malloc( sizeof( asn1_sequence ) ); /* If we can't get memory, return an error */ if( cur->next == NULL ) return( POLARSSL_ERR_ASN1_MALLOC_FAILED ); /* Otherwise, just use that memory uninitialized. */ cur = cur->next; } } /* Set final sequence entry's next pointer to NULL */ cur->next = NULL; 12345678910111213141516 /* Allocate and assign next pointer */ if( *p < end ) { /* Get memory to help the "next" asn1 sequence */ cur->next = (asn1_sequence *) polarssl_malloc( sizeof( asn1_sequence ) ); /* If we can't get memory, return an error */ if( cur->next == NULL ) return( POLARSSL_ERR_ASN1_MALLOC_FAILED ); /* Otherwise, just use that memory uninitialized. */ cur = cur->next; } } /* Set final sequence entry's next pointer to NULL */ cur->next = NULL; If this were the final iteration, p now points to the end of the data stream, then the while loop exists and the next pointer is set to NULL. However, if data still exists, then the if (*p ...) is triggered causing memory for another ASN1 sequence is malloc()‘d and the cur “head” to be updated to this new & uninitialized memory. Upon the next iteration, if the next ASN1 tag is not the same the current and existing tag type (e.g., an error in the data stream), the function returns. This means that the final entry in the linked list has a next element that points to uninitialized memory since next was never explicitly set to NULL. In other words, next could point to some potentially problematic locations. Linked List With Tail Error The attack isn’t triggered during the allocation of the memory, it is triggered during the free(). Eventually, when x509_crt_free() is called to release the memory, this loop runs: C seq_cur = cert_cur->ext_key_usage.next; while( seq_cur != NULL ) { seq_prv = seq_cur; seq_cur = seq_cur->next; polarssl_zeroize( seq_prv, sizeof( x509_sequence ) ); polarssl_free( seq_prv ); } 1234567 seq_cur = cert_cur->ext_key_usage.next; while( seq_cur != NULL ) { seq_prv = seq_cur; seq_cur = seq_cur->next; polarssl_zeroize( seq_prv, sizeof( x509_sequence ) ); polarssl_free( seq_prv ); } This is the same as the classic C linked-list release, except that memory is zero’d out before the free. Ignoring the zero’ing, imagine the loop runs and encounters the final legitimate entry in the linked list. This was allocated correctly, but not initialized. This while loop will save the value of seq_cur->next and release the legitimate memory. However, seq_cur->next was never initialized and this pointer could reference anywhere in memory. At the next iteration of the loop, seq_prv becomes seq_cur and the system will zeroize seq_prv and then attempt to free it. If the memory location is validly accessible by the application, the OS will happily allow it to be zero’d and free’d. If the uninitialized next points to another valid location, the loop will run again. At best, the location will be invalid and the software will core dump and crash (denial of service). At worst, the heap could be broken and data injected elsewhere could be executed (pwnage). The fix for PolarSSL is to initialize the memory after it has been malloc()‘d, thus: C /* Allocate and assign next pointer */ if( *p < end ) { /* Get memory to help the "next" asn1 sequence */ cur->next = (asn1_sequence *) polarssl_malloc( sizeof( asn1_sequence ) ); /* If we can't get memory, return an error */ if( cur->next == NULL ) return( POLARSSL_ERR_ASN1_MALLOC_FAILED ); memset( cur->next, 0, sizeof( asn1_sequence ) ); // NEW LINE OF CODE /* Otherwise, just use that memory uninitialized. */ cur = cur->next; } 123456789101112131415 /* Allocate and assign next pointer */ if( *p < end ) { /* Get memory to help the "next" asn1 sequence */ cur->next = (asn1_sequence *) polarssl_malloc( sizeof( asn1_sequence ) ); /* If we can't get memory, return an error */ if( cur->next == NULL ) return( POLARSSL_ERR_ASN1_MALLOC_FAILED ); memset( cur->next, 0, sizeof( asn1_sequence ) ); // NEW LINE OF CODE /* Otherwise, just use that memory uninitialized. */ cur = cur->next; } The use of the memset() will cause the next value in the newly allotted memory to be zero, e.g. NULL. This is the patch submitted by Certified Secure. Detection Could we have detected this error with current automated tools? Let’s take a look. A thorough run of two static scanning tool AppScan Source and GrepBugs against the codebase did not find this memory issue. Both tools produced a number of false positives. Another static analysis tool, Splint, did detect that something was amiss: asn1parse.c:275:13: Implicitly only storage cur->next (type struct _asn1_sequence *) not released before assignment: cur->next = (asn1_sequence *)malloc(sizeof(asn1_sequence)) asn1parse.c:279:36: Storage *p reachable from parameter is kept (should be unqualified) asn1parse.c:269:9: Storage *p becomes kept asn1parse.c:279:36: Function returns with possibly null storage derivable from parameter cur->next A possibly null pointer is reachable from a parameter or global variable that is not declared using a /*@null@*/ annotation. (Use -nullstate to inhibit warning) asn1parse.c:275:25: Storage cur->next may become null asn1parse.c:279:36: Storage cur->next->buf reachable from parameter contains 3 undefined fields: tag, len, p asn1parse.c:279:36: Storage *(cur->next) reachable from parameter contains 1 undefined field: next 12345678910111213141516 asn1parse.c:275:13: Implicitly only storage cur->next (type struct _asn1_sequence *) not released before assignment: cur->next = (asn1_sequence *)malloc(sizeof(asn1_sequence)) asn1parse.c:279:36: Storage *p reachable from parameter is kept (should be unqualified) asn1parse.c:269:9: Storage *p becomes kept asn1parse.c:279:36: Function returns with possibly null storage derivable from parameter cur->next A possibly null pointer is reachable from a parameter or global variable that is not declared using a /*@null@*/ annotation. (Use -nullstate to inhibit warning) asn1parse.c:275:25: Storage cur->next may become null asn1parse.c:279:36: Storage cur->next->buf reachable from parameter contains 3 undefined fields: tag, len, p asn1parse.c:279:36: Storage *(cur->next) reachable from parameter contains 1 undefined field: next But before we say “use Splint”, it must be noted that splint generated 42 lines of error messages for the asn1_get_sequence_of() function which contains only 46 lines of code (including comments and whitespace). In total, 86 errors were reported in asn1parse.c file. Each of the error messages above also occur in many other files. If Splint were run to exclude what it thought were “–weak” or unlikely errors, then the findings above would not even be reported. In addition, Splint itself crashed when processing the ripemd160.c file in PolarSSL. With the level of false positives and noise that Splint produces, it is not likely to be a trustworthy source for finding memory issues. AppScan and GrepBugs produced less false positives but did not detect this particular security issue in PolarSSL. Attempts to run the codebase through the Coccinelle static analyzer were not successful due to configuration issues. The dynamic memory checker Valgrind was also used to test for this memory leak in PolarSSL. As Valgrind requires the software to be actively run, the PolarSSL unit test suite was run inside of the Valgrind environment. The program test_suite_x509parse runs seven tests which call asn1parse() and during each one, no memory issue was detected. The test suite does provide some invalid ASN1 data streams for error-condition testing, but none of the test cases trigger the asn1parse() error condition with the invalid tag during processing. If the invalid data created by the Certified Secure team is added to the test suite, then Valgrind may be able to detect the invalid memory access. The PolarSSL and Certified Secure teams are wisely waiting a reasonable time frame before adding this attack to the test suite. Prevention The solution was to initialize the memory with zeros so that future calls to walk the linked list will properly reach the end-of-list NULL marker. As such, the issue could have been prevented by using calloc() instead of malloc() globally throughout PolarSSL. The calloc() function basically performs malloc() + memset(0...). Some schools of thought suggest using a calloc()-like function with all calls to obtain memory instead of malloc(). For instance, in Java and C#, all data is zero’d out during the memory allocation. In some BSD Unixen, the operating system zeros out unused memory during CPU idle cycles. The gcc/clang family of compilers have an option to auto-zero some variables upon creation. Visual Studio has a debug mode whereby all uninitialized memory is automatically set to 0xDC for detection of heap and stack leaks. The issue is well known and many tools provide assistance to detect and correct. However, calloc() can be slower for the innermost loops of a heavily taxed application as two operations are performed (malloc() + memset()) instead of one. This StackOverflow Article has a few great explanations of the subtle differences. Generally, in nearly all cases, Aspect Security recommends the use of calloc() to be safe. Most of the code in PolarSSL can easily use calloc() without performance impact. The ASN1 and X509 parsing code is usually performed once per connection and calloc() costs would be negligible. Conclusions Memory leaks are hard to detect by automated tools, so safe coding practices must be employed. The best protection is to employ a language or environment which has built-in memory protection. If that is not possible, follow safe coding practices, such as zeroing out memory in instances where ever pointers are employed. References Certified Secure official alert PolarSSL announcement The problem file on GitHub – all versions before 1.3.9 are vulnerable GotoFail does not affect TLS v1.2 per this explanation Full GotoFail description, including some TLS issues, per these guys Everything you need to know about HeartBleed in pictorial form Command for Valgrind, ymmv: valgrind -v --leak-check=full --smc-check=all ./test_suite_x509parse > 509.txt 2>&1 The test involving AppScan Source was a comprehensive run with out-of-the-box configuration and GrepBugs was performed using the most recently downloaded matches. Custom rules were not configured for either application. Share this:Click to share on Twitter (Opens in new window)Click to share on LinkedIn (Opens in new window)Click to share on Reddit (Opens in new window)Click to share on Pocket (Opens in new window)