[Static Analysis][Division by zero] Function makePfromQandSeed from lib/freebl/pqg.c could lead to a division by 0

RESOLVED FIXED in 3.45

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
28 days ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks 1 bug, {clang-analyzer, good-first-bug})

Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The Static Analysis tool Scan-Build added that variable outlen from makePfromQandSeed could lead to a division by 0 if HASH_ResultLen gets called with improper parameters.
Assignee

Comment 1

4 years ago
Posted patch Bug 1227090.diff (obsolete) — Splinter Review
Assignee

Updated

4 years ago
Summary: [Static Analysis][Logic error] Function makePfromQandSeed from security/nss/lib/freebl/pqg.c could lead to a division by 0 → [Static Analysis][Division by zero] Function makePfromQandSeed from security/nss/lib/freebl/pqg.c could lead to a division by 0
Assignee

Comment 2

4 years ago
Posted patch Bug 1227090.diff (obsolete) — Splinter Review
Updated previous path in order to log the error with invalid argument function call.
Component: Security → Libraries
Product: Core → NSS
Version: Trunk → trunk
Assignee

Comment 3

4 years ago
Posted patch Bug 1227090.diff (obsolete) — Splinter Review
Hello Robert,

Could you please take a look other this patch?

THX
Attachment #8690780 - Attachment is obsolete: true
Attachment #8690782 - Attachment is obsolete: true
Attachment #8696423 - Flags: review?(rrelyea)
Keywords: good-first-bug
Priority: -- → P3
Summary: [Static Analysis][Division by zero] Function makePfromQandSeed from security/nss/lib/freebl/pqg.c could lead to a division by 0 → [Static Analysis][Division by zero] Function makePfromQandSeed from lib/freebl/pqg.c could lead to a division by 0
Comment on attachment 8696423 [details] [diff] [review]
Bug 1227090.diff

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

::: security/nss/lib/freebl/pqg.c
@@ +1009,5 @@
>      CHECK_MPI_OK( mp_init(&V_n)  );
>  
>      hashlen = HASH_ResultLen(hashtype);
>      outlen = hashlen*PR_BITS_PER_BYTE;
> +    if (outlen <= 0)

This should rather be an assertion. There's no way that `makePfromQandSeed()` is passed an invalid hash type.
Attachment #8696423 - Flags: review?(rrelyea) → review-
Assignee

Comment 5

2 years ago
Posted patch Bug 1227090.patch (obsolete) — Splinter Review
Attachment #8907539 - Flags: review?(ttaubert)
Assignee

Updated

2 years ago
Attachment #8696423 - Attachment is obsolete: true

:andi Tim is gone, maybe ask some else to review these?

Flags: needinfo?(bpostelnicu)
QA Contact: jjones
Assignee

Updated

3 months ago
Attachment #8907539 - Attachment is obsolete: true
Flags: needinfo?(bpostelnicu)
Attachment #8907539 - Flags: review?(ttaubert)
Assignee

Updated

Last month
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: Last month
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.45
You need to log in before you can comment on or make changes to this bug.