From 3734cceb44b02ca4d5ee3c6f5cbfe1e12f17cffb Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 23 Jan 2019 17:47:49 +0100 Subject: Fail instead of warn for unsigned lines in InRelease The warnings were introduced 2 years ago without any reports from the wild about them actually appearing for anyone, so now seems to be an as good time as any to switch them to errors. This allows rewritting the code by failing earlier instead of trying to keep going which makes the diff a bit hard to follow but should help simplifying reasoning about it. References: 6376dfb8dfb99b9d182c2fb13aa34b2ac89805e3 --- .../test-cve-2013-1051-InRelease-parsing | 7 ++-- test/libapt/openmaybeclearsignedfile_test.cc | 39 ++++++++-------------- 2 files changed, 19 insertions(+), 27 deletions(-) (limited to 'test') diff --git a/test/integration/test-cve-2013-1051-InRelease-parsing b/test/integration/test-cve-2013-1051-InRelease-parsing index 6238057c3..1f0cbda04 100755 --- a/test/integration/test-cve-2013-1051-InRelease-parsing +++ b/test/integration/test-cve-2013-1051-InRelease-parsing @@ -46,9 +46,12 @@ touch -d '+1hour' aptarchive/dists/stable/InRelease listcurrentlistsdirectory | sed '/_InRelease/ d' > listsdir.lst msgtest 'apt-get update should ignore unsigned data in the' 'InRelease' testwarningequal "Get:1 http://localhost:${APTHTTPPORT} stable InRelease [$(stat -c%s aptarchive/dists/stable/InRelease) B] +Err:1 http://localhost:${APTHTTPPORT} stable InRelease + Splitting up ${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease into data and signature failed Reading package lists... -W: Clearsigned file '${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease' contains unsigned lines. -W: Clearsigned file '${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/localhost:${APTHTTPPORT}_dists_stable_InRelease' contains unsigned lines." --nomsg aptget update +W: An error occurred during the signature verification. The repository is not updated and the previous index files will be used. GPG error: http://localhost:${APTHTTPPORT} stable InRelease: Splitting up ${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease into data and signature failed +W: Failed to fetch http://localhost:${APTHTTPPORT}/dists/stable/InRelease Splitting up ${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease into data and signature failed +W: Some index files failed to download. They have been ignored, or old ones used instead." --nomsg aptget update testfileequal './listsdir.lst' "$(listcurrentlistsdirectory | sed '/_InRelease/ d')" # ensure there is no package diff --git a/test/libapt/openmaybeclearsignedfile_test.cc b/test/libapt/openmaybeclearsignedfile_test.cc index 1f63fb8fc..4c6a0090f 100644 --- a/test/libapt/openmaybeclearsignedfile_test.cc +++ b/test/libapt/openmaybeclearsignedfile_test.cc @@ -190,19 +190,16 @@ TEST(OpenMaybeClearSignedFileTest,TwoSimpleSignedFile) "-----END PGP SIGNATURE-----"); EXPECT_TRUE(_error->empty()); EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); - EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); if (tempfile.empty() == false) unlink(tempfile.c_str()); EXPECT_FALSE(_error->empty()); - EXPECT_TRUE(fd.IsOpen()); - char buffer[100]; - EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); - EXPECT_STREQ(buffer, "Test"); - EXPECT_TRUE(fd.Eof()); - ASSERT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + // technically they are signed, but we just want one message + EXPECT_TRUE(_error->PendingError()); std::string msg; - _error->PopMessage(msg); + EXPECT_TRUE(_error->PopMessage(msg)); EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unsigned lines.", msg); } @@ -244,19 +241,15 @@ TEST(OpenMaybeClearSignedFileTest,GarbageTop) "-----END PGP SIGNATURE-----\n"); EXPECT_FALSE(StartsWithGPGClearTextSignature(tempfile)); EXPECT_TRUE(_error->empty()); - EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); if (tempfile.empty() == false) unlink(tempfile.c_str()); - EXPECT_TRUE(fd.IsOpen()); - char buffer[100]; - EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); - EXPECT_STREQ(buffer, "Test"); - EXPECT_TRUE(fd.Eof()); + EXPECT_FALSE(fd.IsOpen()); ASSERT_FALSE(_error->empty()); - ASSERT_FALSE(_error->PendingError()); + ASSERT_TRUE(_error->PendingError()); std::string msg; - _error->PopMessage(msg); + EXPECT_TRUE(_error->PopMessage(msg)); EXPECT_EQ("Clearsigned file '" + tempfile + "' does not start with a signed message block.", msg); } @@ -313,19 +306,15 @@ TEST(OpenMaybeClearSignedFileTest,GarbageBottom) "Garbage"); EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); EXPECT_TRUE(_error->empty()); - EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); if (tempfile.empty() == false) unlink(tempfile.c_str()); - EXPECT_TRUE(fd.IsOpen()); - char buffer[100]; - EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); - EXPECT_STREQ(buffer, "Test"); - EXPECT_TRUE(fd.Eof()); + EXPECT_FALSE(fd.IsOpen()); ASSERT_FALSE(_error->empty()); - ASSERT_FALSE(_error->PendingError()); + ASSERT_TRUE(_error->PendingError()); std::string msg; - _error->PopMessage(msg); + EXPECT_TRUE(_error->PopMessage(msg)); EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unsigned lines.", msg); } @@ -347,7 +336,7 @@ TEST(OpenMaybeClearSignedFileTest,BogusNoSig) std::string msg; _error->PopMessage(msg); - EXPECT_EQ("Splitting of file " + tempfile + " failed as it doesn't contain all expected parts 0 1 0", msg); + EXPECT_EQ("Splitting of clearsigned file " + tempfile + " failed as it doesn't contain all expected parts", msg); } TEST(OpenMaybeClearSignedFileTest,BogusSigStart) -- cgit v1.2.3 From e2965b0b6bdd68ffcad0e06d11755412a7e16e50 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 23 Jan 2019 20:50:29 +0100 Subject: 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. --- .../test-cve-2019-3462-Release.gpg-payload | 43 +++++++++++++++++++ test/integration/test-method-gpgv | 48 ++++++++++++++-------- 2 files changed, 75 insertions(+), 16 deletions(-) create mode 100755 test/integration/test-cve-2019-3462-Release.gpg-payload (limited to 'test') 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 < [GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2018-08-16 1534459673 0 4 0 1 11 00 4281DEDBD466EAE8C1F4157E5B6896415D44C43E' -- cgit v1.2.3 From 73e3459689c05cd62f15c29d2faddb0fc215ef5e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 23 Jan 2019 22:50:45 +0100 Subject: Merge and reuse tmp file handling across the board Having many rather similar implementations especially if one is exported while others aren't (and the rest of it not factored out at all) seems suboptimal. --- test/integration/test-apt-extracttemplates | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'test') diff --git a/test/integration/test-apt-extracttemplates b/test/integration/test-apt-extracttemplates index 9b07ef79f..a47257cfd 100755 --- a/test/integration/test-apt-extracttemplates +++ b/test/integration/test-apt-extracttemplates @@ -44,6 +44,13 @@ Description: Some bar var testfileequal "$TEMPLATE" "$TEMPLATE_STR" CONFIG=$(cut -f4 -d' ' $OUT) testfileequal "$CONFIG" "$CONFIG_STR" + msgtest 'No extra files or directories in extraction directory' + if [ "$(find ./extracttemplates-out | wc -l)" = '3' ]; then + msgpass + else + msgfail + ls -l ./extracttemplates-out + fi # ensure that the format of the output string has the right number of dots for s in "$CONFIG" "$TEMPLATE"; do -- cgit v1.2.3 From 9b840b59cc80a072e14b8adc9d76669a7a50ab87 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 28 Jan 2019 20:45:02 +0100 Subject: Refuse files with lines unexpectedly starting with a dash We support dash-encoding even if we don't really work with files who would need it as implementations are free to encode every line, but otherwise a line starting with a dash must either be a header we parse explicitly or the file is refused. This is against the RFC which says clients should warn on such files, but given that we aren't expecting any files with dash-started lines to begin with this looks a lot like a we should not continue to touch the file as it smells like an attempt to confuse different parsers by "hiding" headers in-between others. The other slightly more reasonable explanation would be an armor header key starting with a dash, but no existing key does that and it seems unlikely that this could ever happen. Also, it is recommended that clients warn about unknown keys, so new appearance is limited. --- test/libapt/openmaybeclearsignedfile_test.cc | 125 ++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/libapt/openmaybeclearsignedfile_test.cc b/test/libapt/openmaybeclearsignedfile_test.cc index 4c6a0090f..0a4d4438a 100644 --- a/test/libapt/openmaybeclearsignedfile_test.cc +++ b/test/libapt/openmaybeclearsignedfile_test.cc @@ -111,7 +111,6 @@ TEST(OpenMaybeClearSignedFileTest,SignedFileWithContentHeaders) EXPECT_TRUE(fd.Eof()); } -// That isn't how multiple signatures are done TEST(OpenMaybeClearSignedFileTest,SignedFileWithTwoSignatures) { std::string tempfile; @@ -360,3 +359,127 @@ TEST(OpenMaybeClearSignedFileTest,BogusSigStart) _error->PopMessage(msg); EXPECT_EQ("Signature in file " + tempfile + " wasn't closed", msg); } + +TEST(OpenMaybeClearSignedFileTest,DashedSignedFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("dashedsignedfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"\n" +"- Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_TRUE(fd.IsOpen()); + char buffer[100]; + EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); + EXPECT_STREQ(buffer, "Test"); + EXPECT_TRUE(fd.Eof()); +} +TEST(OpenMaybeClearSignedFileTest,StrangeDashArmorFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("strangedashfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"-Hash: SHA512\n" +"\n" +"Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + + std::string msg; + EXPECT_TRUE(_error->PendingError()); + EXPECT_TRUE(_error->PopMessage(msg)); + EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unexpected line starting with a dash (armor)", msg); +} +TEST(OpenMaybeClearSignedFileTest,StrangeDashMsgFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("strangedashfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"\n" +"-Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + + std::string msg; + EXPECT_TRUE(_error->PendingError()); + EXPECT_TRUE(_error->PopMessage(msg)); + EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unexpected line starting with a dash (msg)", msg); +} +TEST(OpenMaybeClearSignedFileTest,StrangeDashSigFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("strangedashfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"\n" +"Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"-/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + + std::string msg; + EXPECT_TRUE(_error->PendingError()); + EXPECT_TRUE(_error->PopMessage(msg)); + EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unexpected line starting with a dash (sig)", msg); +} -- cgit v1.2.3