1227096 - [Static Analysis][Division by zero] Function PQG_VerifyParams from lib/freebl/pqg.c can potentially cause a division by 0

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Andi [:andi]]

The Static Analysis tool Scan-Build added a division by 0 error on variable outlen, it's vallue is set to 0 when hashtype is inconsistent.

Comment 1

[Andi [:andi]]

Attachment: Bug 1227096.diff

Comment 2

[Andi [:andi]]

Comment on attachment 8690787 [details] [diff] [review]
Bug 1227096.diff

Hello Robert,

Can you please take a look other this patch?

THX

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8690787 [details] [diff] [review]
Bug 1227096.diff

Review of attachment 8690787 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/nss/lib/freebl/pqg.c
@@ +1743,5 @@
>  	/* 10. P generated from (L, counter, g, SEED, Q) matches P 
>  	 * in PQGParams. */
>  	outlen = HASH_ResultLen(hashtype)*PR_BITS_PER_BYTE;
> +
> +	if (outlen<=0) 

Only in theory can this be zero, `findQfromSeed()` would fail if we don't find a supported hash type. Maybe add an assertion?

Comment 4

[Andi [:andi]]

Attachment: Bug 1227096.patch

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 8907543 [details] [diff] [review]
Bug 1227096.patch

I had added r+ in https://phabricator.services.mozilla.com/D25746

Comment 6

[Kai Engert (:KaiE:)]

I wonder why I didn't see a link to phabricator before my comment 5

Comment 7

[J.C. Jones [:jcj] (he/him)]

Kaie - this is a different patch, in a different function, than Bug 1227090.

Can you double-check it? I haven't run a try run on it, since it's a splinter patch. (Andi, any chance of reposting in phab?)

Comment 8

[Andi [:andi]]

Attachment: Bug 1227096 - assert if outlen is greater than 0. r=kaie

Comment 9

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/ec6843bc0ce7a4d30150ef65e137c3a1654198a0