1605314 - mp_comba_amd64_masm.asm compatibility issue with 8B mp_digits

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kevin Jacobs [:kjacobs]]

Attachment: test.patch

mp_comba_amd64_masm.asm does clamping (removing leading zero digits) incorrectly when an mp_digit is 64 bits. This is exhibited on Windows x64.

https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/mpi/mp_comba_amd64_masm.asm#7993,8003 doesn't use qword ptr, and thus only checks the lower 32 bits.

Reproducer attached.

The fix is straightforward (changing the highlighted lines fixes the test), but I don't have a full patch ready yet. I'm also curious if this asm version has enough performance benefit to warrant the maintainability cost...

Comment 1

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1605314 - Compare all 8 bytes of an mp_digit when clamping in Windows assembly/mp_comba.

Compare all 8 bytes of an mp_digit when clamping in Windows x64 assembly (mp_sqr/mp_mul). Also adds an assertion to ensure that the size of mp_digit matches implementation assumptions.

Comment 2

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

Kevin, can you take a stab at security classification when you get a chance? Thanks.

Comment 3

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1605314 - Additional tests for mp_comba

Verify that when clamping, the upper DWORD of an mp_digit is checked.

Comment 4

[Kevin Jacobs [:kjacobs]]

Running trials in ecperf, the assembly implementation seems to be 1-4% faster (depending on the curve) and in rsaperf, more like a 40% (vs. the non-comba path [0]). So, we probably want to keep this around.

For classification, I'm going to initially call this sec-moderate primarily due to the difficulty of exploiting it (and FWIW, this seems to align with classifications for similar bugs in other libraries). However, the complexity and fact that it can be triggered through many primitives[1,2] and via mp_mul/mp_sqr makes classification difficult, so I'm open to other opinions.

[0] https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/mpi/mpi.c#963-984
[1] https://searchfox.org/mozilla-central/search?q=symbol:mp_exptmod&redirect=false
[2] https://searchfox.org/mozilla-central/search?q=symbol:mpp_pprime&redirect=false

Comment 5

[Martin Thomson [:mt:]]

With blinding, this might be even less relevant. Still it's a good fix to have. It's also another reminder that MPI is a danger to us all.

Comment 6

[Kevin Jacobs [:kjacobs]]

https://hg.mozilla.org/projects/nss/rev/09673f933c6df7dd676982d9d89b85aa6517c289

Comment 7

[Phabricator Automation]

Comment on attachment 9118439 [details]
Bug 1605314 - Additional tests for mp_comba

Revision D58576 was moved to bug 1609751. Setting attachment 9118439 [details] to obsolete.