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. --- apt-pkg/contrib/gpgv.cc | 46 ++++++++-- test/libapt/openmaybeclearsignedfile_test.cc | 125 ++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 10 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 6e4e9b3df..0b595fc4c 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -297,6 +297,14 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (found_signatures != 0) break; } + else if (buf.starts_with("-")) + { + // the used Radix-64 is not using dash for any value, so a valid line can't + // start with one. Header keys could, but no existent one does and seems unlikely. + // Instead it smells a lot like a header the parser didn't recognize. + apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains unexpected line starting with a dash", FileGPG.c_str()); + local_exit(112); + } } if (found_signatures == 0 && statusfd != -1) { @@ -452,6 +460,10 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, return false; if (buf.empty()) break; // empty line ends the Armor Headers + if (buf.starts_with("-")) + // § 6.2 says unknown keys should be reported to the user. We don't go that far, + // but we assume that there will never be a header key starting with a dash + return _error->Error("Clearsigned file '%s' contains unexpected line starting with a dash (%s)", InFile.c_str(), "armor"); if (ContentHeader != nullptr && buf.starts_with("Hash: ")) ContentHeader->push_back(buf.str()); } @@ -463,17 +475,28 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, if (buf.readFrom(in.get(), InFile) == false) return false; - if (buf == "-----BEGIN PGP SIGNATURE-----") + if (buf.starts_with("-")) { - if (buf.writeTo(SignatureFile) == false) - return false; - break; + if (buf == "-----BEGIN PGP SIGNATURE-----") + { + if (buf.writeTo(SignatureFile) == false) + return false; + break; + } + else if (buf.starts_with("- ")) + { + // we don't have any fields which need to be dash-escaped, + // but implementations are free to escape all lines … + if (buf.writeTo(ContentFile, first_line == false, false, 2) == false) + return false; + } + else + // § 7.1 says a client should warn, but we don't really work with files which + // should contain lines starting with a dash, so it is a lot more likely that + // this is an attempt to trick our parser vs. gpgv parser into ignoring a header + return _error->Error("Clearsigned file '%s' contains unexpected line starting with a dash (%s)", InFile.c_str(), "msg"); } - - // we don't have any fields which need to be dash-escaped, - // but implementations are free to escape all lines … - auto offset = buf.starts_with("- ") ? 2 : 0; - if (buf.writeTo(ContentFile, first_line == false, false, offset) == false) + else if (buf.writeTo(ContentFile, first_line == false, false) == false) return false; first_line = false; } @@ -491,6 +514,11 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, open_signature = true; else if (open_signature == false) return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); + else if (buf.starts_with("-")) + // the used Radix-64 is not using dash for any value, so a valid line can't + // start with one. Header keys could, but no existent one does and seems unlikely. + // Instead it smells a lot like a header the parser didn't recognize. + return _error->Error("Clearsigned file '%s' contains unexpected line starting with a dash (%s)", InFile.c_str(), "sig"); if (buf.writeTo(SignatureFile) == false) return false; 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