1542077 - mp_set_ulong and mp_set_int should return errors on bad values

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

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.

Comment 1

[Marcus Burghardt]

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

Comment 2

[Marcus Burghardt]

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.

Comment 3

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

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.

Comment 4

[Marcus Burghardt]

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.

Comment 5

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

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

Comment 6

[Marcus Burghardt]

Attachment: Bug 1542077 - Added extra controls and tests to mp_set_int and mp_set_ulong. r=jcj

Comment 7

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

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.

Comment 8

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

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

Comment 9

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

https://hg.mozilla.org/projects/nss/rev/9bc47e69613e9ee9c8aaaf555150f815fc8161d9