Skip to content

libroach: fix concurrent access to same file when using encryption#26377

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
mberhault:marc/dont_reuse_cipher
Jun 4, 2018
Merged

libroach: fix concurrent access to same file when using encryption#26377
craig[bot] merged 1 commit intocockroachdb:masterfrom
mberhault:marc/dont_reuse_cipher

Conversation

@mberhault
Copy link
Copy Markdown
Contributor

@mberhault mberhault commented Jun 4, 2018

Fixes #26261.

The various Env::***File objects are used concurrently by rocksdb (eg: in the compaction code).
If multiple reads occur on the same encrypted file handle and CryptoPP is using the software AES implementation, some working state in the cipher gets corrupted by the other calls. This results in broken decryption bubbling up as checksum failures on rocksdb.

Fix this by passing the key to the CTRCipherStream and initializing a new Cipher object for each block of data (the RandomAccessFile::Read block. This will still use one Cipher for multiple AES blocks).

Release note (bug fix): fix concurrent access to same file when using encryption

@mberhault mberhault requested review from a team and bdarnell June 4, 2018 19:45
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@bdarnell
Copy link
Copy Markdown
Contributor

bdarnell commented Jun 4, 2018

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Fixes cockroachdb#26261.

The various `Env::***File` objects are used concurrently by rocksdb (eg: in the compaction code).
If multiple reads occur on the same encrypted file handle and CryptoPP is using the software AES implementation, some working state in the cipher gets corrupted by the other calls. This results in broken decryption bubbling up as checksum failures on rocksdb.

Fix this by passing the key to the `CTRCipherStream` and initializing a new `Cipher` object for each block of data (the `RandomAccessFile::Read` block. This will still use one `Cipher` for multiple AES blocks).

Release note (bug fix): fix concurrent access to same file when using encryption
@mberhault mberhault force-pushed the marc/dont_reuse_cipher branch from 6dfa24f to 6e9ad40 Compare June 4, 2018 21:14
@mberhault
Copy link
Copy Markdown
Contributor Author

TFTR. Rebased and squashed.

@mberhault
Copy link
Copy Markdown
Contributor Author

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Jun 4, 2018
26377: libroach: fix concurrent access to same file when using encryption r=mberhault a=mberhault

Fixes #26261.

The various `Env::***File` objects are used concurrently by rocksdb (eg: in the compaction code).
If multiple reads occur on the same encrypted file handle and CryptoPP is using the software AES implementation, some working state in the cipher gets corrupted by the other calls. This results in broken decryption bubbling up as checksum failures on rocksdb.

Fix this by passing the key to the `CTRCipherStream` and initializing a new `Cipher` object for each block of data (the `RandomAccessFile::Read` block. This will still use one `Cipher` for multiple AES blocks).

Release note (bug fix): fix concurrent access to same file when using encryption

Co-authored-by: marc <marc@cockroachlabs.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Build succeeded

@craig craig bot merged commit 6e9ad40 into cockroachdb:master Jun 4, 2018
@mberhault mberhault deleted the marc/dont_reuse_cipher branch June 4, 2018 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core: bad checksum on release builds when using encryption at rest

3 participants