libxls test.c and test2.c - NULL pointer dereference vulnerability due to xls_parseWorkSheet() misuse

[lockedbyte]

libxls xls_parseWorkSheet() - NULL pointer dereference

INTRODUCTION

A NULL pointer dereference vulnerability has been detected in the function xls_parseWorkSheet() when trying to access the rdi register that is NULL at that time.

Looking at the code we can realize it is because of trying to access long offset = pWS->filepos; to that pointer, which is the argument for the function.

The argument for xls_parseWorkSheet() comes from the return value of xls_getWorkSheet():

pWS = xls_getWorkSheet(pWB, i);
xls_parseWorkSheet(pWS);

If we enter the function xls_getWorkSheet():

xlsWorkSheet * xls_getWorkSheet(xlsWorkBook* pWB,int num)
{
    xlsWorkSheet * pWS = NULL;
    verbose ("xls_getWorkSheet");
    if (num >= 0 && num < (int)pWB->sheets.count) {
        pWS = calloc(1, sizeof(xlsWorkSheet));
        pWS->filepos=pWB->sheets.sheet[num].filepos;
        pWS->workbook=pWB;
        pWS->rows.lastcol=0;
        pWS->rows.lastrow=0;
        pWS->colinfo.count=0;
    }
    return pWS;
}

We can easily deduce that if either num is less than 0, or num is greater than pWB->sheets.count the conditional won't succeed, thus returning pWS directly without assigning it a heap chunk.

As the pointer has been initialized with NULL, a NULL pointer is returned, which will be the argument for the next function, and the program will crash when trying to access it.

REPRODUCE

To reproduce this crash, the test2_libxls file has been used with a specially crafted XLS file.

The crafted file needs to have no BoundSheet8 records to make pWB->sheets.count be 0.

root@ubuntu:/software/libxls-1.6.2# .libs/test2_libxls min/crash.xls 
ole2_open: min/crash.xls
libxls : xls_open_ole
libxls : xls_parseWorkBook
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 16
    Not Processed in parseWoorkBook():  BOF=0x3030 size=16
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 2
    Not Processed in parseWoorkBook():  BOF=0x3030 size=2
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 2
    Not Processed in parseWoorkBook():  BOF=0x3030 size=2
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 0
    Not Processed in parseWoorkBook():  BOF=0x3030 size=0
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 112
    Not Processed in parseWoorkBook():  BOF=0x3030 size=112
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 48
    Not Processed in parseWoorkBook():  BOF=0x3030 size=48
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 48
    Not Processed in parseWoorkBook():  BOF=0x3030 size=48
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 2
    Not Processed in parseWoorkBook():  BOF=0x3030 size=2
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 2
    Not Processed in parseWoorkBook():  BOF=0x3030 size=2
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 48
    Not Processed in parseWoorkBook():  BOF=0x3030 size=48
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 49
    Not Processed in parseWoorkBook():  BOF=0x3030 size=49
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 26880
    Not Processed in parseWoorkBook():  BOF=0x3030 size=26880
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 3030h Unknown ()
   Size: 12336
    Not Processed in parseWoorkBook():  BOF=0x3030 size=12336
----------------------------------------------
libxls : BOF
   ID: 000Ah EOF (End of File)
   Size: 48
libxls : xls_getWorkSheet
Segmentation fault

ANALYSIS

As I explained in the Introduction section, we need num to be less than 0 or greater than pWB->sheets.count. Having one (or both) of those requirements can trigger the NULL pointer dereference.

In main():

