proto_crypt: clear the remaining stack buffers - #437
Open
woahwhattheheck wants to merge 1 commit into
Open
Conversation
426 wiped dk_2 and nonce_y in proto_crypt_mkkeys(). Two buffers holding key material in the same file were not covered by it. proto_crypt_secret() reads the key file into buf[BUFSIZ] and never zeroes it, on the success path or on the read-error path at err2. That buffer holds the pre-image of the shared secret, and for a key file of BUFSIZ bytes or less it holds all of it. The function already wipes the SHA-256 context two lines above, and proto_crypt_secret_free() zeroes the derived secret; the buffer it was derived from was the gap. Both spiped and spipe call this once at startup and then run for the lifetime of the process. proto_crypt_dhmac() leaves dk_1, the derived diffie-hellman MAC keys, in the frame when it returns. ctx in proto_crypt_enc() and proto_crypt_dec() is left alone: it is a copy of k->ctx_init, which stays in the key structure for the whole connection, and those run once per packet. nonce_CS is left alone because the nonces go over the wire in the clear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #436.
#426 wiped
dk_2andnonce_yinproto_crypt_mkkeys(). Two buffers holdingkey material in the same file were not covered by it:
proto_crypt_secret()reads the key file intobuf[BUFSIZ]and never zeroesit, on either the success path or the read-error path at
err2. That bufferis the pre-image of the shared secret — the whole key file, for a key file of
BUFSIZbytes or less. The function already wipes the SHA-256 context twolines above, and
proto_crypt_secret_free()zeroes the derived secret; thebuffer it was derived from was the gap. Both
spipedandspipecall thisonce at startup and then run for the lifetime of the process.
proto_crypt_dhmac()leavesdk_1, the derived diffie-hellman MAC keys, inthe frame when it returns. Called once per connection.
Three
insecure_memzero()calls, using the idiom from #426.Deliberately not covered
ctxinproto_crypt_enc()andproto_crypt_dec()— it is a copy ofk->ctx_init, which stays in the key structure for the whole connection, sowiping the copy buys nothing; and both functions run once per 1024-byte
packet, which is not somewhere to add work for no gain.
nonce_CSinproto_crypt_dhmac()— the nonces are sent over the wire inthe clear.
Happy to add either if you would rather have the file uniformly wiped.
Testing
No behaviour change: the writes happen after the last read of each buffer in
every path, and neither buffer is read again.
insecure_memzero.his alreadyincluded by this file.