1697303 - NSS needs to update it's csp clearing to FIPS 180-3 standards.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Robert Relyea]

FIPS 180-3 updated the standard for clearing sensitive key material in FIPS modules. I've done a complete review of the portions of NSS affected by the FIPS requirements and identified all the areas where we need to update. The report is available here: https://docs.google.com/document/d/1v9kedUiwVYYIUagyT_vQdtrktjGUrA3SFsVP-LA6vOw/edit?usp=sharing

The report is 120 pages, but the summary of changes are included below:
As part of the FIP-140-3 changes, there are new requirements for clearing CPS data. First, we are now required to clear public key data in many instances, primarily to prevent using previously trusted public keys later because they are still in memory. Second, we need to clear certain intermediate values. This review tries to identify all the places we may possible need to clear data. This review was maximal.. that is it took a conservative approach to what has to be cleared and includes stack variables that may hold intermediate values. It also indicated values that it wasn’t clear if they were csp intermediate values or not. Even single bit leaks were identified if found. Things that were explictly not marked as csp data included length values and error returns. Sometimes those values can be used in oracle attacks, but the presumption is that we aren’t trying to defend those side channels.

Many of the stack variables identified are probably overkill, so a final recommendation for each file is included. Variables in mpi, particularly array variables on the stack, or places where lots of variables are on the stack should get more focus as they could be deep stack variables with lots of values. Other places it’s probably not as critical, especially if there are only a few variables, they will likely be placed in registers by the compiler. Same thing with pass by value arguments. Usually they would be in registers anyway. In some cases it’s not actually possible to clear the variable because we are actually returning it back to the caller. Finally some variables are actually returned by the function. They shouldn’t be cleared. In general, only large stack frames may be a problem. Most notiable are the machine generated ECC functions which have stack frames of almost 1k. A mitigating solution would be to call a function with a very large stack frame after we’ve finished all out ECC calculations and before we return from the base function. That function would clear it’s very large stack and then return.

This review is done by directory, then file, then function, then variable. All offending variables are marked in red. Functions with offending variables are marked in red, and files with offending functions are marked in red. Only those functions and files which Red Hat needs has been reviewed, so no attempt has been made to rewiew windows only, or solaris only changes (for instance). We may decide to ignore arm and ppc related changes since those are not part of our FIPS validation currently.

Some notes:
mpi_int values are structures that hold allocated values. When mpi_ints are on the stack, we know that they are properly cleared because if they weren’t we would have memory leaks. All mpi_ints are zeroed before they are freed by the mp_clear command, so most of the data in mpi is already properly cleared, and only drips and drabs based on parts of value can find it’s way onto the stack uncleared.
......
nss/llib/freebl
1. aes-armv8.c - ARM aes hardware accellerator Recommendation: stack is no more that 13x16 bytes, if we care, clear the stack in the base gcm call before returning.
3. aeskeywrap.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
4. aes-x86.c – AESNI support Recommendation: stack only small stack variables, likely to be registers anyway, No action
25. cmac.c - Recommendation: v is only one byte, so not a significant leak, likely to be a register anyway, No action
28. crypto_primitives.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
30. ctr.c Recommendation: fullblocks is a length, not a csp, No action
40. dh.c -Recommendation: Fix the failure to clear on error paths.
41. drbg.c Recommendation: Fix ctx, H, and hash clearing
42. dsa.c Recommendation: Fix these errors
43. ec.c Recommendation: Fix these errors
56. gcm-aarch64.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
57. gcm-arm32-neon.c Recommendation: moderate stack on arm32. If we care, we should fix with a stack clear in gcm base code, No action
58. gcm.c Recommendation: Fix the ghash, H, ctrparams and clearing in the error path. Bmul has a pretty big stack. Another point in favor of clearing the stack on gcm operations.
61. gcm-x86.c Recommendation: stack only small stack variables, maybe clear tmp_out, which is a small array, otherwise No action
63. hmacct.c – ssl/tls mac Recommendation: clear these arrays.
72. intel-gcm-wrap.c Recommendation: small stack arrays, clear if we don’t do a stack smash in gcm proper
90. pqg.c Recommendation: Fix these errors
95. rijndael.c Recommendation: only small stack variables, likely to be registers anyway, exception: inBuf and outBuf which are small stack arrays. This is where we would smash the stack if we wanted to protect against some of the other aes machine dependent leakage otherwise: No action
98. rsa.c Recommendation: stack only small stack variables, likely to be registers anyway, . Exceptions: clear the arena on error below, clear the modulus on cleanup.
99. rsapkcs.c Recommendation: stack only small stack variables, likely to be registers anyway, No action, Exeption: clear the block on error in Format OneBlock before returning to caller.
103. sha1-armv8.c Recommendation: stack only small stack variables. If we care about arm, we can clear the stack in SHA256_Update. No action
106. sha512.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
109. sha_fast.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
110. sha_fast.h Recommendation: stack only small stack variables, likely to be registers anyway, No action
112. shvfy.c Recommendation: Fix the identified errors
118. unix_urandom.c Recommendation: Fix the identified error
nss/lib/freebl/mpi
15. mp_comba.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
19. mp_gf2m.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
22. mpi_amd64.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
27. mpi.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
39. mplogic.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
41. mpmontg.c Recommendation: stack only small stack variables, likely to be registers. Except the PowersArray. Clear the powers array
nss/lib/freebl/ecl
14. ecp_256.c Recommendation: stack only small stack variables, possibly smash the stack in points multiply
15. ecp_384.c Recommendation: clear s
16. ecp_521.c Recommendation: clear si
19. ecp_jac.c Recommendation: stack only small stack variables, likely to be registers anyway, No action
20. ecp_jm.c Recommendation: clear naf when freeing
22. ecp_secp384r1.c – auto generated code from FIAT Recommendation: large stack based not cleared: handle with a stack smash in points mult code
23. ecp_secp521r1.c – auto generated code from FIAT Recommendation: large stack based not cleared: handle with a stack smash in points mult code
nss/lib/softoken
5. fipstokn.c - Recommendation: stack only small stack variables, likely to be registers anyway, No action
11. lowkey.c - Recommendation: clear identified public key objects
14. lowpbe.c - Recommendation: clear identified uncleared objects
19. pkcs11.c Recommendation: clear identified uncleared objects
20. pkcs11c.c Recommendation: clear identified uncleared objects, fix AEOP and PSS not copying required data
23. pkcs11u.c - Recommendation: fix identfied objects to clear
26. sftkdb.c – Recommendation: fix identified objects to cleare
10. sftk_ike_prf - Recommendation: fix newInKey case, verify that test vectors don’t need to be cleared
36. sftkpwd.c - Recommendation: Verify signatures do not need to be cleared. Fix all other identified issues

