1280844 - Fix null-check-after-deref issues found in NSS with µchex

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nicholas Nethercote [inactive]]

µchex is a static analysis tool described in this paper:

  How to Build Static Checking Systems Using Orders of Magnitude Less Code
  Fraser Brown, Andres Nötzli, Dawson Engler
  ASPLOS '16

I've been in contact with the authors and they've given me access to the result of an up-to-date run of the null-check-after-deref issues found in NSS.

There are two categories of problem:

- Redundant checks. It would be safe to leave these in the code, but I have
  removed them all.

- Missing/bad checks. These are actively dangerous.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Fix up null checks in cmd/. .

This patch:
- Adds some missing null checks after allocations.
- Moves some derefs of pointers after null checks of those same pointers.
- Removes some redundant null checks for pointers that are guaranteed to be
  non-null.

Review commit: https://reviewboard.mozilla.org/r/59582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59582/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Remove useless assert() in pathsub.c. .

The assert() assumes that |from| and |to| are non-null, which the subsequent
check does not assume. It's also redundant w.r.t. the subsequent checks, which
will cause informative termination on failure. It's also the only assert() in
this file.

Review commit: https://reviewboard.mozilla.org/r/59584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59584/

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Fix a null check in parse.c. .

This patch fixes things so that we don't deref file before checking it.

Review commit: https://reviewboard.mozilla.org/r/59586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59586/

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Remove some redundant null checks in lib/. .

Review commit: https://reviewboard.mozilla.org/r/59588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59588/

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Move some null checks earlier in lib/. .

Review commit: https://reviewboard.mozilla.org/r/59590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59590/

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Remove some more redundant null checks in lib/. .

Review commit: https://reviewboard.mozilla.org/r/59592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59592/

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Remove even more redundant null checks in lib/. .

Review commit: https://reviewboard.mozilla.org/r/59594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59594/

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Fix a null check in p12d.c. .

Review commit: https://reviewboard.mozilla.org/r/59596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59596/

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Bug 1280844 - Remove still more redundant null checks in lib/. .

Review commit: https://reviewboard.mozilla.org/r/59598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59598/

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8763422 [details]
Bug 1280844 - Fix a null check in parse.c. .

https://reviewboard.mozilla.org/r/59586/#review56682

Not sure if this is even compiled...

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8763420 [details]
Bug 1280844 - Fix up null checks in cmd/. .

https://reviewboard.mozilla.org/r/59582/#review56684

::: security/nss/cmd/certcgi/certcgi.c
(Diff revision 1)
> -    if (arena) {
> -        PORT_FreeArena(arena, PR_FALSE);
> +    PORT_FreeArena(arena, PR_FALSE);
> -    }

The general pattern in NSS is to have a redundant check here, but I guess that blindly following the rules is not necessary.

::: security/nss/cmd/crlutil/crlgen.c:520
(Diff revision 1)
> +    extHandle = crlGenData->crlExtHandle;
>      rv = CERT_AddExtension(extHandle, SEC_OID_X509_CRL_NUMBER, &encodedItem,
>                             (*dataArr[1] == '1') ? PR_TRUE : PR_FALSE,
>                             PR_TRUE);

Maybe just remove the temporary instead.

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8763421 [details]
Bug 1280844 - Remove useless assert() in pathsub.c. .

https://reviewboard.mozilla.org/r/59584/#review56686

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 8763423 [details]
Bug 1280844 - Remove some redundant null checks in lib/. .

https://reviewboard.mozilla.org/r/59588/#review56688

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8763424 [details]
Bug 1280844 - Move some null checks earlier in lib/. .

https://reviewboard.mozilla.org/r/59590/#review56690

