From e6d9bb0774aa19fcef5c6cbde4660626a55b83e4 Mon Sep 17 00:00:00 2001 From: Tim Ledbetter Date: Thu, 5 Oct 2023 17:51:29 +0100 Subject: [PATCH] LibTLS: Don't attempt to read past EOF when parsing TBSCertificate This allows the decoder to fail gracefully when reading a partial or malformed TBSCertificate. We also now ensure that the certificate data is valid before making a copy of it. --- Meta/Lagom/CMakeLists.txt | 1 + Tests/LibTLS/CMakeLists.txt | 1 + Tests/LibTLS/TestTLSCertificateParser.cpp | 15 +++++++++++++++ Userland/Libraries/LibTLS/Certificate.cpp | 6 ++++-- 4 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 Tests/LibTLS/TestTLSCertificateParser.cpp diff --git a/Meta/Lagom/CMakeLists.txt b/Meta/Lagom/CMakeLists.txt index c901519180..5652fdbebf 100644 --- a/Meta/Lagom/CMakeLists.txt +++ b/Meta/Lagom/CMakeLists.txt @@ -655,6 +655,7 @@ if (BUILD_LAGOM) # LibTLS needs a special working directory to find cacert.pem lagom_test(../../Tests/LibTLS/TestTLSHandshake.cpp LibTLS LIBS LibTLS LibCrypto) + lagom_test(../../Tests/LibTLS/TestTLSCertificateParser.cpp LibTLS LIBS LibTLS) # The FLAC tests need a special working directory to find the test files lagom_test(../../Tests/LibAudio/TestFLACSpec.cpp LIBS LibAudio WORKING_DIRECTORY "${FLAC_TEST_PATH}/..") diff --git a/Tests/LibTLS/CMakeLists.txt b/Tests/LibTLS/CMakeLists.txt index 282a940406..7badc93a4f 100644 --- a/Tests/LibTLS/CMakeLists.txt +++ b/Tests/LibTLS/CMakeLists.txt @@ -1,4 +1,5 @@ set(TEST_SOURCES + TestTLSCertificateParser.cpp TestTLSHandshake.cpp ) diff --git a/Tests/LibTLS/TestTLSCertificateParser.cpp b/Tests/LibTLS/TestTLSCertificateParser.cpp new file mode 100644 index 0000000000..407e0067fe --- /dev/null +++ b/Tests/LibTLS/TestTLSCertificateParser.cpp @@ -0,0 +1,15 @@ +/* + * Copyright (c) 2023, Tim Ledbetter + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include + +TEST_CASE(certificate_with_malformed_tbscertificate_should_fail_gracefully) +{ + Array invalid_certificate_data { 0xB0, 0x02, 0x70, 0x00 }; + auto parse_result = TLS::Certificate::parse_certificate(invalid_certificate_data); + EXPECT(parse_result.is_error()); +} diff --git a/Userland/Libraries/LibTLS/Certificate.cpp b/Userland/Libraries/LibTLS/Certificate.cpp index 1e7c8327cc..b5ceb459b8 100644 --- a/Userland/Libraries/LibTLS/Certificate.cpp +++ b/Userland/Libraries/LibTLS/Certificate.cpp @@ -691,10 +691,11 @@ static ErrorOr parse_tbs_certificate(Crypto::ASN1::Decoder& decoder ENTER_TYPED_SCOPE(Sequence, "TBSCertificate"sv); auto post_cert_buffer = TRY(decoder.peek_entry_bytes()); - auto asn1_data = TRY(ByteBuffer::copy(pre_cert_buffer.slice(0, post_cert_buffer.size() + entry_length_byte_count))); + if (pre_cert_buffer.size() < post_cert_buffer.size() + entry_length_byte_count) { + ERROR_WITH_SCOPE("Unexpected end of file"); + } Certificate certificate; - certificate.tbs_asn1 = asn1_data; certificate.version = TRY(parse_certificate_version(decoder, current_scope)).to_u64(); certificate.serial_number = TRY(parse_serial_number(decoder, current_scope)); certificate.algorithm = TRY(parse_algorithm_identifier(decoder, current_scope)); @@ -702,6 +703,7 @@ static ErrorOr parse_tbs_certificate(Crypto::ASN1::Decoder& decoder certificate.validity = TRY(parse_validity(decoder, current_scope)); certificate.subject = TRY(parse_name(decoder, current_scope)); certificate.public_key = TRY(parse_subject_public_key_info(decoder, current_scope)); + certificate.tbs_asn1 = TRY(ByteBuffer::copy(pre_cert_buffer.slice(0, post_cert_buffer.size() + entry_length_byte_count))); if (!decoder.eof()) { auto tag = TRY(decoder.peek());