summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Kalnischkies <david@kalnischkies.de>2019-01-23 20:50:29 +0100
committerDavid Kalnischkies <david@kalnischkies.de>2019-01-23 22:48:16 +0100
commite2965b0b6bdd68ffcad0e06d11755412a7e16e50 (patch)
tree24bac675db43d9e013f2e9481ca4599a350e3f34
parent3734cceb44b02ca4d5ee3c6f5cbfe1e12f17cffb (diff)
Fail on non-signature lines in Release.gpg
The exploit for CVE-2019-3462 uses the fact that a Release.gpg file can contain additional content beside the expected detached signature(s). We were passing the file unchecked to gpgv which ignores these extras without complains, so we reuse the same line-reading implementation we use for InRelease splitting to detect if a Release.gpg file contains unexpected data and fail in this case given that we in the previous commit we established that we fail in the similar InRelease case now.
-rw-r--r--apt-pkg/contrib/gpgv.cc84
-rwxr-xr-xtest/integration/test-cve-2019-3462-Release.gpg-payload43
-rwxr-xr-xtest/integration/test-method-gpgv48
3 files changed, 139 insertions, 36 deletions
diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc
index f6594e62e..be71b1eed 100644
--- a/apt-pkg/contrib/gpgv.cc
+++ b/apt-pkg/contrib/gpgv.cc
@@ -27,6 +27,28 @@
#include <apti18n.h>
/*}}}*/
+
+static bool GetLineErrno(std::unique_ptr<char, decltype(&free)> &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false)/*{{{*/
+{
+ errno = 0;
+ auto lineptr = buffer.release();
+ auto const result = getline(&lineptr, n, stream);
+ buffer.reset(lineptr);
+ if (errno != 0)
+ return _error->Errno("getline", "Could not read from %s", InFile.c_str());
+ if (result == -1)
+ {
+ if (acceptEoF)
+ return false;
+ return _error->Error("Splitting of clearsigned file %s failed as it doesn't contain all expected parts", InFile.c_str());
+ }
+ // We remove all whitespaces including newline here as
+ // a) gpgv ignores them for signature
+ // b) we can write out a \n in code later instead of dealing with \r\n or not
+ _strrstrip(buffer.get());
+ return true;
+}
+ /*}}}*/
static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/
{
std::string out;
@@ -176,6 +198,48 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG,
if (releaseSignature == DETACHED)
{
+ std::unique_ptr<FILE, decltype(&fclose)> detached{fopen(FileGPG.c_str(), "r"), &fclose};
+ if (detached.get() == nullptr)
+ {
+ apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' could not be opened", FileGPG.c_str());
+ local_exit(EINTERNAL);
+ }
+ std::unique_ptr<char, decltype(&free)> buf{nullptr, &free};
+ size_t buf_size = 0;
+ bool open_signature = false;
+ bool found_badcontent = false;
+ size_t found_signatures = 0;
+ while (GetLineErrno(buf, &buf_size, detached.get(), FileGPG, true))
+ {
+ if (open_signature && strcmp(buf.get(), "-----END PGP SIGNATURE-----") == 0)
+ open_signature = false;
+ else if (open_signature == false && strcmp(buf.get(), "-----BEGIN PGP SIGNATURE-----") == 0)
+ {
+ open_signature = true;
+ ++found_signatures;
+ }
+ else if (open_signature == false)
+ found_badcontent = true;
+ }
+ if (found_signatures == 0 && statusfd != -1)
+ {
+ // This is not an attack attempt but a file even gpgv would complain about
+ // likely the result of a paywall which is covered by the gpgv method
+ auto const errtag = "[GNUPG:] NODATA\n";
+ FileFd::Write(fd[1], errtag, strlen(errtag));
+ local_exit(113);
+ }
+ else if (found_badcontent)
+ {
+ apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains lines not belonging to a signature", FileGPG.c_str());
+ local_exit(112);
+ }
+ if (open_signature == true)
+ {
+ apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains unclosed signatures", FileGPG.c_str());
+ local_exit(112);
+ }
+
Args.push_back(FileGPG.c_str());
Args.push_back(File.c_str());
}
@@ -290,26 +354,6 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG,
}
/*}}}*/
// SplitClearSignedFile - split message into data/signature /*{{{*/
-static bool GetLineErrno(std::unique_ptr<char, decltype(&free)> &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false)
-{
- errno = 0;
- auto lineptr = buffer.release();
- auto const result = getline(&lineptr, n, stream);
- buffer.reset(lineptr);
- if (errno != 0)
- return _error->Errno("getline", "Could not read from %s", InFile.c_str());
- if (result == -1)
- {
- if (acceptEoF)
- return false;
- return _error->Error("Splitting of clearsigned file %s failed as it doesn't contain all expected parts", InFile.c_str());
- }
- // We remove all whitespaces including newline here as
- // a) gpgv ignores them for signature
- // b) we can write out a \n in code later instead of dealing with \r\n or not
- _strrstrip(buffer.get());
- return true;
-}
bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile,
std::vector<std::string> * const ContentHeader, FileFd * const SignatureFile)
{
diff --git a/test/integration/test-cve-2019-3462-Release.gpg-payload b/test/integration/test-cve-2019-3462-Release.gpg-payload
new file mode 100755
index 000000000..fd0f96713
--- /dev/null
+++ b/test/integration/test-cve-2019-3462-Release.gpg-payload
@@ -0,0 +1,43 @@
+#!/bin/sh
+set -e
+
+# This is not covered by the CVE and harmless by itself, but used in
+# the exploit and while harmless it is also pointless to allow it
+
+TESTDIR="$(readlink -f "$(dirname "$0")")"
+. "$TESTDIR/framework"
+
+setupenvironment
+configarchitecture 'amd64'
+
+export APT_DONT_SIGN='InRelease'
+
+insertpackage 'unstable' 'foo' 'all' '1'
+setupaptarchive
+rm -rf rootdir/var/lib/apt/lists
+
+verify() {
+ testfailure apt update
+ testsuccess grep '^ Detached signature file' rootdir/tmp/testfailure.output
+ testfailure apt show foo
+}
+
+msgmsg 'Payload after detached signature'
+find aptarchive -name 'Release.gpg' | while read FILE; do
+ cp -a "$FILE" "${FILE}.bak"
+ echo "evil payload" >> "$FILE"
+done
+verify
+
+msgmsg 'Payload in-between detached signatures'
+find aptarchive -name 'Release.gpg' | while read FILE; do
+ cat "${FILE}.bak" >> "$FILE"
+done
+verify
+
+msgmsg 'Payload before detached signature'
+find aptarchive -name 'Release.gpg' | while read FILE; do
+ echo "evil payload" > "$FILE"
+ cat "${FILE}.bak" >> "$FILE"
+done
+verify
diff --git a/test/integration/test-method-gpgv b/test/integration/test-method-gpgv
index 70521881d..bfa5af4c2 100755
--- a/test/integration/test-method-gpgv
+++ b/test/integration/test-method-gpgv
@@ -71,44 +71,60 @@ testrun() {
[GNUPG:] VALIDSIG 891CC50E605796A0C6E733F74BC0A39C27CE74F9 2016-09-01 1472742629 0 4 0 1 11 00 891CC50E605796A0C6E733F74BC0A39C27CE74F9'
}
+echo 'Test' > message.data
+cat >message.sig <<EOF
+-----BEGIN PGP SIGNATURE-----
+
+iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt
+cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l
+3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg
+X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k
+V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx
+pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns
+JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq
+=TB1F
+-----END PGP SIGNATURE-----
+EOF
+
+
gpgvmethod() {
- echo '601 Configuration
+ echo "601 Configuration
Config-Item: Debug::Acquire::gpgv=1
Config-Item: Dir::Bin::apt-key=./faked-apt-key
Config-Item: APT::Hashes::SHA1::Weak=true
600 URI Acquire
-URI: file:///dev/null
-Filename: /dev/zero
-' | runapt "${METHODSDIR}/gpgv"
+URI: file://${TMPWORKINGDIRECTORY}/message.sig
+Filename: ${TMPWORKINGDIRECTORY}/message.data
+" | runapt "${METHODSDIR}/gpgv"
}
testrun
gpgvmethod() {
- echo '601 Configuration
+ echo "601 Configuration
Config-Item: Debug::Acquire::gpgv=1
Config-Item: Dir::Bin::apt-key=./faked-apt-key
Config-Item: APT::Hashes::SHA1::Weak=true
600 URI Acquire
-URI: file:///dev/null
-Filename: /dev/zero
+URI: file://${TMPWORKINGDIRECTORY}/message.sig
+Filename: ${TMPWORKINGDIRECTORY}/message.data
Signed-By: /dev/null,34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE
-' | runapt "${METHODSDIR}/gpgv"
+" | runapt "${METHODSDIR}/gpgv"
}
testrun
gpgvmethod() {
- echo '601 Configuration
+ echo "601 Configuration
Config-Item: Debug::Acquire::gpgv=1
Config-Item: Dir::Bin::apt-key=./faked-apt-key
Config-Item: APT::Hashes::SHA1::Weak=true
600 URI Acquire
-URI: file:///dev/null
-Filename: /dev/zero
+URI: file://${TMPWORKINGDIRECTORY}/message.sig
+Filename: ${TMPWORKINGDIRECTORY}/message.data
Signed-By: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE,/dev/null
-' | runapt "${METHODSDIR}/gpgv"
+" | runapt "${METHODSDIR}/gpgv"
}
testrun
@@ -122,16 +138,16 @@ testsuccess grep '^\s\+Good:\s\+$' method.output
testsuccess grep 'verified because the public key is not available: GOODSIG' method.output
gpgvmethod() {
- echo '601 Configuration
+ echo "601 Configuration
Config-Item: Debug::Acquire::gpgv=1
Config-Item: Dir::Bin::apt-key=./faked-apt-key
Config-Item: APT::Hashes::SHA1::Weak=true
600 URI Acquire
-URI: file:///dev/null
-Filename: /dev/zero
+URI: file://${TMPWORKINGDIRECTORY}/message.sig
+Filename: ${TMPWORKINGDIRECTORY}/message.data
Signed-By: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE!
-' | runapt "${METHODSDIR}/gpgv"
+" | runapt "${METHODSDIR}/gpgv"
}
testgpgv 'Exact matched subkey signed with long keyid' 'Good: GOODSIG 5A90D141DBAC8DAE' '34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE!' '[GNUPG:] GOODSIG 5A90D141DBAC8DAE Sebastian Subkey <subkey@example.org>
[GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2018-08-16 1534459673 0 4 0 1 11 00 4281DEDBD466EAE8C1F4157E5B6896415D44C43E'