for (i = 0; i < pWB->sheets.count; i++) {
	int isFirstLine = 1;

The first condition will never fail as i will always be greater or equal to 0.

Also, i cannot be greater than pWB->sheets.count as depends on it's value to be incremented.

But... what if pWB->sheets.count is equal to 0? Then i will be 0 too.

In the conditional i won't be less than pWB->sheets.count, but equal, thus failing the conditional.

The function xls_addSheet(), responsible for incrementing the pWB->sheets.count value is never executed with the current PoC xls file, thus keeping until crash time the value which the program gave it when being initialized at xls_open_ole():

pWB->sheets.count=0;
pWB->xfs.count=0;
pWB->fonts.count=0;
pWB->charset = strdup(charset ? charset : "UTF-8");

But how do we make the program to not execute xls_addSheet() ?

We initially need to avoid any XLS_RECORD_BOUNDSHEET record.

If we craft an XLS file that do not contain any BoundSheet8 record, this code will never reached:

case XLS_RECORD_BOUNDSHEET:
		{
			//printf("ADD SHEET\n");
			BOUNDSHEET *bs = (BOUNDSHEET *)buf;
        xlsConvertBoundsheet(bs);
			// different for BIFF5 and BIFF8
        if ((retval = xls_addSheet(pWB, bs, bof1.size)) != LIBXLS_OK) {
            goto cleanup;
        }
		}
    break;

And pWB->sheets.count will be 0 when reaching the conditional code.

The result is a SIGSEGV (Segmentation fault) interruption crashing the program trying to access non-mapped memory:

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── threads ────
[#0] Id 1, Name: "test2_libxls", stopped 0x7ffff7fab7cd in xls_parseWorkSheet (), reason: SIGSEGV
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── trace ────
[#0] 0x7ffff7fab7cd → xls_parseWorkSheet(pWS=0x0)
[#1] 0x555555555545 → main(argc=<optimized out>, argv=0x7fffffffe378)
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
gef➤  p pWS
$9 = (xlsWorkSheet *) 0x0
gef➤  x/i $rip
=> 0x7ffff7fab7cd <xls_parseWorkSheet+77>:	mov    rdx,QWORD PTR [rdi+0x18]
gef➤  

IMPACT

The most common issue with this type of vulnerability is a Denial of Service (DoS) once a crash has been triggered as demonstrated above with the crash PoC.

SOLUTION

A solution for this issue could be adding a pointer check before using it, and change the actions once a pointer is detected to be NULL.

Patch example:

xls_error_t xls_parseWorkSheet(xlsWorkSheet* pWS)
{
    BOF tmp;
    BYTE* buf = NULL;
    
    /* --- PATCH START --- */
    
    if(pWS == NULL)
    	return -1; // or any value to be checked from the parent function when failing
    
    /* --- PATCH END --- */
    	
    long offset = pWS->filepos;
    size_t read;
    xls_error_t retval = 0;

	struct st_cell_data *cell = NULL;
	xlsWorkBook *pWB = pWS->workbook;

    verbose ("xls_parseWorkSheet");

    if ((retval = xls_preparseWorkSheet(pWS)) != LIBXLS_OK) {
        goto cleanup;
    }
	// printf("size=%d fatpos=%d)\n", pWS->workbook->olestr->size, pWS->workbook->olestr->fatpos);

    if ((retval = xls_makeTable(pWS)) != LIBXLS_OK) {
        goto cleanup;
    }

Obviously, the pointer is used multiple times in the code, so having it NULL is something not expected. A more complex patch is needed to avoid unexpected results for the next functions to be executed instead of just returning if a NULL pointer is detected.

Anyway, a better solution for this patch is update the conditional at xls_getWorkSheet() to this:

if (num >= 0 && num <= (int)pWB->sheets.count) {

This time, if num is equal to pWB->sheets.count the heap chunk will be returned avoiding a NULL pointer being returned.

[evanmiller]

Hi, thank you for the extensive report. However I believe that your concerns are somewhat misplaced; the library behaves as intended, returning NULL from getWorkSheet for a non-existent sheet index.

There is indeed a bug in test2.c inasmuch as it should ensure that sheets.count > 0 before calling xls_getWorkSheet with an index of 0. However, this bug represents a misuse of the library, rather than a problem in the library itself. You can see an example of correct library usage in fuzz/fuzz_xls.c.

I will be happy to entertain a pull request that modifies test2.c to use the library correctly. Because test2.c is not considered production code, I do not see a DoS attack vector based on the current report.

[lockedbyte]

Hi,

Actually xls2csv.c has a protection agains zero-sheet xls files as the calls to xls_getWorkSheet() and xls_parseWorkBook() are incapsulated into a for loop that iters among the available sheets:

for (i = 0; i < pWB->sheets.count; i++) {

If no loop there, it would be the same as test2.c.

The bug has been reproduced in both test and test2.

The bug can be reproduced just with the following steps when opening a zero-sheet XLS file:

1. get a pWB pointer somehow through any of those functions like xls_open_file().
2. call xls_getWorkSheet() with the pWB (anything is valid as num argument).
3. there you have a NULL pointer returned

As you say, programmers when using the library should take care about:

1. Check if pWB->sheets.count is greater than 0.
2. Check if the xls_getWorkSheet() returned pointer is not NULL

Both test.c and test2.c (which are examples for the library usage) are vulnerable, and a patch would be either checking pWB->sheets.count or checking a NULL returned from xls_getWorkSheet() .

Also it would be better to add a NULL pointer check at the beginning of xls_parseWorkBook() function knowing this can be a common misuse.

I'll change title to a more proper problem description

Thanks

[lockedbyte]

UPDATE: Requested 3 Pull requests

1. Fixing NULL pointer dereference on test.c ( Fix NULL pointer dereference on test.c #96 )
2. Fixing NULL pointer dereference on test2.c ( Fix NULL pointer dereference on test2.c #95 )
3. Adding check code on some functions to avoid library misuse leading to NULL pointer dereference ( Fix possible library misuse (NULL ptr deref) #97 )

I leave the solution for the decision of accepting pull requests or not.

Closing this issue

Thanks