• Robert Relyea's avatar
    Bug 1703936 New coverity/cpp scanner errors. · 350807b3
    Robert Relyea authored
    Redhat has run our scanners on the full NSS tree and identifed 123
    errors our security team determined where 'critical'. I've reviewed
    the errors and identified a much smaller subset of errors that are
    either real, or confusing enough to warrent suppression comments.
    
    Many errors are in cmd and gtest. I've skip those commands red hat does
    not ship in this report, and I've skipped the issues in gtest.
    
    Also, There's a large number of leaked_storage errors because evidently
    coverity gets confused when you have a pointer in a local variable and
    you pass that pointer off to a global or a function variable. I've
    skipped most of those as well.
    
    changes:
    crlutil.c: add missing arena free in error path. #def4
    secutil.c: (Not coverity found) make sure we don't overflow our buffer
               in badly encoded ECC oids.
    modultil.c: Fix incorrect free operation in pk11install case. #def6
    signtool/javascript.c: free old archiveDir after use in PR_smprintf #def8
                           don't double free curitem (curitem is almost
                           certainly NULL at this point, so the current code
                           is a noop). #def9, #def10
    signtool/list.c: remove unused ugly_list variable (which is leaked) #def12
    signtool/util.c: don't leak 'dir' in error path #def13 #def14
    sigver/pk7print.c: coverity: use static pk rather than an allocated and
                       leaked pointer. #def15
                       add code for EC
                       disbled DSA code that isn't actually working
                       (PQG params).
    symkeyutil.c: free name (depends on PORT_Free null check). #def16
    pkix_pl_nameconstraints.c: Fix coverity double free warning.
                               PKIX_ERROR_RECEIVED is almost
                               certainly false in this case, but by
                               setting arena to NULL we make
                               sure it's not used or freed again. #def99
    pkix_pl_string.c: Fix varargs leak in the error path. #def109
    pk11parse.c: secmod_doDescCopy can reallocate our newSpec, but the
                 pointer we are passed is an offset from newSpec. Pass in
                 both pointers and return our newly allocated spec and
                 length in that case.#def113
    cmsutil.c: suppress cppcheck warnings do to cmsutil use of unions to
               cast pointers. #def113-117
    pkcs11.c: support coverity incorrect use_after_free warning. #def118
    
    scanner errors:
    Error: USE_AFTER_FREE (CWE-416): [#def4]
    nss-3.60.1/nss/cmd/crlutil/crlutil.c:389: freed_arg: "PORT_FreeArena_Util" frees "modArena".
    nss-3.60.1/nss/cmd/crlutil/crlutil.c:455: double_free: Calling "PORT_FreeArena_Util" frees pointer "modArena" which has already been freed.
    453| }
    454| if (modArena && (!modCrl || modCrl->arena != modArena)) {
    455|-> PORT_FreeArena(modArena, PR_FALSE);
    456| }
    457| if (modCrl)
    
    Error: BAD_FREE (CWE-590): [#def6]
    nss-3.60.1/nss/cmd/modutil/install-ds.c:1046: address_free: "PR_Free" frees address of "_this->forwardCompatible".
    1044| Pk11Install_PlatformName_delete(&_this->forwardCompatible[i]);
    1045| }
    1046|-> PR_Free(&_this->forwardCompatible);
    1047| _this->numForwardCompatible = 0;
    1048| }
    
    Error: USE_AFTER_FREE (CWE-416): [#def8]
    nss-3.60.1/nss/cmd/signtool/javascript.c:1346: freed_arg: "PR_Free" frees "archiveDir".
    nss-3.60.1/nss/cmd/signtool/javascript.c:1347: pass_freed_arg: Passing freed pointer "archiveDir" as an argument to "PR_smprintf".
    1345| warningCount++;
    1346| PR_Free(archiveDir);
    1347|-> archiveDir = PR_smprintf("%s.arc", archiveDir);
    1348| } else {
    1349| PL_strcpy(archiveDir + strlen(archiveDir) - 4, ".arc");
    
    Error: USE_AFTER_FREE (CWE-416): [#def9]
    nss-3.60.1/nss/cmd/signtool/javascript.c:1477: alias: Assigning: "entityListTail" = "entityItem". Now both point to the same storage.
    nss-3.60.1/nss/cmd/signtool/javascript.c:1477: alias: Assigning: "entityList" = "entityListTail". Now both point to the same storage.
    nss-3.60.1/nss/cmd/signtool/javascript.c:1623: alias: Assigning: "curitem" = "entityList". Now both point to the same storage.
    nss-3.60.1/nss/cmd/signtool/javascript.c:1651: freed_arg: "PR_Free" frees "entityListTail".
    nss-3.60.1/nss/cmd/signtool/javascript.c:1654: double_free: Calling "PR_Free" frees pointer "curitem" which has already been freed.
    1652| }
    1653| if (curitem) {
    1654|-> PR_Free(curitem);
    1655| }
    1656| if (basedir) {
    
    Error: USE_AFTER_FREE (CWE-416): [#def10]
    nss-3.60.1/nss/cmd/signtool/javascript.c:1477: alias: Assigning: "entityListTail" = "entityItem". Now both point to the same storage.
    nss-3.60.1/nss/cmd/signtool/javascript.c:1477: alias: Assigning: "entityList" = "entityListTail". Now both point to the same storage.
    nss-3.60.1/nss/cmd/signtool/javascript.c:1623: alias: Assigning: "curitem" = "entityList". Now both point to the same storage.
    nss-3.60.1/nss/cmd/signtool/javascript.c:1651: freed_arg: "PR_Free" frees "entityListTail".
    nss-3.60.1/nss/cmd/signtool/javascript.c:1654: pass_freed_arg: Passing freed pointer "curitem" as an argument to "PR_Free".
    1652| }
    1653| if (curitem) {
    1654|-> PR_Free(curitem);
    1655| }
    1656| if (basedir) {
    
    Error: RESOURCE_LEAK (CWE-772): [#def12]
    nss-3.60.1/nss/cmd/signtool/list.c:36: alloc_fn: Storage is returned from allocation function "PORT_ZAlloc_Util".
    nss-3.60.1/nss/cmd/signtool/list.c:36: var_assign: Assigning: "ugly_list" = storage returned from "PORT_ZAlloc_Util(16UL)".
    nss-3.60.1/nss/cmd/signtool/list.c:137: leaked_storage: Variable "ugly_list" going out of scope leaks the storage it points to.
    135|
    136| if (failed) {
    137|-> return -1;
    138| }
    139| return 0;
    
    Error: RESOURCE_LEAK (CWE-772): [#def13]
    nss-3.60.1/nss/cmd/signtool/util.c:131: alloc_fn: Storage is returned from allocation function "PR_OpenDir".
    nss-3.60.1/nss/cmd/signtool/util.c:131: var_assign: Assigning: "dir" = storage returned from "PR_OpenDir(path)".
    nss-3.60.1/nss/cmd/signtool/util.c:139: identity_transfer: Passing "dir" as argument 1 to function "PR_ReadDir", which returns an offset off that argument.
    nss-3.60.1/nss/cmd/signtool/util.c:139: noescape: Resource "dir" is not freed or pointed-to in "PR_ReadDir".
    nss-3.60.1/nss/cmd/signtool/util.c:139: var_assign: Assigning: "entry" = storage returned from "PR_ReadDir(dir, PR_SKIP_BOTH)".
    nss-3.60.1/nss/cmd/signtool/util.c:142: leaked_storage: Variable "entry" going out of scope leaks the storage it points to.
    nss-3.60.1/nss/cmd/signtool/util.c:142: leaked_storage: Variable "dir" going out of scope leaks the storage it points to.
    140| if (snprintf(filename, sizeof(filename), "%s/%s", path, entry->name) >= sizeof(filename)) {
    141| errorCount++;
    142|-> return -1;
    143| }
    144| if (rm_dash_r(filename))
    
    Error: RESOURCE_LEAK (CWE-772): [#def14]
    nss-3.60.1/nss/cmd/signtool/util.c:131: alloc_fn: Storage is returned from allocation function "PR_OpenDir".
    nss-3.60.1/nss/cmd/signtool/util.c:131: var_assign: Assigning: "dir" = storage returned from "PR_OpenDir(path)".
    nss-3.60.1/nss/cmd/signtool/util.c:139: identity_transfer: Passing "dir" as argument 1 to function "PR_ReadDir", which returns an offset off that argument.
    nss-3.60.1/nss/cmd/signtool/util.c:139: noescape: Resource "dir" is not freed or pointed-to in "PR_ReadDir".
    nss-3.60.1/nss/cmd/signtool/util.c:139: var_assign: Assigning: "entry" = storage returned from "PR_ReadDir(dir, PR_SKIP_BOTH)".
    nss-3.60.1/nss/cmd/signtool/util.c:145: leaked_storage: Variable "entry" going out of scope leaks the storage it points to.
    nss-3.60.1/nss/cmd/signtool/util.c:145: leaked_storage: Variable "dir" going out of scope leaks the storage it points to.
    143| }
    144| if (rm_dash_r(filename))
    145|-> return -1;
    146| }
    147|
    
    Error: RESOURCE_LEAK (CWE-772): [#def15]
    nss-3.60.1/nss/cmd/signver/pk7print.c:325: alloc_fn: Storage is returned from allocation function "PORT_ZAlloc_Util".
    nss-3.60.1/nss/cmd/signver/pk7print.c:325: var_assign: Assigning: "pk" = storage returned from "PORT_ZAlloc_Util(328UL)".
    nss-3.60.1/nss/cmd/signver/pk7print.c:351: leaked_storage: Variable "pk" going out of scope leaks the storage it points to.
    349| default:
    350| fprintf(out, "%s=bad SPKI algorithm type\n", msg);
    351|-> return 0;
    352| }
    353|
    
    Error: RESOURCE_LEAK (CWE-772): [#def16]
    nss-3.60.1/nss/cmd/symkeyutil/symkeyutil.c:289: alloc_fn: Storage is returned from allocation function "PK11_GetSymKeyNickname".
    nss-3.60.1/nss/cmd/symkeyutil/symkeyutil.c:289: var_assign: Assigning: "name" = storage returned from "PK11_GetSymKeyNickname(symKey)".
    nss-3.60.1/nss/cmd/symkeyutil/symkeyutil.c:298: noescape: Resource "name ? name : " "" is not freed or pointed-to in "printf". [Note: The source code implementation of the function has been overridden by a builtin model.]
    nss-3.60.1/nss/cmd/symkeyutil/symkeyutil.c:306: leaked_storage: Variable "name" going out of scope leaks the storage it points to.
    304| }
    305| printf("\n");
    306|-> }
    307|
    308| SECStatus
    
    Error: USE_AFTER_FREE (CWE-416): [#def99]
    nss-3.60.1/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_nameconstraints.c:835: freed_arg: "PORT_FreeArena_Util" frees "arena".
    nss-3.60.1/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_nameconstraints.c:854: double_free: Calling "PORT_FreeArena_Util" frees pointer "arena" which has already been freed.
    852| PKIX_CERTNAMECONSTRAINTS_DEBUG
    853| ("\t\tCalling PORT_FreeArena).\n");
    854|-> PORT_FreeArena(arena, PR_FALSE);
    855| }
    856| }
    
    Error: VARARGS (CWE-237): [#def109]
    nss-3.60.1/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_string.c:428: va_init: Initializing va_list "args".
    nss-3.60.1/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_string.c:534: missing_va_end: "va_end" was not called for "args".
    532| }
    533|
    534|-> PKIX_RETURN(STRING);
    535| }
    536|
    
    Error: USE_AFTER_FREE (CWE-416): [#def112]
    nss-3.60.1/nss/lib/pk11wrap/pk11pars.c:1099: alias: Assigning: "newSpecPtr" = "newSpec". Now both point to the same storage.
    nss-3.60.1/nss/lib/pk11wrap/pk11pars.c:1156: freed_arg: "secmod_doDescCopy" frees "newSpecPtr".
    nss-3.60.1/nss/lib/pk11wrap/pk11pars.c:1211: use_after_free: Using freed pointer "newSpec".
    1209| /* no target found, return the newSpec */
    1210| if (target == NULL) {
    1211|-> return newSpec;
    1212| }
    1213|
    
    Error: CPPCHECK_WARNING (CWE-562): [#def113]
    nss-3.60.1/nss/lib/smime/cmsutil.c:309: error[returnDanglingLifetime]: Returning pointer to local variable 'digestedData' that will be invalid when returning.
    307| }
    308| }
    309|-> return cinfo;
    310| }
    311|
    
    Error: CPPCHECK_WARNING (CWE-562): [#def114]
    nss-3.60.1/nss/lib/smime/cmsutil.c:309: error[returnDanglingLifetime]: Returning pointer to local variable 'encryptedData' that will be invalid when returning.
    307| }
    308| }
    309|-> return cinfo;
    310| }
    311|
    
    Error: CPPCHECK_WARNING (CWE-562): [#def115]
    nss-3.60.1/nss/lib/smime/cmsutil.c:309: error[returnDanglingLifetime]: Returning pointer to local variable 'envelopedData' that will be invalid when returning.
    307| }
    308| }
    309|-> return cinfo;
    310| }
    311|
    
    Error: CPPCHECK_WARNING (CWE-562): [#def116]
    nss-3.60.1/nss/lib/smime/cmsutil.c:309: error[returnDanglingLifetime]: Returning pointer to local variable 'genericData' that will be invalid when returning.
    307| }
    308| }
    309|-> return cinfo;
    310| }
    311|
    
    Error: CPPCHECK_WARNING (CWE-562): [#def117]
    nss-3.60.1/nss/lib/smime/cmsutil.c:309: error[returnDanglingLifetime]: Returning pointer to local variable 'signedData' that will be invalid when returning.
    307| }
    308| }
    309|-> return cinfo;
    310| }
    311|
    
    Error: USE_AFTER_FREE (CWE-416): [#def118]
    nss-3.60.1/nss/lib/softoken/pkcs11.c:2671: freed_arg: "PORT_Realloc_Util" frees "oldNscSlotList".
    nss-3.60.1/nss/lib/softoken/pkcs11.c:2674: use_after_free: Using freed pointer "oldNscSlotList".
    2672| nscSlotListSize[index] * sizeof(CK_SLOT_ID));
    2673| if (nscSlotList[index] == NULL) {
    2674|-> nscSlotList[index] = oldNscSlotList;
    2675| nscSlotListSize[index] = oldNscSlotListSize;
    2676| return CKR_HOST_MEMORY;
    
    Some of these are false positives, but they are reasonable issues for the scanners to flag, so scanner suppression comments, along with human reviewer comments will be added.
    
    Differential Revision: https://phabricator.services.mozilla.com/D111339
    
    --HG--
    extra : rebase_source : 45ad7590affb7fdb8a0e182e29c6c88f909ff260
    350807b3
Name
Last commit
Last update
automation Loading commit data...
cmd Loading commit data...
coreconf Loading commit data...
cpputil Loading commit data...
doc Loading commit data...
fuzz Loading commit data...
gtests Loading commit data...
lib Loading commit data...
nss/automation/abi-check Loading commit data...
nss-tool Loading commit data...
pkg Loading commit data...
tests Loading commit data...
.arcconfig Loading commit data...
.clang-format Loading commit data...
.gitignore Loading commit data...
.hgignore Loading commit data...
.hgtags Loading commit data...
.sancov-blacklist Loading commit data...
.taskcluster.yml Loading commit data...
COPYING Loading commit data...
Makefile Loading commit data...
build.sh Loading commit data...
exports.gyp Loading commit data...
help.txt Loading commit data...
mach Loading commit data...
manifest.mn Loading commit data...
nss.gyp Loading commit data...
readme.md Loading commit data...
trademarks.txt Loading commit data...