KdbxXmlReader: Support Protected attribute on most nodes #1646

luzat · 2018-03-05T20:17:57Z

Description

When parsing KDBX XML, the KdbXmlReader now supports Protected="true" on all attributes where readString or readBinary is used.

Motivation and context

I had a KDBX 3.1 file which contained Binary entries with Protected="true". This lead to KeePassXC mostly reading the file fine, but because of the differences in the state of the random stream all passwords and attached files were silently mangled.

I am not sure which application created those (KeePass2, KyPass or Keepass2Android), but KeePass2 seems to parse Protected="true" in a similar way in its KdbxFile::ProcessNode method. To solve this specific case, handling Protected="true" only on <Binary> elements would have sufficed, but this could have lead to additional bugs later on if any other element is ever protected. This could corrupt the database. It would be helpful if the file format had a checksum for this stream (or its final state) to detect such errors.

How has this been tested?

I did check that attachments and passwords work in the KDBX 3.1 file and its KDBX 4 counterpart (contains a couple of hundred entries and a few dozen attached files). I created a Mingw build on Windows 10 Insider Preview with everything but YubiKey support. The automated test suite passed, GUI and CLI also seem to work fine.

Types of changes

I consider this a bug fix which should allow KeePassXC to load more files successfully instead of silently failing (and possibly corrupting them on save). The code should probably be more thoroughly tested with various databases and test cases.

Checklist:

✅ I have read the CONTRIBUTING document.
✅ My code follows the code style of this project.
✅ All new and existing tests passed.

Not needed from what I can tell:

My change requires a change to the documentation and I have updated it accordingly.

BUT, I failed to do these:

I have added tests to cover my changes.
I have compiled and verified my code with -DWITH_ASAN=ON

Existing tests work (including those with protected) and the Windows build environment I tested with didn't allow -DWITH_ASAN=ON.

luzat · 2018-03-05T20:38:48Z

A workaround for users is to switch to KDBX 4, but in theory some application could decide to use Protected="true" on other fields and break all protected fields (while at least KeePass2 can still read them).

phoerious

This is something I noticed, too while writing the KeePass2 XML format RFC: https://github.com/keepassxreboot/keepassxc-specs/blob/master/kdbx-xml/rfc.txt
ALL non-complex elements can be protected.

phoerious · 2018-03-05T20:47:30Z

src/format/KdbxXmlReader.cpp

+                raiseError(tr("Unable to decrypt entry string"));
+                continue;
+            }
+
            bool isProtected = attr.value("Protected") == "True";
            bool protectInMemory = attr.value("ProtectInMemory") == "True";


You should remove this here. Both Protected and ProtectInMemory should be handled by the readString() method. That method should have additional output parameters to indicate whether the attribute was protected and should be protected in memory.

Not sure how to handle that. Two output references? But readString and readBinary are called without requiring the result. Should I add 0 and 2 parameter overloads?

Two output references, yes. You may add overloads, but I ideally, all callers of this method should use those parameters.

Dozens of occurences and (iirc) only 2 need the output, so I chose the overloaded version.

phoerious · 2018-03-05T20:48:52Z

src/format/KdbxXmlReader.cpp

+    QXmlStreamAttributes attr = m_xml.attributes();
+    QString value = m_xml.readElementText();
+    bool isProtected = attr.hasAttribute("Protected")
+        && (attr.value("Protected") == "True");


You should compare case-insensitively and also allow 1 and 0 in addition to True and False.

I was wondering and reused the existing code. This seems to be handled differently in different places. Should I introduce a static helper function isTruthy or similar?

If you don't have to change too many lines of code, you can do that. Otherwise just handle it locally. KeePass seems to be somewhat inconsistent and sometimes uses True, or true or 1.

For reference, KeePass2 does bProtect = ((strProtect != null) && (strProtect == ValTrue)); there. For strict compatibility an equality check may be preferable, but it's unlikely to encounter incompatibilities with real data there.

Ye, it dos it here, but while writing the RFC I tested various configurations and I noticed that from other boolean elements/attributes, KeePass2 sometimes writes true/false or 1/0. I therefore made that a feature of the spec. For compatibility we should always write it as True/False, but should support reading true/false and 1/0.

phoerious · 2018-03-05T20:50:25Z

src/format/KdbxXmlReader.cpp

+    QString value = m_xml.readElementText();
+    QByteArray data = QByteArray::fromBase64(value.toLatin1());
+    bool isProtected = attr.hasAttribute("Protected")
+        && (attr.value("Protected") == "True");


phoerious · 2018-03-05T20:52:25Z

src/format/KdbxXmlReader.cpp

@@ -814,26 +814,14 @@ void KdbxXmlReader::parseEntryString(Entry* entry)
            QXmlStreamAttributes attr = m_xml.attributes();
            value = readString();