Comment 1

[Benjamin Beurdouche [:beurdouche]]

Bob, is this blocking for FIPS certifications? Or are we aiming at improving our code?

Comment 2

[Robert Relyea]

Attachment: Bug 1697303 NSS needs to update it's csp clearing to FIPS 180-3 standards.

FIPS 180-3 updated the standard for clearing sensitive key material in FIPS modules. I've done a complete review of the portions of NSS affected by the FIPS requirements and identified all the areas where we need to update. The report is available here: https://docs.google.com/document/d/1v9kedUiwVYYIUagyT_vQdtrktjGUrA3SFsVP-LA6vOw/edit?usp=sharing

This patch does the following:
- Clears the stack in gcm and ecc to deal with large stack leakages.
This only happens in FIPS enabled case. The size of the stack is based on the
size of the leakage, with some extra to make sure we reach down into that area.
Most of the leakage happens in either auto generated code or machine dependent
acceleration code.
- Clears hash related data that wasn't cleared previously
- Clears public key exponents that wasn't cleared previously.
- Clears components that should have been cleared previously but wasn't.

Usually clearing takes one of the following forms:
PORT_Free(x) -> PORT_Free(x, size). This means we need to know what
the size is supposed to be. In some cases we need to add code to preserve
the size.
PORT_Free(x.data) -> SECITEM_ZfreeItem(&x, PR_FALSE). In this case x is
a SECITEM, which carries the length. PR_FALSE means clear and free the data in
the item, not the item itself. The code should have had SECITEM_FreeItem before
anyway.
SECIEM_FreeItem(item, bool) -> SECITEM_ZfreeItem(item, bool). Simply change
the normal SECITEM free call to the one that clears the item.
PR_ArenaFree(arena, PR_FALSE) -> PR_ArenaFree(arena, PR_TRUE). The bool here
means whether or not to clear as well as free the data in the arena.
PORT_Memset(value, 0, size). This the obvious clear operation. It happens
if the variable is a stack variable, or if the memory isn't cleared with one
of the three clearing functions above.

In addition this patch fixes the following:
- moves the determination if whether or not a slot is in FIPS mode by
slotID to a macro. NSS allows user defined slots to be opened. If you open a
user defined slot using the FIPS slot, the resulting slots will also be FIPS
slots. In areas where the semantics change based on the slot, these slots should
have the FIPS semantics. Directly checking if the slot is the FIPS slot now
only happens when we really mean the main FIPS slot and not just any FIPS slot.
- In handling the clearing of PSS and OAEP, I identified an issue. These
functions where holding a pointer to the pMechanismParams in their C_XXXXInit
calls for later use in the C_XXXXUpdate/C_XXXXFinal/C_XXXX calls. The problem
is applications are allowed to free their pMechanismParams once C_XXXXInit is
complete. We need to make a copy of the params to use them.

Comment 3

[Benjamin Beurdouche [:beurdouche]]

Heads up to @karthik @ekr @mt. These changes will affect our wip patches for NIST primitives.
I agree that clearing those values is better anyway, but for verification, we will have to move the entire HACL* code base to salloc before we can do any uplift to NSS in the future. So this is a lot of work.

Comment 4

[Benjamin Beurdouche [:beurdouche]]

@Bob, can you confirm that this is absolutely needed by RH?
(We can block uplifting of verified primitives that don’t clear intermediate values, if needed, but I am not sure that would be a net win for security)
An intermediate solution could be to keep two versions, one for FIPS builds with CSP clearing, one verified without it, until the verified code does it systematically.

Comment 5

[Robert Relyea]

Yup, it's new FIPS-180-3 requirements. We've always had to clear CSPs (so some of these are CSPs that were missed before), but 180-3 requires clearing intermediate results as well as public key clearing).

For some cases, we can handle it by clearing the stack after the operation. For instance, our constant time ECC code has machine generated code in it, and had big chunks of code with several k of intermediate data on the stack. I simply cleared a big chunk of stack in the higher level ECC code to handle this.

bob

Comment 6

[Benjamin Beurdouche [:beurdouche]]

https://hg.mozilla.org/projects/nss/rev/be386bdafeb8dcfd894af7ff151b04afe748857a