From 000fa9d3cfb610799fce1f9b86df0a4e85b4f92f Mon Sep 17 00:00:00 2001 From: Marcus Brinkmann Date: Thu, 28 Jun 2018 04:00:50 +0200 Subject: [PATCH] Improve the error handling of the OpenPGP parser. --- lib/parser/openpgp.cpp | 90 +++++++++++++++++++++++++++++++----- lib/parser/openpgp.h | 13 +++++- lib/parser/openpgp_tests.cpp | 10 ++-- 3 files changed, 96 insertions(+), 17 deletions(-) diff --git a/lib/parser/openpgp.cpp b/lib/parser/openpgp.cpp index f3a9270d8..2383ed24e 100644 --- a/lib/parser/openpgp.cpp +++ b/lib/parser/openpgp.cpp @@ -23,6 +23,9 @@ struct state { size_t packet_pos; std::unique_ptr header; + // The exception object if a packet could not be parsed. + std::unique_ptr exc; + // The length of the current packet body (or part of it). size_t packet_len; @@ -42,25 +45,61 @@ struct packet_body_data { template class Action, template class Control, typename Input> static bool match(Input& in, state& st) { - if (in.size(st.packet_len) >= st.packet_len) { + size_t available = in.size(st.packet_len); + if (available >= st.packet_len) { in.bump(st.packet_len); return true; + } else { + uint32_t max = RawPacketParser::MAX_PARSER_BUFFER; + available = in.size(max); + if (st.packet_len > max && available == max) { + // Best we can do at this point is to skip over the packet and set an + // error. + uint32_t skip = st.packet_len; + while (skip > 0) { + assert(skip >= available); + in.bump(available); + in.discard(); + skip -= available; + uint32_t skip_this = max; + if (skip < skip_this) skip_this = skip; + available = in.size(skip_this); + if (available < skip_this) { + st.exc = make_unique(parser_error( + "packet too short (while skipping too large packet)", in)); + skip = available; + // Fallthrough for one last iteration. + } + } + if (!st.exc) + st.exc = + make_unique(parser_error("packet too large", in)); + st.packet_len = 0; + return true; + } else { + in.bump(available); + in.discard(); + st.packet_len = 0; + st.exc = make_unique(parser_error("packet too short", in)); + return true; + } } + // Not reached. return false; } -}; +}; // namespace openpgp -// We discard the input buffer so that we do not need to account for the packet -// or length header in the max buffer size calculation (so we can use a power of -// two rather than a power of two plus a small number of bytes). +// We discard the input buffer so that we do not need to account for the +// packet or length header in the max buffer size calculation (so we can use a +// power of two rather than a power of two plus a small number of bytes). struct packet_body : seq {}; // For old packets of indeterminate length, we set an arbitrary chunk size with // the packet_body rule, and finish up with packet_body_data_rest. #define INDETERMINATE_LENGTH_CHUNK_SIZE 8192 -// We do not discard the input buffer here, as we come here after back-tracking -// from packet_body, and we want to preserve the data. +// We do not discard the input buffer here, as we come here after +// back-tracking from packet_body, and we want to preserve the data. struct packet_body_data_rest : until {}; struct packet_body_indeterminate @@ -114,8 +153,8 @@ struct new_packet_data_definite new_packet_length_five>, packet_body> {}; -// New packet data always ends with a definite length part, optionally preceeded -// by an arbitrary number of partial data parts. +// New packet data always ends with a definite length part, optionally +// preceeded by an arbitrary number of partial data parts. struct new_packet_data : seq, new_packet_data_definite> {}; @@ -135,7 +174,7 @@ struct is_packet : at> {}; struct packet : seq> {}; // OpenPGP consists of a sequence of packets. -struct grammar : until {}; +struct grammar : seq, must> {}; template struct action : nothing {}; @@ -273,6 +312,7 @@ struct action { static void apply(const Input& in, state& st) { st.packet_type = PacketType::Reserved; st.header.reset(nullptr); + st.exc.reset(nullptr); st.partial = false; st.started = false; } @@ -287,13 +327,24 @@ struct action { if (!st.started) { if (!st.partial) { - st.sink.next_packet(std::move(st.header), data, length); + if (st.exc) + st.sink.error_packet(std::move(st.header), std::move(st.exc)); + else + st.sink.next_packet(std::move(st.header), data, length); } else { + // At this point, we don't support error packets for partial packets, + // because we can't skip them easily. The semantics would be unclear. + if (st.exc) throw *st.exc; + st.sink.start_packet(std::move(st.header)); st.sink.continue_packet(data, length); } st.started = true; } else { + // At this point, we don't support error packets for partial packets, + // because we can't skip them easily. The semantics would be unclear. + if (st.exc) throw *st.exc; + if (st.partial) { st.sink.continue_packet(data, length); } else { @@ -319,8 +370,23 @@ struct action { } }; +// Control +template +struct control : pegtl::normal { + static const std::string error_message; + + template + static void raise(const Input& in, States&&...) { + throw parser_error(error_message, in); + } +}; + +template <> +const std::string control::error_message = "input has trailing data"; + } // namespace openpgp +// FIXME: Pass filename to ParserInput (everywhere). void RawPacketParser::process(Botan::DataSource& source) { using reader_t = std::function; @@ -333,7 +399,7 @@ void RawPacketParser::process(Botan::DataSource& source) { }; buffer_input input("reader", MAX_PARSER_BUFFER, reader); - parse(input, state); + parse(input, state); } void RawPacketParser::process(std::istream& source) { diff --git a/lib/parser/openpgp.h b/lib/parser/openpgp.h index 6142d44ce..e4ce78cc4 100644 --- a/lib/parser/openpgp.h +++ b/lib/parser/openpgp.h @@ -5,6 +5,7 @@ #pragma once +#include #include #include @@ -31,6 +32,15 @@ class NEOPG_UNSTABLE_API RawPacketSink { // (only valid for new packet format).. virtual void finish_packet(std::unique_ptr length_info, const char* data, size_t length) = 0; + + // Error while parsing a packet (usually if a packet is too large for the + // input buffer, or if the final packet is truncated). The packet content is + // skipped, and processing can continue. Takes ownership of HEADER. + virtual void error_packet(std::unique_ptr header, + std::unique_ptr error) = 0; + + // Prevent memory leak when upcasting in smart pointer containers. + virtual ~RawPacketSink() = default; }; class NEOPG_UNSTABLE_API RawPacketParser { @@ -44,7 +54,8 @@ class NEOPG_UNSTABLE_API RawPacketParser { // our own limits, see the NeoPG OpenPGP profile. Post-quantum cryptography // will require some large key packets, so this limit will go up eventually. // For partial length data packets, GnuPG emits at most 8KiB. - const size_t MAX_PARSER_BUFFER = 1024 * 1024; // 1 MiB. + // Photo ids can be much larger. + static const size_t MAX_PARSER_BUFFER = 4 * 1024 * 1024; // 4 MiB RawPacketParser(RawPacketSink& sink) : m_sink(sink) {} diff --git a/lib/parser/openpgp_tests.cpp b/lib/parser/openpgp_tests.cpp index f355b9d3a..08f294c3f 100644 --- a/lib/parser/openpgp_tests.cpp +++ b/lib/parser/openpgp_tests.cpp @@ -39,10 +39,12 @@ class TestSink : public RawPacketSink { m_packets.emplace_back(std::move(packet)); } - void start_packet(std::unique_ptr header){}; - void continue_packet(const char* data, size_t length){}; + void start_packet(std::unique_ptr header) {} + void continue_packet(const char* data, size_t length) {} void finish_packet(std::unique_ptr length_info, - const char* data, size_t length){}; + const char* data, size_t length) {} + void error_packet(std::unique_ptr header, + std::unique_ptr exc) {} }; TEST(NeopgTest, parser_openpgp_test) { @@ -53,7 +55,7 @@ TEST(NeopgTest, parser_openpgp_test) { { std::stringstream data; - auto packet = RawPacket{PacketType::Reserved, "reserved"}; + RawPacket packet{PacketType::Reserved, "reserved"}; packet.write(data); packets.clear();