Explicitly zero potentially sensitive data in software crypto and cxgbe.
ClosedPublic
Actions

Authored by jhb on May 29 2020, 9:51 PM.

Details

Reviewers

delphij
jmg

Commits

rS361777: Explicitly zero IVs on the stack.
rS361776: Explicitly zero AES key schedules on the stack.
rS361774: Explicitly zero on-stack IVs, tags, and HMAC keys.
rS361773: Add explicit bzero's of sensitive data in software crypto consumers.

Summary

Add explicit bzero's of sensitive data in software crypto consumers.

Explicitly zero IVs, block buffers, and hashes/digests.

Explicitly zero on-stack IVs, tags, and HMAC keys.

Explicitly AES zero key schedules on the stack.

Explicitly zero IVs on the stack.

Test Plan

tested with KTLS sending (uses AES-GCM) and cryptocheck

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jhb created this revision.May 29 2020, 9:51 PM

Herald added a subscriber: ae. · View Herald TranscriptMay 29 2020, 9:51 PM

Harbormaster completed remote builds in B31381: Diff 72431.May 29 2020, 9:51 PM

jhb requested review of this revision.May 29 2020, 9:51 PM

Xin,

Conrad suggested you might be the best one to review this. These buffers are all on-stack buffers and contain different types of data: IVs / nonces, temporary copies of blocks of cipher or plain text, key schedules, and hash results. The current patch takes the approach of trying to zero all of the things I could find in cryptosoft, aesni, and ccr. However, if some of these data types aren't really sensitive I'm happy to pare down the number of explicit zeroes.

jhb added a child revision: D25058: Various optimizations to software AES-CCM and AES-GCM..May 29 2020, 11:30 PM

Looks good to me (there are a few minor nits or optional alternative proposals)

sys/crypto/aesni/aesni.c
819 ↗	(On Diff #72431)	GOOD: iv and tag were properly zeroed here, no exit after their first taint with data.
849 ↗	(On Diff #72431)	[OPTIONAL] Personally I'd probably just copy key to hmac_key and pad the remaining cells with 0's, and do a full sized ^ HMAC_IPAD_VAL, but that's just a personal taste and I'm fine if this is kept as-is as long as keylen is guaranteed to be <= sizeof(hmac_key) which appear to be checked in aesni_authprepare() if I followed the code correctly.
874 ↗	(On Diff #72431)	GOOD: hmac_key is only used in this block and the cleanse was done correctly. I'd probably define the variable in this if block to make it clear.
898 ↗	(On Diff #72431)	GOOD: res2 is only used in this block and the cleanse was done correctly. I'd probably move the definition into this block to make that clear.
901 ↗	(On Diff #72431)	GOOD: no exit point after res gets taineted and cleanse here is correct.
sys/dev/cxgbe/crypto/t4_crypto.c
754 ↗	(On Diff #72431)	GOOD: tainted in line 675 and no exit point befor this cleanse.
1073 ↗	(On Diff #72431)	GOOD: tainted in line 975 and no other exit point before cleansed here.
1356 ↗	(On Diff #72431)	GOOD: tainted in line 1275 and no exit point before cleansed here.
1501 ↗	(On Diff #72431)	GOOD: defined locally, tainted in line 1482 and no exit point before cleanse.
1509 ↗	(On Diff #72431)	GOOD: no exit points prior to this point for all 3 variables.
1824 ↗	(On Diff #72431)	GOOD: tainted in line 1731 and no exit points.
1964 ↗	(On Diff #72431)	GOOD: local variable cleansed at end of scope and no exit point.
1972 ↗	(On Diff #72431)	GOOD: no exit point before cleansing.
sys/dev/cxgbe/crypto/t4_keyctx.c
78 ↗	(On Diff #72431)	GOOD.
173 ↗	(On Diff #72431)	GOOD.
sys/opencrypto/cryptosoft.c
280 ↗	(On Diff #72431)	GOOD: tainted in line 182 and no exit before cleanse.
281 ↗	(On Diff #72431)	GOOD: tainted in line 142 and cleansed without exit.
282 ↗	(On Diff #72431)	GOOD: First potential taint in line 216 and no exit before cleanse.
392 ↗	(On Diff #72431)	GOOD: note that uaalg should probably be defined in this if block to make it clear that it only needs to get cleansed here.
397 ↗	(On Diff #72431)	GOOD: First potential taint in line 362 and properly cleansed here with no exit before.
454 ↗	(On Diff #72431)	GOOD: note that uaalg should probably be defined in this if block to make it clear that it only needs to get cleansed here.
459 ↗	(On Diff #72431)	GOOD: First possible taint in line 434 and no exit prior to this.
460 ↗	(On Diff #72431)	GOOD: First possible taint in line 446 and no exit prior to this.
461 ↗	(On Diff #72431)	GOOD: First possible taint in line 429 and no exit prior to this.
555 ↗	(On Diff #72431)	GOOD: note that uaalg should probably be defined in this if block to make it clear that it only needs to get cleansed here.
580 ↗	(On Diff #72431)	GOOD: First possible taint in line 508 and no exit prior to this.
581 ↗	(On Diff #72431)	GOOD: First possible taint in line 546 and no exit prior to this.
582 ↗	(On Diff #72431)	GOOD: First possible taint in line 498 and no exit prior to this.
637 ↗	(On Diff #72431)	GOOD: note that uaalg should probably be defined in this if block to make it clear that it only needs to get cleansed here.
657 ↗	(On Diff #72431)	GOOD: ctx are cleansed by Final methods.
771 ↗	(On Diff #72431)	GOOD: tainted in line 697 and no return here.
772 ↗	(On Diff #72431)	GOOD: tainted in line 712 and cleansed here without exit.
773 ↗	(On Diff #72431)	GOOD: tainted in line 680 and no return here.

This revision is now accepted and ready to land.May 30 2020, 4:40 AM

jhb added inline comments.Jun 3 2020, 7:26 PM

sys/crypto/aesni/aesni.c
849 ↗	(On Diff #72431)	Most other drivers now use a single function to generate the partial hash context which does exactly this (sys/opencrypto/crypto.c: hmac_init_pad()). The key length is checked and should be short enough, yes.
898 ↗	(On Diff #72431)	style(9) discourages moving the declarations into blocks, but I think in this case it might be a compelling reason to do so.