Closed Bug 1542077 Opened 7 months ago Closed 2 months ago

mp_set_ulong and mp_set_int should return errors on bad values

Categories

(NSS :: Libraries, defect, P2)

3.44
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jcj, Assigned: marcus.apb)

References

Details

(Keywords: csectype-nullptr, sec-audit)

Attachments

(1 file)

mp_set_ulong and mp_set_int (at least) currently return MP_OKAY no matter what madness they are commanded to work with. They seem to handle the madness fine internally, but is that something we can clean up without breaking other apps?

Let's analyze and consider that.

I can create some PoC and do a deeper analysis to fundament the answer for this doubt.

Assignee: nobody → marcus.apb
Status: NEW → ASSIGNED
Keywords: wsec-crypto
Priority: P3 → P2

Those functions are very simple and to cause an error with the values received, I believe only if some error in code be done.
In this case, the dependent functions or our current gtests will catch.

However, those functions expect to receive an initialized MP and don't test this appropriately.
I could cause some errors sending invalid MPs for them.

We have to implement more controls here.

Group: crypto-core-security

It looks to me like mp_set_ulong does check that mt is not null, and mp_set_int calls mp_set_ulong right away (and propagates its' err).

I do see that nothing checks that MP_DIGITS(mp) is non-null, though.

I haven't found anything that would be an erroneous code path to these yet, but I've only been looking in a glancing fashion. Do you know of one? Otherwise this is probably not a security bug and we can unhide it. If we do find a path, then this would be a sec-audit bug.

No I didn't find any error code. This hypothesis was if some future change happen. But, as I mentioned before, we can avoid it naturally during the review process.

This check of MP null I will review during the implementation of this extra controls, as I received SEGFAULT when sent a not initialized mp for them.
Ok. We can unhide it. : )

Thanks J.C.

Let's keep it hidden over the weekend to be safe.

We don't see any viable exploit paths here.

As a note, we determined that we can't really change the behavior of these functions, but in auditing we wanted to improve the behavior of other functions, and improve the chances of catching future correctness issues.

Un-hiding, per comment 5 and in-review discussion.

Group: crypto-core-security
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.46
You need to log in before you can comment on or make changes to this bug.