1894572 - Use after free in sftk_getParameters()

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P5)

Description

[mozillabugs]

sftk_getParameters() (nss/lib/softoken/pkcs11.c) uses the freed variable libParams if the call to PR_Read() fails.

Using the following code from FIREFOX_124_0_2_RELEASE, imagine that control reaches line 3384, then PR_Read() returns an error. That causes line 3385 to execute, freeing libParams [1]. Control then falls through to line 3392, then line 3397 (whose condition is false), then line 3405, which uses the freed variable:

3352: static CK_RV
3353: sftk_getParameters(CK_C_INITIALIZE_ARGS *init_args, PRBool isFIPS,
3354:                    sftk_parameters *paramStrings)
3355: {
...
3360:     PRBool free_mem = PR_FALSE;
3361: 
3362:     if (!init_args || !init_args->LibraryParameters) {
3363:         /* Library parameters were not provided via C_Initialize_args*/
3364: 
3365:         /* Enviromental value has precedence to configuration filename */
3366:         libParams = PR_GetEnvSecure("NSS_LIB_PARAMS");
3367: 
3368:         if (!libParams) {
3369:             /* Load from config filename or use default */
3370:             filename = PR_GetEnvSecure("NSS_LIB_PARAMS_FILE");
...
3376:             if (filename) {
3377:                 file_dc = PR_OpenFile(filename, PR_RDONLY, 444);
3378:                 if (file_dc) {
3379:                     /* file opened */
3380:                     PRInt32 len = PR_Available(file_dc);
3381:                     libParams = PORT_NewArray(char, len + 1);
3382:                     if (libParams) {
3383:                         /* memory allocated */
3384:                         if (PR_Read(file_dc, libParams, len) == -1) {
3385:                             PR_Free(libParams);
3386:                         } else {
3387:                             free_mem = PR_TRUE;
3388:                             libParams[len] = '\0';
3389:                         }
3390:                     }
3391: 
3392:                     PR_Close(file_dc);
3393:                 }
3394:             }
3395:         }
3396: 
3397:         if (!libParams)
3398:             libParams = LIB_PARAM_DEFAULT;
3399: 
3400:     } else {
...
3403:     }
3404: 
3405:     crv = sftk_parseParameters(libParams, paramStrings, isFIPS);
...
3414:         PR_Free(libParams);
...
3417: }

[1] Also, line 3385 should be using PORT_Free(), not PR_Free(), as should line 3414.

Comment 1

[Daniel Veditz [:dveditz]]

John, is this possibly related to the softoken crash you were looking at?

Comment 2

[John Schanck [:jschanck]]

No, Firefox initializes softoken with a non-null value in init_args->LibraryParameters. To hit this bug you have to omit library parameters and you have to set the NSS_LIB_PARAMS_FILE environment variable.

This function was added in Bug 1837987 so that softoken could be loaded by other applications. I'm not sure if RedHat is using this, or if it was just added for testing. fkrenzel, can you take a look?

Comment 3

[fkrenzel]

Hey, thanks for catching the PORT_Free(),

As to the libParams on the line 3398. This shouldn't be a problem as libParams is a pointer (char* if I recall correctly) and the operation just assigns the static address of LIB_PARAM_DEFAULT to it.

This is what happens:
libParams → PR_GetEnvSecure("NSS_LIB_PARAMS"); //Points to the Environmental variable
libParams → Array[len + 1] //Point to newly allocated memory
libParams → NULL // freed the array
libParams → LIB_PARAM_DEFAULT // Point tho the statically allocated memory

Comment 4

[mozillabugs]

(In reply to fkrenzel from comment #3)

Hey, thanks for catching the PORT_Free(),

As to the libParams on the line 3398. This shouldn't be a problem as libParams is a pointer (char* if I recall correctly) and the operation just assigns the static address of LIB_PARAM_DEFAULT to it.

This is what happens:
libParams → PR_GetEnvSecure("NSS_LIB_PARAMS"); //Points to the Environmental variable
libParams → Array[len + 1] //Point to newly allocated memory
libParams → NULL // freed the array
libParams → LIB_PARAM_DEFAULT // Point tho the statically allocated memory

I don't see where libParams gets assigned NULL. Line 3385 just frees the memory, so the condition on line 3397 is false and line 3398 never gets executed. Then line 3405 uses the freed memory. PR_Free() does not null its argument, and in fact cannot, because the argument isa void * and not a void ** or void *&. https://searchfox.org/mozilla-central/source/nsprpub/pr/src/malloc/prmem.c#470 .

Comment 5

[fkrenzel]

(In reply to mozillabugs from comment #4)

(In reply to fkrenzel from comment #3)

Hey, thanks for catching the PORT_Free(),

As to the libParams on the line 3398. This shouldn't be a problem as libParams is a pointer (char* if I recall correctly) and the operation just assigns the static address of LIB_PARAM_DEFAULT to it.

This is what happens:
libParams → PR_GetEnvSecure("NSS_LIB_PARAMS"); //Points to the Environmental variable
libParams → Array[len + 1] //Point to newly allocated memory
libParams → NULL // freed the array
libParams → LIB_PARAM_DEFAULT // Point tho the statically allocated memory

I don't see where libParams gets assigned NULL. Line 3385 just frees the memory, so the condition on line 3397 is false and line 3398 never gets executed. Then line 3405 uses the freed memory. PR_Free() does not null its argument, and in fact cannot, because the argument isa void * and not a void ** or void *&. https://searchfox.org/mozilla-central/source/nsprpub/pr/src/malloc/prmem.c#470 .

I see, you are correct, will propose a patch for this.

Comment 6

[fkrenzel]

Attachment: Bug 1894572 - sftk_getParameters(): Fix fallback to default variable after error with configfile. r=jschanck

Comment 7

[fkrenzel]

Patch proposed, thanks! For reporting and for your follow-up explanation of the problem.

Comment 8

[mozillabugs]

(In reply to fkrenzel from comment #7)

Patch proposed, thanks! For reporting and for your follow-up explanation of the problem.

Thanks for fixing the bugs.

Comment 9

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/6fcf40ee865e2d4a7f27f36153649667c6237b04