[PATCH common 0/3] pem parser: handle missing newline edge case

[PATCH common 0/3] pem parser: handle missing newline edge case

[Thomas Ellmenreich]

After the previous series: [0] was already applied, I realized that the
parsing ignores a specific edgecase. Currently the header does not force
a newline, and neither does the footer, only the content does. This means
that something like the following is allowed and this series addresses
this edgecase.

    -----BEGIN CERTIFICATE-----TG9yZW0gaXBzdW0=
    -----END CERTIFICATE-----

Since I began changing the header and footer regexes, I wrote some tests
to also fix the functionality of pem_to_der, which is when I noticed that
the above certificate is decoded to "<gibberish bytes>Lorem ipsum" instead
of just "Lorem ipsum".

In the case of pem_to_der I have decided to introduce a clearer error
boundary, meaning that we die quickly instead of returning gibberish. I
further restricted the allowed characters of a label so that the label
can't just glob up everything.

    This example would be interpreted as one long header, and then die,
    saying that a footer is missing, which might be misleading.

               v - label begins here       ...        ends here - v
    -----BEGIN CERTIFICATE-----TG9yZW0gaXBzdW0=-----END CERTIFICATE-----
                          ^ *here

    By limiting the label characters it will stop looking for a label
    *here and then just say it can't find a header as there is no newline.

Open Questions
--------------

 - There is one edge case I did not properly cover. This occurs when a chain
   like the following is passed to the pem_to_der subroutine:

   v -----BEGIN CERTIFICATE-----
   . TG9yZW0gaXBzdW0=
   . -----END CERTIFICATE----------BEGIN CERTIFICATE-----
   . TG9yZW0gaXBzdW0=
   ^ -----END CERTIFICATE-----

   In this case, everything between the first and last line is passed to
   the decoding function. That function stops decoding as soon as the first
   '=' is encountered, so the result is actually correct.

   I have added a test for this, and deemed it not necessary to fix, as it
   would add a lot of unnecessary complexity, and works as it is now.


[0]: https://lore.proxmox.com/all/20260805141114.190477-1-t.ellmenreich@proxmox.com/T/#u


pve-common:

Thomas Ellmenreich (3):
  pem to der: make header and footer parsing stricter
  tests: introduce tests for pem_to_der
  check pem: force at least one newline after header and footer

 src/PVE/Certificate.pm  |  15 ++--
 test/Makefile           |   1 +
 test/check_pem_test.pl  |  77 ++++++++++++++++++-
 test/pem_to_der_test.pl | 161 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 247 insertions(+), 7 deletions(-)
 create mode 100755 test/pem_to_der_test.pl


Summary over all repositories:
  4 files changed, 247 insertions(+), 7 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH common 1/3] pem to der: make header and footer parsing stricter

[Thomas Ellmenreich]

When parsing a PEM to then decode it to a DER, the header and footer
regex could previously lead to the header being included in the
decoded content as is the case with the following example:

    -----BEGIN CERTIFICATE-----TG9yZW0gaXBzdW0=
    -----END CERTIFICATE-----

Since the subroutine's focus is not to validate the structure but just to
decode the content, create a clearer error boundary by die'ing quickly if
no header or footer is found.

