From 372a967a146d06497b2fbff7b4c907da01c1a1f3 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Aug 2026 11:10:03 +0200 Subject: [PATCH 1/2] feat(oldms): report an encrypted .doc, .ppt or .xls as encrypted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An encrypted legacy file was parsed as if it were not, so the encrypted bytes were read as structure and the caller got whatever they happened to mean — for `odr-public/doc/encrypted.doc`, "Unexpected negative Fib.ccpText: -1408755250". OpenDocument.droid keys its password dialog off `FileEncrypted`, so a supported document that only needed a password looked unsupported, and the app offered to upload it to the online converter instead. Each format keeps what says so in the clear, which is what makes this readable without the password: - `.doc` — `FibBase.fEncrypted` ([MS-DOC] 2.5.2) - `.ppt` — `CurrentUserAtom.headerToken` ([MS-PPT] 2.3.2) - `.xls` — a `FilePass` record in the globals substream ([MS-XLS] 2.4.117) `parse_meta` asks the format's own module, and `LegacyMicrosoftFile` follows the odf/ooxml shape from there: `encryption_state()` answers `encrypted`, and `document()` throws `FileEncryptedError` rather than letting the parser loose on ciphertext. Detection only — `decrypt` still throws, so a reader can prompt but not open. Closes #638 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU --- CHANGELOG.md | 3 + src/odr/internal/oldms/oldms_file.cpp | 33 +++- src/odr/internal/oldms/oldms_file.hpp | 1 + .../oldms/presentation/ppt_parser.cpp | 19 +++ .../oldms/presentation/ppt_parser.hpp | 6 + .../oldms/presentation/ppt_structs.hpp | 5 + src/odr/internal/oldms/spreadsheet/AGENTS.md | 7 +- .../internal/oldms/spreadsheet/xls_parser.cpp | 36 +++++ .../internal/oldms/spreadsheet/xls_parser.hpp | 8 + .../oldms/spreadsheet/xls_structs.hpp | 1 + src/odr/internal/oldms/text/AGENTS.md | 5 +- src/odr/internal/oldms/text/doc_parser.cpp | 17 +++ src/odr/internal/oldms/text/doc_parser.hpp | 7 + src/odr/internal/oldms/text/doc_structs.hpp | 3 + test/CMakeLists.txt | 1 + test/src/html_output_test.cpp | 6 +- test/src/internal/oldms/encryption_test.cpp | 141 ++++++++++++++++++ 17 files changed, 290 insertions(+), 9 deletions(-) create mode 100644 test/src/internal/oldms/encryption_test.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 23d7ea928..288d44bc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,9 @@ The release run heads these entries with the version and opens a fresh spreadsheets alike. - Text copied out of a pdf laid out glyph by glyph reads as words, not as `L a g e`. A word break also survives a run with nothing extractable in it. +- An encrypted `.doc`, `.ppt` or `.xls` reports itself encrypted and raises + `FileEncrypted` instead of a parse error, so a reader can prompt for the + password. Decrypting them is still out of reach. ## v6.9.0 - 2026-08-18 diff --git a/src/odr/internal/oldms/oldms_file.cpp b/src/odr/internal/oldms/oldms_file.cpp index 6b83d5126..f33045605 100644 --- a/src/odr/internal/oldms/oldms_file.cpp +++ b/src/odr/internal/oldms/oldms_file.cpp @@ -4,8 +4,11 @@ #include #include +#include #include +#include #include +#include #include #include @@ -13,6 +16,22 @@ namespace odr::internal::oldms { namespace { +/// Each format keeps the bytes that say so in the clear, so this is readable +/// without the password. Detection only — odrcore cannot decrypt any of them. +bool parse_password_encrypted(const FileType type, + const abstract::ReadableFilesystem &files) { + switch (type) { + case FileType::legacy_word_document: + return text::password_encrypted(files); + case FileType::legacy_powerpoint_presentation: + return presentation::password_encrypted(files); + case FileType::legacy_excel_worksheets: + return spreadsheet::password_encrypted(files); + default: + return false; + } +} + FileMeta parse_meta(const abstract::ReadableFilesystem &files) { struct Variant { FileType type{FileType::unknown}; @@ -53,6 +72,8 @@ FileMeta parse_meta(const abstract::ReadableFilesystem &files) { throw UnknownFileType(); } + result.password_encrypted = parse_password_encrypted(result.type, files); + return result; } } // namespace @@ -61,6 +82,10 @@ LegacyMicrosoftFile::LegacyMicrosoftFile( std::shared_ptr files) : m_files{std::move(files)} { m_file_meta = parse_meta(*m_files); + + m_encryption_state = m_file_meta.password_encrypted + ? EncryptionState::encrypted + : EncryptionState::not_encrypted; } std::shared_ptr LegacyMicrosoftFile::file() const noexcept { @@ -86,7 +111,7 @@ bool LegacyMicrosoftFile::password_encrypted() const noexcept { } EncryptionState LegacyMicrosoftFile::encryption_state() const noexcept { - return EncryptionState::unknown; + return m_encryption_state; } std::shared_ptr LegacyMicrosoftFile::decrypt( @@ -98,6 +123,12 @@ std::shared_ptr LegacyMicrosoftFile::decrypt( bool LegacyMicrosoftFile::is_decodable() const noexcept { return false; } std::shared_ptr LegacyMicrosoftFile::document() const { + // otherwise the encrypted bytes get read as structure, and the caller sees a + // parse error where a password prompt belongs + if (m_encryption_state == EncryptionState::encrypted) { + throw FileEncryptedError(); + } + switch (file_type()) { case FileType::legacy_word_document: return std::make_shared(m_files); diff --git a/src/odr/internal/oldms/oldms_file.hpp b/src/odr/internal/oldms/oldms_file.hpp index ac6870a7e..2a33e9f13 100644 --- a/src/odr/internal/oldms/oldms_file.hpp +++ b/src/odr/internal/oldms/oldms_file.hpp @@ -40,6 +40,7 @@ class LegacyMicrosoftFile final : public abstract::DocumentFile { private: std::shared_ptr m_files; FileMeta m_file_meta; + EncryptionState m_encryption_state{EncryptionState::unknown}; }; } // namespace odr::internal::oldms diff --git a/src/odr/internal/oldms/presentation/ppt_parser.cpp b/src/odr/internal/oldms/presentation/ppt_parser.cpp index bd450fdc4..b7f2d68e0 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.cpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.cpp @@ -919,4 +919,23 @@ presentation::parse_tree(ElementRegistry ®istry, return root_id; } +bool presentation::password_encrypted( + const abstract::ReadableFilesystem &files) { + const std::shared_ptr file = + files.open(AbsPath("/Current User")); + if (file == nullptr) { + return false; + } + + const std::unique_ptr stream = file->stream(); + CurrentUserAtomHead head{}; + stream->read(reinterpret_cast(&head), sizeof(head)); + if (stream->gcount() != sizeof(head) || + head.rh.recType != RT_CurrentUserAtom) { + return false; + } + + return head.headerToken == current_user_token_encrypted; +} + } // namespace odr::internal::oldms diff --git a/src/odr/internal/oldms/presentation/ppt_parser.hpp b/src/odr/internal/oldms/presentation/ppt_parser.hpp index fb6f433b0..bc5dc1d2e 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.hpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.hpp @@ -16,4 +16,10 @@ ElementIdentifier parse_tree(ElementRegistry ®istry, StyleRegistry &style_registry, const abstract::ReadableFilesystem &files); +/// Whether the presentation is encrypted, from `CurrentUserAtom.headerToken` +/// ([MS-PPT] 2.3.2). A `/Current User` stream that is missing or too short +/// reads as not encrypted — parsing then fails on its own. +[[nodiscard]] bool +password_encrypted(const abstract::ReadableFilesystem &files); + } // namespace odr::internal::oldms::presentation diff --git a/src/odr/internal/oldms/presentation/ppt_structs.hpp b/src/odr/internal/oldms/presentation/ppt_structs.hpp index 639a18e6d..f7a23246a 100644 --- a/src/odr/internal/oldms/presentation/ppt_structs.hpp +++ b/src/odr/internal/oldms/presentation/ppt_structs.hpp @@ -9,6 +9,11 @@ namespace odr::internal::oldms::presentation { // LSB-first hosts only — see oldms/AGENTS.md. /// Record types relevant to text extraction. See [MS-PPT] 2.13.24 RecordType. +/// CurrentUserAtom.headerToken ([MS-PPT] 2.3.2); the file is encrypted when it +/// carries the second one. +constexpr std::uint32_t current_user_token_plain = 0xE391C05F; +constexpr std::uint32_t current_user_token_encrypted = 0xF3D1C4DF; + enum RecordType : std::uint16_t { RT_DocumentContainer = 0x03E8, //< top-level document RT_DocumentAtom = 0x03E9, //< slide size etc. [MS-PPT] 2.4.2 diff --git a/src/odr/internal/oldms/spreadsheet/AGENTS.md b/src/odr/internal/oldms/spreadsheet/AGENTS.md index 3475469f2..166481035 100644 --- a/src/odr/internal/oldms/spreadsheet/AGENTS.md +++ b/src/odr/internal/oldms/spreadsheet/AGENTS.md @@ -144,9 +144,10 @@ ignore their format codes. Fix by following the format chain: - **Hidden rows/columns** (`Row.fDyZero`, `ColInfo.fHidden`). - **Typed cell values**: expose numeric/bool/date `ValueType`s instead of pre-rendered strings. -- **Encrypted workbooks**: a `FilePass` (0x002F) in globals means the rest is - encrypted ([MS-OFFCRYPTO]); currently parses as garbage or throws — should report - password-protected. +- **Encrypted workbooks**: a `FilePass` (0x002F) in globals is what + `password_encrypted()` reports, so the file surfaces as encrypted rather than + parsing as garbage; reading one still needs a `decrypt` that throws + ([MS-OFFCRYPTO]). - **BIFF5/BIFF7** (`vers != 0x0600`): currently throws; older files exist in the wild (no SST — `Label` records carry strings inline). - **Drawings/charts/images** — likely never worth it for text extraction. diff --git a/src/odr/internal/oldms/spreadsheet/xls_parser.cpp b/src/odr/internal/oldms/spreadsheet/xls_parser.cpp index 5e4212025..34e083325 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_parser.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_parser.cpp @@ -243,4 +243,40 @@ spreadsheet::parse_tree(ElementRegistry ®istry, return root_id; } +bool spreadsheet::password_encrypted( + const abstract::ReadableFilesystem &files) { + const std::shared_ptr file = files.open(AbsPath("/Workbook")); + if (file == nullptr) { + return false; + } + + const std::unique_ptr stream = file->stream(); + BiffReader reader{*stream}; + + // FilePass sits at the head of the globals substream, right after BOF and an + // optional WriteProtect ([MS-XLS] 2.1.7.20.1). Record headers are not + // encrypted, so they can be walked either way; the substream's own EOF, or + // the BOF of the first sheet, ends the search. + try { + if (!reader.next_record() || reader.record_type() != biff_bof) { + return false; + } + while (reader.next_record()) { + switch (reader.record_type()) { + case biff_filepass: + return true; + case biff_bof: + case biff_eof: + return false; + default: + break; + } + } + } catch (const std::exception &) { + // a stream that cannot be walked cannot say it is encrypted + } + + return false; +} + } // namespace odr::internal::oldms diff --git a/src/odr/internal/oldms/spreadsheet/xls_parser.hpp b/src/odr/internal/oldms/spreadsheet/xls_parser.hpp index df3e668bc..1f09f52f9 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_parser.hpp +++ b/src/odr/internal/oldms/spreadsheet/xls_parser.hpp @@ -18,4 +18,12 @@ ElementIdentifier parse_tree(ElementRegistry ®istry, StyleRegistry &style_registry, const abstract::ReadableFilesystem &files); +/// Whether the workbook is encrypted, i.e. whether the globals substream +/// carries a FilePass record ([MS-XLS] 2.4.117). Record headers stay in the +/// clear, which is what makes this readable at all. A `/Workbook` stream that +/// is missing or malformed reads as not encrypted — parsing then fails on its +/// own. +[[nodiscard]] bool +password_encrypted(const abstract::ReadableFilesystem &files); + } // namespace odr::internal::oldms::spreadsheet diff --git a/src/odr/internal/oldms/spreadsheet/xls_structs.hpp b/src/odr/internal/oldms/spreadsheet/xls_structs.hpp index c68237dbb..a76ef52a6 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_structs.hpp +++ b/src/odr/internal/oldms/spreadsheet/xls_structs.hpp @@ -14,6 +14,7 @@ namespace odr::internal::oldms::spreadsheet { enum BiffRecordType : std::uint16_t { biff_formula = 0x0006, //< [MS-XLS] 2.4.127 biff_eof = 0x000A, //< [MS-XLS] 2.4.103 + biff_filepass = 0x002F, //< [MS-XLS] 2.4.117 biff_font = 0x0031, //< [MS-XLS] 2.4.122 biff_continue = 0x003C, //< [MS-XLS] 2.4.58 biff_boundsheet = 0x0085, //< [MS-XLS] 2.4.28 BoundSheet8 diff --git a/src/odr/internal/oldms/text/AGENTS.md b/src/odr/internal/oldms/text/AGENTS.md index 56898e258..24fd46430 100644 --- a/src/odr/internal/oldms/text/AGENTS.md +++ b/src/odr/internal/oldms/text/AGENTS.md @@ -200,8 +200,9 @@ WordDocument stream margins, §2.6.4) are unparsed. - **Images / OLE / drawn objects** — anchor chars dropped; would need `PlcfSpa` / Office Art (`dggInfo`). -- **Encrypted / obfuscated** — `fEncrypted`/`fObfuscated` parsed but not acted on; - `decrypt` throws. +- **Encrypted / obfuscated** — `fEncrypted` is what `password_encrypted()` + reports, so the file surfaces as encrypted rather than throwing a parse error; + reading one still needs a `decrypt` that throws ([MS-OFFCRYPTO]). ## 3. Smaller shortcomings diff --git a/src/odr/internal/oldms/text/doc_parser.cpp b/src/odr/internal/oldms/text/doc_parser.cpp index 933d074de..0c0ebdbbd 100644 --- a/src/odr/internal/oldms/text/doc_parser.cpp +++ b/src/odr/internal/oldms/text/doc_parser.cpp @@ -253,4 +253,21 @@ ElementIdentifier text::parse_tree(ElementRegistry ®istry, return root_id; } +bool text::password_encrypted(const abstract::ReadableFilesystem &files) { + const std::shared_ptr file = + files.open(AbsPath("/WordDocument")); + if (file == nullptr) { + return false; + } + + const std::unique_ptr stream = file->stream(); + FibBase base{}; + stream->read(reinterpret_cast(&base), sizeof(base)); + if (stream->gcount() != sizeof(base) || base.wIdent != fib_wIdent) { + return false; + } + + return base.fEncrypted != 0; +} + } // namespace odr::internal::oldms diff --git a/src/odr/internal/oldms/text/doc_parser.hpp b/src/odr/internal/oldms/text/doc_parser.hpp index 24dfb4139..92b83aaff 100644 --- a/src/odr/internal/oldms/text/doc_parser.hpp +++ b/src/odr/internal/oldms/text/doc_parser.hpp @@ -17,4 +17,11 @@ ElementIdentifier parse_tree(ElementRegistry ®istry, StyleRegistry &style_registry, const abstract::ReadableFilesystem &files); +/// Whether the document is encrypted or obfuscated, from `FibBase.fEncrypted` +/// ([MS-DOC] 2.5.2). The FIB itself stays in the clear, which is what makes +/// this readable at all. A `/WordDocument` stream that is missing or too short +/// reads as not encrypted — parsing then fails on its own. +[[nodiscard]] bool +password_encrypted(const abstract::ReadableFilesystem &files); + } // namespace odr::internal::oldms::text diff --git a/src/odr/internal/oldms/text/doc_structs.hpp b/src/odr/internal/oldms/text/doc_structs.hpp index 52a474133..4fc086c76 100644 --- a/src/odr/internal/oldms/text/doc_structs.hpp +++ b/src/odr/internal/oldms/text/doc_structs.hpp @@ -12,6 +12,9 @@ namespace odr::internal::oldms::text { // Filled by copying file bytes straight in (see doc_io): little-endian, // LSB-first hosts only — see oldms/AGENTS.md. +/// FibBase.wIdent of every word binary document ([MS-DOC] 2.5.2). +constexpr std::uint16_t fib_wIdent = 0xA5EC; + enum NFibValues : std::uint16_t { nFib97 = 0x00C1, nFib2000 = 0x00D9, diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 8f9ac2866..8b516952c 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -62,6 +62,7 @@ add_executable(odr_test "src/internal/odf/odf_table_test.cpp" "src/internal/oldms/doc_test.cpp" + "src/internal/oldms/encryption_test.cpp" "src/internal/oldms/ppt_test.cpp" "src/internal/oldms/xls_test.cpp" diff --git a/test/src/html_output_test.cpp b/test/src/html_output_test.cpp index b81793e8e..eaf130fad 100644 --- a/test/src/html_output_test.cpp +++ b/test/src/html_output_test.cpp @@ -131,14 +131,14 @@ TEST_P(HtmlOutputTests, html_meta) { GTEST_SKIP(); } - // TODO oldms decryption + EXPECT_EQ(test_file.password.has_value(), file.password_encrypted()); + + // TODO oldms decryption — detected, but odrcore cannot open it if (test_file.password.has_value() && test_file.type == FileType::legacy_word_document) { GTEST_SKIP(); } - EXPECT_EQ(test_file.password.has_value(), file.password_encrypted()); - if (test_file.password.has_value()) { file = file.decrypt(test_file.password.value()); diff --git a/test/src/internal/oldms/encryption_test.cpp b/test/src/internal/oldms/encryption_test.cpp new file mode 100644 index 000000000..f30f9edf3 --- /dev/null +++ b/test/src/internal/oldms/encryption_test.cpp @@ -0,0 +1,141 @@ +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include + +#include +#include +#include + +using namespace odr; +using namespace odr::internal; +using namespace odr::internal::oldms; +using odr::test::TestData; +using odr::test::oldms::append_u16; +using odr::test::oldms::append_u32; + +namespace { + +/// A filesystem holding one stream, standing in for the cfb one of a real +/// file — the probes only ever open a stream by name. +VirtualFilesystem filesystem_of(const std::string &path, + const std::string &data) { + VirtualFilesystem result; + result.copy(std::make_shared(data), AbsPath(path)); + return result; +} + +/// A `FibBase` ([MS-DOC] 2.5.2) with everything but the flags zeroed. +std::string word_document_stream(const bool encrypted) { + std::string result; + append_u16(result, text::fib_wIdent); + append_u16(result, text::nFib97); + append_u16(result, 0); // unused + append_u16(result, 0); // lid + append_u16(result, 0); // pnNext + // fDot .. fObfuscated; fEncrypted is bit 8 + append_u16(result, encrypted ? 0x0100 : 0x0000); + result.resize(sizeof(text::FibBase), '\0'); + return result; +} + +/// A `CurrentUserAtom` head ([MS-PPT] 2.3.2). +std::string current_user_stream(const bool encrypted) { + std::string result; + append_u16(result, 0); // recVer / recInstance + append_u16(result, presentation::RT_CurrentUserAtom); + append_u32(result, 0x14); // recLen + append_u32(result, 0x14); // size + append_u32(result, encrypted ? presentation::current_user_token_encrypted + : presentation::current_user_token_plain); + append_u32(result, 0); // offsetToCurrentEdit + return result; +} + +void append_record(std::string &out, const std::uint16_t type, + const std::size_t size) { + append_u16(out, type); + append_u16(out, static_cast(size)); + out.append(size, '\0'); +} + +/// A globals substream ([MS-XLS] 2.1.7.20.1) — BOF, then the records the +/// search walks. +std::string workbook_stream(const bool encrypted) { + std::string result; + append_record(result, spreadsheet::biff_bof, 16); + if (encrypted) { + append_record(result, spreadsheet::biff_filepass, 54); + } + append_record(result, spreadsheet::biff_font, 14); + append_record(result, spreadsheet::biff_eof, 0); + return result; +} + +} // namespace + +TEST(OldMsEncryption, doc_reports_the_encrypted_flag) { + EXPECT_TRUE(text::password_encrypted( + filesystem_of("/WordDocument", word_document_stream(true)))); + EXPECT_FALSE(text::password_encrypted( + filesystem_of("/WordDocument", word_document_stream(false)))); +} + +TEST(OldMsEncryption, ppt_reports_the_header_token) { + EXPECT_TRUE(presentation::password_encrypted( + filesystem_of("/Current User", current_user_stream(true)))); + EXPECT_FALSE(presentation::password_encrypted( + filesystem_of("/Current User", current_user_stream(false)))); +} + +TEST(OldMsEncryption, xls_reports_a_file_pass_record) { + EXPECT_TRUE(spreadsheet::password_encrypted( + filesystem_of("/Workbook", workbook_stream(true)))); + EXPECT_FALSE(spreadsheet::password_encrypted( + filesystem_of("/Workbook", workbook_stream(false)))); +} + +TEST(OldMsEncryption, a_missing_stream_is_not_encrypted) { + const VirtualFilesystem empty; + + EXPECT_FALSE(text::password_encrypted(empty)); + EXPECT_FALSE(presentation::password_encrypted(empty)); + EXPECT_FALSE(spreadsheet::password_encrypted(empty)); +} + +TEST(OldMsEncryption, a_truncated_stream_is_not_encrypted) { + EXPECT_FALSE(text::password_encrypted( + filesystem_of("/WordDocument", word_document_stream(true).substr(0, 8)))); + EXPECT_FALSE(presentation::password_encrypted( + filesystem_of("/Current User", current_user_stream(true).substr(0, 8)))); + EXPECT_FALSE(spreadsheet::password_encrypted( + filesystem_of("/Workbook", workbook_stream(true).substr(0, 3)))); +} + +/// The file that reported the issue: it threw +/// "Unexpected negative Fib.ccpText" instead of asking for a password. +TEST(OldMsEncryption, an_encrypted_doc_surfaces_as_encrypted) { + const DecodedFile file = + odr::open(TestData::test_file_path("odr-public/doc/encrypted.doc")); + + EXPECT_EQ(file.file_type(), FileType::legacy_word_document); + EXPECT_TRUE(file.password_encrypted()); + EXPECT_EQ(file.encryption_state(), EncryptionState::encrypted); + EXPECT_THROW(static_cast(file.as_document_file().document()), + FileEncryptedError); +} From a3614936c3602f1f036b6d2427eebac759945e24 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Aug 2026 11:54:59 +0200 Subject: [PATCH 2/2] fix(oldms): keep the encryption state unknown where the probe cannot read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A probe that could not inspect its stream — a missing `/Current User`, a truncated FIB, a record walk that throws — returned `false`, and that became the definitive `EncryptionState::not_encrypted`. A file we could not read is not a file that told us it is in the clear. The probes answer `std::optional` now: nothing where they could not read the signal. `FileMeta::password_encrypted` still takes `false` from that — it has only the boolean — but `encryption_state()` answers `unknown`, which is what it meant for every legacy file before this, so a malformed file goes through the parser and fails on its own terms. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU --- src/odr/internal/oldms/oldms_file.cpp | 23 ++++--- .../oldms/presentation/ppt_parser.cpp | 8 +-- .../oldms/presentation/ppt_parser.hpp | 8 ++- .../internal/oldms/spreadsheet/xls_parser.cpp | 14 ++-- .../internal/oldms/spreadsheet/xls_parser.hpp | 10 +-- src/odr/internal/oldms/text/doc_parser.cpp | 7 +- src/odr/internal/oldms/text/doc_parser.hpp | 9 ++- test/src/internal/oldms/encryption_test.cpp | 69 +++++++++++++------ 8 files changed, 96 insertions(+), 52 deletions(-) diff --git a/src/odr/internal/oldms/oldms_file.cpp b/src/odr/internal/oldms/oldms_file.cpp index f33045605..b44f1e126 100644 --- a/src/odr/internal/oldms/oldms_file.cpp +++ b/src/odr/internal/oldms/oldms_file.cpp @@ -11,6 +11,7 @@ #include #include +#include #include namespace odr::internal::oldms { @@ -18,8 +19,10 @@ namespace odr::internal::oldms { namespace { /// Each format keeps the bytes that say so in the clear, so this is readable /// without the password. Detection only — odrcore cannot decrypt any of them. -bool parse_password_encrypted(const FileType type, - const abstract::ReadableFilesystem &files) { +/// Nothing where the format's own probe could not read the signal. +std::optional +parse_password_encrypted(const FileType type, + const abstract::ReadableFilesystem &files) { switch (type) { case FileType::legacy_word_document: return text::password_encrypted(files); @@ -28,7 +31,7 @@ bool parse_password_encrypted(const FileType type, case FileType::legacy_excel_worksheets: return spreadsheet::password_encrypted(files); default: - return false; + return {}; } } @@ -72,8 +75,6 @@ FileMeta parse_meta(const abstract::ReadableFilesystem &files) { throw UnknownFileType(); } - result.password_encrypted = parse_password_encrypted(result.type, files); - return result; } } // namespace @@ -83,9 +84,15 @@ LegacyMicrosoftFile::LegacyMicrosoftFile( : m_files{std::move(files)} { m_file_meta = parse_meta(*m_files); - m_encryption_state = m_file_meta.password_encrypted - ? EncryptionState::encrypted - : EncryptionState::not_encrypted; + // `EncryptionState::unknown` where the probe could not read the signal: a + // stream that cannot be inspected is not a document that said it is in the + // clear, and `FileMeta` has only the boolean to carry it. + const std::optional encrypted = + parse_password_encrypted(m_file_meta.type, *m_files); + m_file_meta.password_encrypted = encrypted.value_or(false); + m_encryption_state = !encrypted.has_value() ? EncryptionState::unknown + : *encrypted ? EncryptionState::encrypted + : EncryptionState::not_encrypted; } std::shared_ptr LegacyMicrosoftFile::file() const noexcept { diff --git a/src/odr/internal/oldms/presentation/ppt_parser.cpp b/src/odr/internal/oldms/presentation/ppt_parser.cpp index b7f2d68e0..c903f87b1 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.cpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.cpp @@ -919,12 +919,12 @@ presentation::parse_tree(ElementRegistry ®istry, return root_id; } -bool presentation::password_encrypted( - const abstract::ReadableFilesystem &files) { +std::optional +presentation::password_encrypted(const abstract::ReadableFilesystem &files) { const std::shared_ptr file = files.open(AbsPath("/Current User")); if (file == nullptr) { - return false; + return {}; } const std::unique_ptr stream = file->stream(); @@ -932,7 +932,7 @@ bool presentation::password_encrypted( stream->read(reinterpret_cast(&head), sizeof(head)); if (stream->gcount() != sizeof(head) || head.rh.recType != RT_CurrentUserAtom) { - return false; + return {}; } return head.headerToken == current_user_token_encrypted; diff --git a/src/odr/internal/oldms/presentation/ppt_parser.hpp b/src/odr/internal/oldms/presentation/ppt_parser.hpp index bc5dc1d2e..34363c5c3 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.hpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.hpp @@ -2,6 +2,8 @@ #include +#include + namespace odr::internal::abstract { class ReadableFilesystem; } @@ -17,9 +19,9 @@ ElementIdentifier parse_tree(ElementRegistry ®istry, const abstract::ReadableFilesystem &files); /// Whether the presentation is encrypted, from `CurrentUserAtom.headerToken` -/// ([MS-PPT] 2.3.2). A `/Current User` stream that is missing or too short -/// reads as not encrypted — parsing then fails on its own. -[[nodiscard]] bool +/// ([MS-PPT] 2.3.2). Nothing where the `/Current User` stream is missing, too +/// short, or does not hold a CurrentUserAtom: that is not an answer. +[[nodiscard]] std::optional password_encrypted(const abstract::ReadableFilesystem &files); } // namespace odr::internal::oldms::presentation diff --git a/src/odr/internal/oldms/spreadsheet/xls_parser.cpp b/src/odr/internal/oldms/spreadsheet/xls_parser.cpp index 34e083325..61f97dff8 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_parser.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_parser.cpp @@ -243,11 +243,11 @@ spreadsheet::parse_tree(ElementRegistry ®istry, return root_id; } -bool spreadsheet::password_encrypted( - const abstract::ReadableFilesystem &files) { +std::optional +spreadsheet::password_encrypted(const abstract::ReadableFilesystem &files) { const std::shared_ptr file = files.open(AbsPath("/Workbook")); if (file == nullptr) { - return false; + return {}; } const std::unique_ptr stream = file->stream(); @@ -259,7 +259,7 @@ bool spreadsheet::password_encrypted( // the BOF of the first sheet, ends the search. try { if (!reader.next_record() || reader.record_type() != biff_bof) { - return false; + return {}; } while (reader.next_record()) { switch (reader.record_type()) { @@ -273,10 +273,12 @@ bool spreadsheet::password_encrypted( } } } catch (const std::exception &) { - // a stream that cannot be walked cannot say it is encrypted + // a stream that cannot be walked cannot answer either way + return {}; } - return false; + // the records ran out before the globals substream ended + return {}; } } // namespace odr::internal::oldms diff --git a/src/odr/internal/oldms/spreadsheet/xls_parser.hpp b/src/odr/internal/oldms/spreadsheet/xls_parser.hpp index 1f09f52f9..ff1f77001 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_parser.hpp +++ b/src/odr/internal/oldms/spreadsheet/xls_parser.hpp @@ -2,6 +2,8 @@ #include +#include + namespace odr::internal::abstract { class ReadableFilesystem; } // namespace odr::internal::abstract @@ -20,10 +22,10 @@ ElementIdentifier parse_tree(ElementRegistry ®istry, /// Whether the workbook is encrypted, i.e. whether the globals substream /// carries a FilePass record ([MS-XLS] 2.4.117). Record headers stay in the -/// clear, which is what makes this readable at all. A `/Workbook` stream that -/// is missing or malformed reads as not encrypted — parsing then fails on its -/// own. -[[nodiscard]] bool +/// clear, which is what makes this readable at all. Nothing where the +/// `/Workbook` stream is missing, or where the record walk does not reach the +/// end of the globals substream: that is not an answer. +[[nodiscard]] std::optional password_encrypted(const abstract::ReadableFilesystem &files); } // namespace odr::internal::oldms::spreadsheet diff --git a/src/odr/internal/oldms/text/doc_parser.cpp b/src/odr/internal/oldms/text/doc_parser.cpp index 0c0ebdbbd..53baef488 100644 --- a/src/odr/internal/oldms/text/doc_parser.cpp +++ b/src/odr/internal/oldms/text/doc_parser.cpp @@ -253,18 +253,19 @@ ElementIdentifier text::parse_tree(ElementRegistry ®istry, return root_id; } -bool text::password_encrypted(const abstract::ReadableFilesystem &files) { +std::optional +text::password_encrypted(const abstract::ReadableFilesystem &files) { const std::shared_ptr file = files.open(AbsPath("/WordDocument")); if (file == nullptr) { - return false; + return {}; } const std::unique_ptr stream = file->stream(); FibBase base{}; stream->read(reinterpret_cast(&base), sizeof(base)); if (stream->gcount() != sizeof(base) || base.wIdent != fib_wIdent) { - return false; + return {}; } return base.fEncrypted != 0; diff --git a/src/odr/internal/oldms/text/doc_parser.hpp b/src/odr/internal/oldms/text/doc_parser.hpp index 92b83aaff..4f8f71a51 100644 --- a/src/odr/internal/oldms/text/doc_parser.hpp +++ b/src/odr/internal/oldms/text/doc_parser.hpp @@ -2,6 +2,8 @@ #include +#include + namespace odr::internal::abstract { class ReadableFilesystem; } @@ -19,9 +21,10 @@ ElementIdentifier parse_tree(ElementRegistry ®istry, /// Whether the document is encrypted or obfuscated, from `FibBase.fEncrypted` /// ([MS-DOC] 2.5.2). The FIB itself stays in the clear, which is what makes -/// this readable at all. A `/WordDocument` stream that is missing or too short -/// reads as not encrypted — parsing then fails on its own. -[[nodiscard]] bool +/// this readable at all. Nothing where the `/WordDocument` stream is missing or +/// too short to carry a FIB: that is not an answer, and saying "not encrypted" +/// would be claiming one. +[[nodiscard]] std::optional password_encrypted(const abstract::ReadableFilesystem &files); } // namespace odr::internal::oldms::text diff --git a/test/src/internal/oldms/encryption_test.cpp b/test/src/internal/oldms/encryption_test.cpp index f30f9edf3..985293aa3 100644 --- a/test/src/internal/oldms/encryption_test.cpp +++ b/test/src/internal/oldms/encryption_test.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -90,41 +91,67 @@ std::string workbook_stream(const bool encrypted) { } // namespace TEST(OldMsEncryption, doc_reports_the_encrypted_flag) { - EXPECT_TRUE(text::password_encrypted( - filesystem_of("/WordDocument", word_document_stream(true)))); - EXPECT_FALSE(text::password_encrypted( - filesystem_of("/WordDocument", word_document_stream(false)))); + EXPECT_EQ(text::password_encrypted( + filesystem_of("/WordDocument", word_document_stream(true))), + true); + EXPECT_EQ(text::password_encrypted( + filesystem_of("/WordDocument", word_document_stream(false))), + false); } TEST(OldMsEncryption, ppt_reports_the_header_token) { - EXPECT_TRUE(presentation::password_encrypted( - filesystem_of("/Current User", current_user_stream(true)))); - EXPECT_FALSE(presentation::password_encrypted( - filesystem_of("/Current User", current_user_stream(false)))); + EXPECT_EQ(presentation::password_encrypted( + filesystem_of("/Current User", current_user_stream(true))), + true); + EXPECT_EQ(presentation::password_encrypted( + filesystem_of("/Current User", current_user_stream(false))), + false); } TEST(OldMsEncryption, xls_reports_a_file_pass_record) { - EXPECT_TRUE(spreadsheet::password_encrypted( - filesystem_of("/Workbook", workbook_stream(true)))); - EXPECT_FALSE(spreadsheet::password_encrypted( - filesystem_of("/Workbook", workbook_stream(false)))); + EXPECT_EQ(spreadsheet::password_encrypted( + filesystem_of("/Workbook", workbook_stream(true))), + true); + EXPECT_EQ(spreadsheet::password_encrypted( + filesystem_of("/Workbook", workbook_stream(false))), + false); } -TEST(OldMsEncryption, a_missing_stream_is_not_encrypted) { +/// Not an answer either way — saying "not encrypted" would be claiming one. +TEST(OldMsEncryption, a_missing_stream_answers_nothing) { const VirtualFilesystem empty; - EXPECT_FALSE(text::password_encrypted(empty)); - EXPECT_FALSE(presentation::password_encrypted(empty)); - EXPECT_FALSE(spreadsheet::password_encrypted(empty)); + EXPECT_FALSE(text::password_encrypted(empty).has_value()); + EXPECT_FALSE(presentation::password_encrypted(empty).has_value()); + EXPECT_FALSE(spreadsheet::password_encrypted(empty).has_value()); } -TEST(OldMsEncryption, a_truncated_stream_is_not_encrypted) { +TEST(OldMsEncryption, a_truncated_stream_answers_nothing) { EXPECT_FALSE(text::password_encrypted( - filesystem_of("/WordDocument", word_document_stream(true).substr(0, 8)))); + filesystem_of("/WordDocument", + word_document_stream(true).substr(0, 8))) + .has_value()); EXPECT_FALSE(presentation::password_encrypted( - filesystem_of("/Current User", current_user_stream(true).substr(0, 8)))); - EXPECT_FALSE(spreadsheet::password_encrypted( - filesystem_of("/Workbook", workbook_stream(true).substr(0, 3)))); + filesystem_of("/Current User", + current_user_stream(true).substr(0, 8))) + .has_value()); + EXPECT_FALSE( + spreadsheet::password_encrypted( + filesystem_of("/Workbook", workbook_stream(true).substr(0, 3))) + .has_value()); +} + +/// A file whose stream says nothing readable keeps `unknown`, which is what the +/// state meant before any of this: the parser then fails on its own terms. +TEST(OldMsEncryption, an_unreadable_stream_leaves_the_state_unknown) { + const auto files = std::make_shared( + filesystem_of("/WordDocument", "too short")); + + const LegacyMicrosoftFile file(files); + + EXPECT_EQ(file.file_type(), FileType::legacy_word_document); + EXPECT_FALSE(file.password_encrypted()); + EXPECT_EQ(file.encryption_state(), EncryptionState::unknown); } /// The file that reported the issue: it threw