::: security/nss/lib/ssl/sslmutex.c:414
(Diff revision 1)
> -    if (!pMutex || (hMutex = pMutex->u.sslMutx) == 0 ||
> +    if ((hMutex = pMutex->u.sslMutx) == 0 || hMutex == INVALID_HANDLE_VALUE) {
> -        hMutex == INVALID_HANDLE_VALUE) {

Isn't this change sufficient?  This isn't a public function, and we have an assert that pMutex is non-null.

Comment 15

[Martin Thomson [:mt:]]

Comment on attachment 8763425 [details]
Bug 1280844 - Remove some more redundant null checks in lib/. .

https://reviewboard.mozilla.org/r/59592/#review56692

Comment 16

[Nicholas Nethercote [inactive]]

https://reviewboard.mozilla.org/r/59590/#review56690

> Isn't this change sufficient?  This isn't a public function, and we have an assert that pMutex is non-null.

Probably, but NSS is very cautious with argument handling and this kind of double checking (assert non-null and then return if null) is prevalent, so I stuck with it.

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8763420 [details]
Bug 1280844 - Fix up null checks in cmd/. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59582/diff/1-2/

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8763421 [details]
Bug 1280844 - Remove useless assert() in pathsub.c. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59584/diff/1-2/

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8763422 [details]
Bug 1280844 - Fix a null check in parse.c. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59586/diff/1-2/

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8763423 [details]
Bug 1280844 - Remove some redundant null checks in lib/. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59588/diff/1-2/

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8763424 [details]
Bug 1280844 - Move some null checks earlier in lib/. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59590/diff/1-2/

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8763425 [details]
Bug 1280844 - Remove some more redundant null checks in lib/. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59592/diff/1-2/

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 8763426 [details]
Bug 1280844 - Remove even more redundant null checks in lib/. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59594/diff/1-2/

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8763427 [details]
Bug 1280844 - Fix a null check in p12d.c. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59596/diff/1-2/

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 8763428 [details]
Bug 1280844 - Remove still more redundant null checks in lib/. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59598/diff/1-2/

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8763426 [details]
Bug 1280844 - Remove even more redundant null checks in lib/. .

https://reviewboard.mozilla.org/r/59594/#review56818

Comment 27

[Martin Thomson [:mt:]]

Comment on attachment 8763427 [details]
Bug 1280844 - Fix a null check in p12d.c. .

https://reviewboard.mozilla.org/r/59596/#review56820

::: security/nss/lib/pkcs12/p12d.c:1619
(Diff revision 2)
>      if(!bag->attribs) {
>  	return SECFailure;
>      }
>  
>      bag->attribs[i] = PORT_ArenaZNew(bag->arena, sec_PKCS12Attribute);
> -    if(!bag->attribs) {
> +    if(!bag->attribs[i]) {

Nice.

Comment 28

[Martin Thomson [:mt:]]

Comment on attachment 8763428 [details]
Bug 1280844 - Remove still more redundant null checks in lib/. .

https://reviewboard.mozilla.org/r/59598/#review56822

Comment 29

[Martin Thomson [:mt:]]

Comment on attachment 8763424 [details]
Bug 1280844 - Move some null checks earlier in lib/. .

https://reviewboard.mozilla.org/r/59590/#review56824

Excess paranoia is harmless I guess.

Comment 30

[Martin Thomson [:mt:]]

remote:   https://hg.mozilla.org/projects/nss/rev/a534095c4347
remote:   https://hg.mozilla.org/projects/nss/rev/d46ba3e226e1
remote:   https://hg.mozilla.org/projects/nss/rev/1d90aaeb3df8
remote:   https://hg.mozilla.org/projects/nss/rev/839ebb15e92f
remote:   https://hg.mozilla.org/projects/nss/rev/89779ff5d079
remote:   https://hg.mozilla.org/projects/nss/rev/cbb357c8e651
remote:   https://hg.mozilla.org/projects/nss/rev/5afcbd7d80f8
remote:   https://hg.mozilla.org/projects/nss/rev/e8aeeeec31c0
remote:   https://hg.mozilla.org/projects/nss/rev/f01aa49c7629