Tests: cover the security-critical paths of the crypto and platform layers. - #1850
Open
bernardladenthin wants to merge 1 commit into
Open
Tests: cover the security-critical paths of the crypto and platform layers.#1850bernardladenthin wants to merge 1 commit into
bernardladenthin wants to merge 1 commit into
Conversation
Contributor
Author
|
Please merge #1844 before this PR. Both touch |
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.
What
Adds 14 test blocks to the existing self-test suites. No production code is touched and no
existing test is modified — 1133 lines, additions only, across 6 files:
src/Volume/EncryptionTest.cpp/.hVolumeInforound trip, volume layoutssrc/Platform/PlatformTest.cpp/.hBuffer/SecureBuffer/Memory,StringConverter, file operations, exception transportsrc/Main/UserInterface.cpp/.hMountOptionsround tripThe tests target paths the existing known-answer tests never reach, because those only ever
take the success path: rejection of bad parameters, uninitialised state, out-of-range access,
and the serialisation round trips that cross the IPC boundary to the privileged core service.
Merge order
Please merge #1844 first. Both PRs touch
src/Volume/EncryptionTest.{cpp,h}; the changes areindependent, but #1844 declares its method before
TestCipherswhile this branch insertsafter it. A plain
git applyof #1844 onto this branch fails on the header context, whilegit apply -3, a merge or a rebase all succeed. Merging #1844 first avoids the question. Iwill rebase this branch afterwards.
Coverage
Measured A/B in a single binary — same build, the new test calls commented out for the
baseline — using
gcov. Base ismaster@b48e31f5(1.26.29).Volume/Hash.cppVolume/VolumeLayout.cppVolume/VolumePasswordCache.cppCore/RandomNumberGenerator.cppVolume/Keyfile.cppVolume/VolumeInfo.cppPlatform/FileCommon.cppCore/MountOptions.cppPlatform/Memory.cppVolume/VolumePassword.cppVolume/Pkcs5Kdf.cppVolume/Cipher.cppPlatform/Buffer.cppVolume/EncryptionAlgorithm.cppVolume/VolumeHeader.cppVolume/EncryptionModeXTS.cppPlatform/StringConverter.cpp17 files improved, five of them starting from zero.
Coverage was produced without modifying any build file, using the existing hooks in
src/Makefile:A note for anyone repeating this: use
gcovdirectly. Bothlcovandgcovrmis-attributeresults here, because each subproject is built through
make -Cand the working directoryrecorded in the
.gcnofiles cannot be resolved afterwards.lcovreported one file at 8.7 %where the true figure is 91.4 %, and produced merged totals lower than the individual runs.
Detection proof
Coverage alone does not show that a test would catch anything, so every block was verified by
injecting a defect and confirming that the new test itself reports it. 27 injected defects in
total, among them:
Buffer::Eraseno longer wipesBufferTestBuffer::GetRangeskips its bounds checkBufferTestMemory::Compareinverts its orderingBufferTestTestKeyfileApplicationTestKeyfileApplicationTestKeyfileApplicationMountOptionsloses the protection mode in transitTestMountOptionsSerializationTestPasswordHandlingTestPasswordHandlingTestPasswordHandlingTestRandomNumberGeneratorStart()TestRandomNumberGeneratorTestRandomNumberGeneratorFile::Copytruncates the copyFileTestReadCompleteBufferaccepts short readsFileTestExceptionTransportTestTestEdgeCasesSetKeyaccepts a wrong-length keyTestEdgeCasesTestHashClassesTestKdfSelectionTestVolumeLayoutsOne side effect worth mentioning: the random number generator carries its own pool-mixing
known-answer self-test inside
Start(). That self-test previously ran only when the graphicalor the text interface started the generator —
--testnever started it, so it never ran underCI. It does now.
Verification environment
Linux — Docker
ubuntu:24.04, GCC 13, distribution wxWidgets 3.2,make NOGUI=1. Therepository is mounted read-only and copied into the container before building, so no build
artefacts touch the working tree. The self-tests were run twice to confirm repeatability, and
the temporary files created by the keyfile and file tests are removed even when an assertion
fires.
Windows — Visual Studio 2022 17.14.37, MSVC 14.44.35228, Windows SDK 10.0.26100.0,
WDK 10.0.26100. Each affected translation unit was compiled in user mode at
/W4and inkernel mode at
/W4 /WX, matching theTreatWarningAsErrorsetting inDriver.vcxproj.None of the six changed files appears in any
.vcxproj, so the Windows build cannot beaffected by this change. The compile checks were run regardless.
Limitations
Volume/VolumeHeader.cppmoves by only 1.6 pp. Its remaining uncovered part is thedecrypt-attempt loop, which needs a real encrypted volume — out of scope for a unit test.
Volume/Keyfile.cppstops at 59 %. The remaining branches are the security-token anddirectory-enumeration paths, which need a token or a populated directory.
Format, Mount or ExpandVolume actually start, not automatically.
AI assistance
These tests were written with the assistance of Anthropic's Claude Opus. Every claim in this
description — the coverage figures, the injected-defect results, the build and warning output
— was produced by running the builds and measurements described above rather than estimated.
The changes are additive and confined to test code; review of the assertions themselves is of
course still welcome.