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

RESOLVED FIXED in 3.45

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
17 days ago

People

(Reporter: andi, Assigned: andi)

Tracking

({good-first-bug})

Firefox Tracking Flags

(firefox45 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch Bug 1227096.diff (obsolete) — Splinter Review
Component: Security → Libraries
Product: Core → NSS
Version: Trunk → trunk
Comment on attachment 8690787 [details] [diff] [review]
Bug 1227096.diff

Hello Robert,

Can you please take a look other this patch?

THX
Attachment #8690787 - Flags: review?(rrelyea)
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?
Attachment #8690787 - Flags: review?(rrelyea) → review-
Priority: -- → P3
Summary: [Static Analysis][Division by zero] Function PQG_VerifyParams from security/nss/lib/freebl/pqg.c can potentially cause a division by 0 → [Static Analysis][Division by zero] Function PQG_VerifyParams from lib/freebl/pqg.c can potentially cause a division by 0
Posted patch Bug 1227096.patch (obsolete) — Splinter Review
Attachment #8690787 - Attachment is obsolete: true
Attachment #8907543 - Flags: review?(ttaubert)
Attachment #8907543 - Flags: review?(ttaubert) → review?(kaie)

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

QA Contact: jjones
Keywords: checkin-needed

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?)

Flags: needinfo?(kaie)
Attachment #8907543 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.45
Flags: needinfo?(kaie)
You need to log in before you can comment on or make changes to this bug.