Additionally, there was an edgecase where the regex for the label would
glob up the whole PEM. A stricter label regex avoids globbing up the whole
PEM and can then be used to produce a more meaningful error.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/Certificate.pm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
index 589bce8..6985d75 100644
--- a/src/PVE/Certificate.pm
+++ b/src/PVE/Certificate.pm
@@ -183,14 +183,17 @@ sub check_pem {
 sub pem_to_der {
     my ($content) = @_;
 
-    my $header = $header_re->(qr/.*?/);
-    my $footer = $footer_re->(qr/.*?/);
+    # RFC 7468 allows a single dash, so only dissallow 2 consecutive dashes
+    my $header = $header_re->(qr/(?:(?!--).)+?/);
+    my $footer = $footer_re->(qr/(?:(?!--).)+?/);
 
     $content = strip_leading_text($content);
 
     # only take first PEM entry
-    $content =~ s/^$header$//mg;
-    $content =~ s/$footer.*//sg;
+    $content =~ s/^$header$//mg
+        or die "Could not convert PEM to DER, no header found.\n";
+    $content =~ s/^$footer.*//ms
+        or die "Could not convert PEM to DER, no footer found.\n";
 
     $content = decode_base64($content);
 
-- 
2.47.3

[PATCH common 2/3] tests: introduce tests for pem_to_der

[Thomas Ellmenreich]

To pin down the functionality of the pem_to_der subroutine, introduce a
small testsuite. Also reduces the risk to future changes in subroutines
used by the pem_to_der subroutine.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 test/Makefile           |   1 +
 test/pem_to_der_test.pl | 161 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 162 insertions(+)
 create mode 100755 test/pem_to_der_test.pl

diff --git a/test/Makefile b/test/Makefile
index 9caba10..f111511 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -19,6 +19,7 @@ TESTS = lock_file.test			\
 	ticket_test.test \
 	check_pem_test.test \
 	split_pem_test.test \
+	pem_to_der_test.test \
 
 all:
 
diff --git a/test/pem_to_der_test.pl b/test/pem_to_der_test.pl
new file mode 100755
index 0000000..c35e1f4
--- /dev/null
+++ b/test/pem_to_der_test.pl
@@ -0,0 +1,161 @@
+#!/usr/bin/perl
+# Tests the PVE::Certificate::pem_to_der function for
+# correctness and coverage of edgecases.
+use strict;
+use warnings;
+
+use lib '../src';
+
+use Test::More;
+
+use PVE::Certificate;
+
+# Arrange
+my $setup = [
+    {
+        name => "single pem",
+        pem => q{-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQsIGNvbnNlY3RldHVyIGFkaXBpc2Np
+bmcgZWxpdCwgc2VkIGRvIGVpdXNtb2QgdGVtcG9yIGluY2lkaWR1bnQgdXQgbGFi
+b3JlIGV0IGRvbG9yZSBtYWduYSBhbGlxdWEuIFV0IGVuaW0gYWQgbWluaW0gdmVu
+aWFtLCBxdWlzIG5vc3RydWQgZXhlcmNpdGF0aW9uIHVsbGFtY28gbGFib3JpcyBu
+aXNpIHV0IGFsaXF1aXAgZXggZWEgY29tbW9kbyBjb25zZXF1YXQuIER1aXMgYXV0
+ZSBpcnVyZSBkb2xvciBpbiByZXByZWhlbmRlcml0IGluIHZvbHVwdGF0ZSB2ZWxp
+dCBlc3NlIGNpbGx1bSBkb2xvcmUgZXUgZnVnaWF0IG51bGxhIHBhcmlhdHVyLiBF
+eGNlcHRldXIgc2ludCBvY2NhZWNhdCBjdXBpZGF0YXQgbm9uIHByb2lkZW50LCBz
+dW50IGluIGN1bHBhIHF1aSBvZmZpY2lhIGRlc2VydW50IG1vbGxpdCBhbmltIGlk
+IGVzdCBsYWJvcnVtLg==
+-----END CERTIFICATE-----
+},
+        expected_decoded_result =>
+            "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.",
+    },
+    {
+        name => "single pem no terminating newline",
+        pem => q{-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0=
+-----END CERTIFICATE-----},
+        expected_decoded_result => "Lorem ipsum",
+    },
+    {
+        name => "multiple pem",
+        pem => q{-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMQ==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMg==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMw==
+-----END CERTIFICATE-----
+},
+        expected_decoded_result => "Lorem ipsum 1",
+    },
+    {
+        name => "multiple pem leading newline",
+        pem => q{
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMQ==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMg==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMw==
+-----END CERTIFICATE-----
+},
+        expected_decoded_result => "Lorem ipsum 1",
+    },
+    {
+        name => "multiple pem leading text",
+        pem => q{
+some leading
+text that is expected
+to be stripped
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMQ==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMg==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMw==
+-----END CERTIFICATE-----
+},
+        expected_decoded_result => "Lorem ipsum 1",
+    },
+    {
+        name => "multiple pem interleaved text",
+        pem => q{
+some leading text
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMQ==
+-----END CERTIFICATE-----
+some leading text
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMg==
+-----END CERTIFICATE-----
+some leading text
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMw==
+-----END CERTIFICATE-----
+some end text
+},
+        expected_decoded_result => "Lorem ipsum 1",
+    },
+    {
+        name => "multiple pem no newline after footer",
+        pem => q{
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMQ==
+-----END CERTIFICATE----------BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMg==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0gMw==
+-----END CERTIFICATE-----
+},
+        expected_decoded_result => "Lorem ipsum 1",
+    },
+    {
+        name => "all in one line",
+        pem => "-----BEGIN CERTIFICATE-----TG9yZW0gaXBzdW0=-----END CERTIFICATE-----",
+        expected_error => "header",
+    },
+    {
+        name => "only newline after begin",
+        pem => q{-----BEGIN CERTIFICATE-----
+TG9yZW0gaXBzdW0=-----END CERTIFICATE-----},
+        expected_error => "footer",
+    },
+    {
+        name => "only newline after content",
+        pem => q{-----BEGIN CERTIFICATE-----TG9yZW0gaXBzdW0=
+-----END CERTIFICATE-----},
+        expected_error => "header",
+    },
+    {
+        name => "only newline after end",
+        pem => q{-----BEGIN CERTIFICATE-----TG9yZW0gaXBzdW0=-----END CERTIFICATE-----
+},
+        expected_error => "header",
+    },
+];
+
+for my $data ($setup->@*) {
+    subtest $data->{name} => sub {
+        my $to_der_result = eval { PVE::Certificate::pem_to_der($data->{pem}) };
+        my $to_der_error = $@;
+        if (defined($data->{expected_decoded_result})) {
+            is($to_der_result, $data->{expected_decoded_result}, "has correct decoded result");
+        } else {
+            like(
+                $to_der_error,
+                qr/Could not convert PEM to DER, no $data->{expected_error} found./,
+                "has died with expected error",
+            );
+        }
+    }
+}
+
+done_testing();
-- 
2.47.3

[PATCH common 3/3] check pem: force at least one newline after header and footer

[Thomas Ellmenreich]

Previously, the newlines after a header and footer were optional, only
the content enforced a newline, which meant that the validation didn't
catch an erroneous PEM's.

Extend the header and footer regexes to account for this, as well as
expecting the start of a line before each. In the case of the footer
the following newline can also be the end of the string.

Fixes: 2e8941e ("fix #5978: pem parser: relax parsing of chain entries")
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/Certificate.pm |  8 ++---
 test/check_pem_test.pl | 77 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
index 6985d75..7072c21 100644
--- a/src/PVE/Certificate.pm
+++ b/src/PVE/Certificate.pm
@@ -106,11 +106,11 @@ PVE::JSONSchema::register_standard_option(
 
 my $header_re = sub {
     my ($label) = @_;
-    return qr!-----BEGIN\ $label-----(?:\s|\n)*!;
+    return qr!(?m:^)-----BEGIN\ $label-----(?:\s|\n)*\n(?:\s|\n)*!;
 };
 my $footer_re = sub {
     my ($label) = @_;
-    return qr!-----END\ $label-----(?:\s|\n)*!;
+    return qr!(?m:^)-----END\ $label-----(?:\s|\n)*(?:$|\n)!;
 };
 my $pem_re = sub {
     my ($label) = @_;
@@ -190,9 +190,9 @@ sub pem_to_der {
     $content = strip_leading_text($content);
 
     # only take first PEM entry
-    $content =~ s/^$header$//mg
+    $content =~ s/$header//mg
         or die "Could not convert PEM to DER, no header found.\n";
-    $content =~ s/^$footer.*//ms
+    $content =~ s/$footer.*//ms
         or die "Could not convert PEM to DER, no footer found.\n";
 
     $content = decode_base64($content);
diff --git a/test/check_pem_test.pl b/test/check_pem_test.pl
index 4917d2b..b47a54d 100755
--- a/test/check_pem_test.pl
+++ b/test/check_pem_test.pl
@@ -28,6 +28,32 @@ Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
 SAAwRQIhANfakefakefakefakefakefakefakefakefake
 -----END CERTIFICATE-----
 EOF
+    },
+    {
+        expected_success => 1,
+        name => "full pem no terminating newline",
+        pem => q{-----BEGIN CERTIFICATE-----
+MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
+AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
+c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
+EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
+ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
+0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
+MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
+Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
+SAAwRQIhANfakefakefakefakefakefakefakefakefake
+-----END CERTIFICATE-----},
+        expected_result => q{-----BEGIN CERTIFICATE-----
+MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
+AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
+c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
+EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
+ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
+0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
+MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
+Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
+SAAwRQIhANfakefakefakefakefakefakefakefakefake
+-----END CERTIFICATE-----}
     },
     {
         expected_success => 1,
@@ -68,7 +94,7 @@ test
 EOF
     },
     {
-        expected_success => 1,
+        expected_success => 0,
         name => "only newline pem",
         pem => <<'EOF',
 -----BEGIN CERTIFICATE-----
@@ -152,6 +178,22 @@ EOF
 -----BEGIN CERTIFICATE-----
 42JlZ2tseTxzZWl2IGF3ZWlidm4=
 -----END CERTIFICATE-----
+EOF
+    },
+    {
+        expected_success => 0,
+        name => "missing newline pem",
+        multiple => 1,
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----12JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE----------BEGIN CERTIFICATE-----
+
+
+
+
+
+22JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
 EOF
     },
     {
@@ -175,6 +217,39 @@ dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
 c2JlZ2tseTxzZWl2IGF3ZWlidm4=
 -----END CERTIFICATE-----
 EOF
+    },
+    {
+        expected_success => 1,
+        name => "multiple pem with no terminating newline",
+        multiple => 1,
+        pem => q{Subject: CN=some.test.content
+Issuer: CN=Org Issuing CA 3
+-----BEGIN CERTIFICATE-----
+1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+Subject: CN=Org Issuing CA 3
+Issuer: CN=Org Root CA
+-----BEGIN CERTIFICATE-----
+2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----},
+        expected_result => q{-----BEGIN CERTIFICATE-----
+1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----}
     },
     {
         expected_success => 1,
-- 
2.47.3