+            if (m_error) {
+                raiseError(tr("Unable to decrypt entry string"));


This discards the randomStream error.

That's intended to preserve the previous behavior and return another message in this case. Should I just remove this? In that case, also remove the translations?

I think it makes more sense to return the actual stream error message.

phoerious · 2018-03-05T20:53:31Z

src/format/KdbxXmlReader.cpp

+            value.clear();
+            raiseError(m_randomStream->errorString());
+        } else {
+            value = QString::fromUtf8(plaintext);


return after raiseError and remove else.

phoerious · 2018-03-05T22:58:44Z

share/translations/keepassx_ar.ts

@@ -2346,10 +2346,6 @@ This is a one-way migration. You won&apos;t be able to open the imported databas
        <source>History element with different uuid</source>
        <translation>سجل العنصر مع uuid مختلف</translation>
    </message>
-    <message>


Please remove all TS files from the PR. We will update them before the release.

phoerious · 2018-03-05T23:02:01Z

src/format/KdbxXmlReader.cpp

@@ -188,6 +188,15 @@ QString KdbxXmlReader::errorString() const
    return QString();
 }

+bool KdbxXmlReader::isTruthyAttribute(const QString& name)


I wouldn't call this isTruthyAttribute. Call is isTrueValue and return true if the string matches any of the True values. Move the attribute logic outside, so we can reuse the method for boolean elements, not just attributes.

phoerious

Looks really good now. Thanks!
Please squash the commits and rebase against release/2.3.1, so I can merge it there.

Similar to KeePass2 most elements are checked for a Protected="True" attribute. Previously, files KDBX 3.1 files with protected Binary entries, as seen in the wild, could not be read (mangled passwords and file attachments).

- Fix unnecessary automatic upgrade to KDBX 4.0 and prevent challenge-response key being stripped [#1568] - Abort saving and show an error message when challenge-response fails [#1659] - Support inner stream protection on all string attributes [#1646] - Fix favicon downloads not finishing on some websites [#1657] - Fix freeze due to invalid STDIN data [#1628] - Correct issue with encrypted RSA SSH keys [#1587] - Fix crash on macOS due to QTBUG-54832 [#1607] - Show error message if ssh-agent communication fails [#1614] - Fix --pw-stdin and filename parameters being ignored [#1608] - Fix Auto-Type syntax check not allowing spaces and special characters [#1626] - Fix reference placeholders in combination with Auto-Type [#1649] - Fix qtbase translations not being loaded [#1611] - Fix startup crash on Windows due to missing SVG libraries [#1662] - Correct database tab order regression [#1610] - Fix GCC 8 compilation error [#1612] - Fix copying of advanced attributes on KDE [#1640] - Fix member initialization of CategoryListWidgetDelegate [#1613] - Fix inconsistent toolbar icon sizes and provide higher-quality icons [#1616] - Improve preview panel geometry [#1609]

phoerious requested changes Mar 5, 2018

View reviewed changes

phoerious added this to the v2.3.1 milestone Mar 5, 2018

phoerious added bug file format labels Mar 5, 2018

phoerious requested changes Mar 5, 2018

View reviewed changes

phoerious approved these changes Mar 6, 2018

View reviewed changes

luzat force-pushed the protected-binaries branch from 73614a8 to 3846495 Compare March 6, 2018 09:56

KdbxXmlReader: Support Protected attribute on most nodes

aebe2d9

Similar to KeePass2 most elements are checked for a Protected="True" attribute. Previously, files KDBX 3.1 files with protected Binary entries, as seen in the wild, could not be read (mangled passwords and file attachments).

phoerious force-pushed the protected-binaries branch from 3846495 to aebe2d9 Compare March 6, 2018 18:03

phoerious changed the base branch from develop to release/2.3.1 March 6, 2018 18:03

phoerious merged commit 4c9dcf5 into keepassxreboot:release/2.3.1 Mar 6, 2018

phoerious mentioned this pull request Mar 6, 2018

Corrupt passwords #1661

Closed

phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KdbxXmlReader: Support Protected attribute on most nodes #1646

KdbxXmlReader: Support Protected attribute on most nodes #1646

luzat commented Mar 5, 2018

luzat commented Mar 5, 2018

phoerious left a comment

phoerious Mar 5, 2018

luzat Mar 5, 2018

phoerious Mar 5, 2018

luzat Mar 5, 2018

phoerious Mar 6, 2018

phoerious Mar 5, 2018

luzat Mar 5, 2018

phoerious Mar 5, 2018

luzat Mar 5, 2018 •

edited

Loading

phoerious Mar 6, 2018

phoerious Mar 5, 2018

phoerious Mar 5, 2018

luzat Mar 5, 2018

phoerious Mar 5, 2018

phoerious Mar 5, 2018

phoerious Mar 5, 2018

phoerious Mar 5, 2018

phoerious left a comment

KdbxXmlReader: Support Protected attribute on most nodes #1646

KdbxXmlReader: Support Protected attribute on most nodes #1646

Conversation

luzat commented Mar 5, 2018

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

luzat commented Mar 5, 2018

phoerious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luzat Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious left a comment

Choose a reason for hiding this comment

luzat Mar 5, 2018 •

edited

Loading