[openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
PEM consists of base64 inside a header and trailer line.

OpenSSL crashes with embedded newlines.  This was mentioned to me by the
OpenXPKI project.

See RFC 7468 section 2:

 Data before the encapsulation boundaries are
   permitted, and parsers MUST NOT malfunction when processing such
   data.  Furthermore, parsers SHOULD ignore whitespace and other non-
   base64 characters and MUST handle different newline conventions.

Reproducible with the attached PEM certificate request and OpenSSL 1.02h
(linux).

openssl req -text -in t/csr1.pem
unable to load X509 request
3086379164:error:0906D066:PEM routines:PEM_read_bio:bad end
line:pem_lib.c:809:

This request is valid - although it (intentionally) also exceeds the
standard line length.

Note that OpenSSL will accept it if re-formatted:
| perl -Mwarnings -Mstrict -MMIME::Base64 -e'local $/; my $x = <STDIN>;
$x =~ s/.*^(-----BEGIN CERTIFICATE REQUEST-----\r?\n)(.*)^(-----END
CERTIFICATE REQUEST-----).*/$1 . encode_base64(decode_base64( $2 )) .
$3/ems; print $x' <t/csr1.pem | openssl req -text||
|
OpenSSL should accept PEM with embedded whitespace and long lines.

--
Timothe Litt
ACM Distinguished Engineer
--------------------------
This communication may not represent the ACM or my employer's views,
if any, on the matters discussed.


--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

csr1.pem.txt (4K) Download Attachment
smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
Well, it is a SHOULD not a MUST.  But point taken it could be (much) better :)


--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
On 05-Oct-16 07:52, Salz, Rich via RT wrote:

> Well, it is a SHOULD not a MUST.  But point taken it could be (much) better :)
>
>
It's an important SHOULD.  Whitespace introduction happens in the wild.

This is the quote from the OpenXPKI folks:
> I just saw this today at a customer install that a user uploaded a
> PCSK10 request with extra newlines, anything based on Crypt::PKCS10 is
> happy with it but openssl crashes when it tries to sign.

See https://github.com/openxpki/openxpki/issues/437


--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

[openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
In reply to this post by Rich Salz via RT
To be noted, there's more in section 2:

   Most extant parsers ignore blanks at the ends of lines; blanks at the
   beginnings of lines or in the middle of the base64-encoded data are
   far less compatible.  These observations are codified in Figure 1.
   The most lax parser implementations are not line-oriented at all and
   will accept any mixture of whitespace outside of the encapsulation
   boundaries (see Figure 2).  Such lax parsing may run the risk of
   accepting text that was not intended to be accepted in the first
   place (e.g., because the text was a snippet or sample).

I haven't looked enough in our code recently to remember if we're doing
"standard" (figure 1) or "strict" (figure 3) parsing... what I hear is a
request for us to move to "lax" (figure 2) parsing.

Cheers,
Richard


On Wed Oct 05 12:02:54 2016, [hidden email] wrote:

> On 05-Oct-16 07:52, Salz, Rich via RT wrote:
>
> > Well, it is a SHOULD not a MUST. But point taken it could be (much)
> > better :)
> >
> >
> It's an important SHOULD. Whitespace introduction happens in the
> wild.
>
> This is the quote from the OpenXPKI folks:
> > I just saw this today at a customer install that a user uploaded a
> > PCSK10 request with extra newlines, anything based on Crypt::PKCS10
> > is
> > happy with it but openssl crashes when it tries to sign.
>
> See https://github.com/openxpki/openxpki/issues/437


--
Richard Levitte
[hidden email]

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
On 05-Oct-16 08:56, Richard Levitte via RT wrote:

> To be noted, there's more in section 2:
>
>    Most extant parsers ignore blanks at the ends of lines; blanks at the
>    beginnings of lines or in the middle of the base64-encoded data are
>    far less compatible.  These observations are codified in Figure 1.
>    The most lax parser implementations are not line-oriented at all and
>    will accept any mixture of whitespace outside of the encapsulation
>    boundaries (see Figure 2).  Such lax parsing may run the risk of
>    accepting text that was not intended to be accepted in the first
>    place (e.g., because the text was a snippet or sample).
>
> I haven't looked enough in our code recently to remember if we're doing
> "standard" (figure 1) or "strict" (figure 3) parsing... what I hear is a
> request for us to move to "lax" (figure 2) parsing.
Yes.  Actually, the text is even more lax than the BNF; it says in
paragraph 1 that

   parsers SHOULD ignore whitespace and other non-
   base64 characters

That is, anything but A-Za-z0-9+/ and = at the end (as pad) should be
ignored between the
header and the footer.  Many decoders do that silently, some warn if the
junk isn't whitespace.

Let's step back a bit from the letter of the RFCs and consider what
brought this up:

The real-word issues that drive this are cases like cut and paste of a
CSR, certificate, or key from
a webpage, terminal window or e-mail.  All may re-wrap such that
whitespace is introduced
or lost.

Further, especially with long keys, the text may not all be visible at
once, so one ends up
scrolling and/or copy/pasting in sections.  Again introducing and/or
losing white space.  And exactly how
textboxes on web pages represent EOL and interact with copy/paste
varies.  Lost newlines
can produce long lines, and many base64 encoders (e.g. Perl's
MIME::Base64) produce PEM
 that's longer than 64 characters (e.g. the 72 characters recommended
for MIME.)

CSRs/Certificates/Keys appear on webpages generated by embedded devices
(think NAS, routers), as well
as CAs and terminal windows.  So while one would like to think that
they're never touched by human hands,
the reality is that they are.

I'm not as concerned about "accepting text that was not intended to be
accepted in the first  place"
because validation of the data will occur.  CSRs and certificates are
signed, and will fail validation if
corrupt.  Keys won't work if corrupt.  All have to pass ASN.1 parsing,
which also will catch many forms
of corruption.

OpenSSL should accept the CSR that I posted as a test case.  Whether to
also ignore
non-base64 characters is debatable.  I vote for warning (e.g. a distinct
SUCCESS code that
the caller can elect to report or ignore).

What's fixed is that there must be a "-----BEGIN" line, and there's
little excuse for not having
a "-----END" line, though the newline after the "-----END" may be
optional.  Embedded whitespace
must be ignored - which includes that line length is unrestricted.  This
is something that
both humans with a mouse and software can comprehend...

The approach I use is to discard all whitespace, check for only base64 +
optional pad &
ensure that the length, including 0-2 pad (=) at the end is an even
multiple of 4 characters
long.  Otherwise, (non-base64 or not a sane length) I warn but process
the input.  (A Perl
implementation is in the OpenXPKI issue that I cited.)

Naturally, I am NOT arguing that PEM can be produced in lax form; this
is only about
making the input parsing compatible with (RFC-compliant) cases common in
the real world.

I hope this provides context for your decisions...

Timothe Litt
ACM Distinguished Engineer
--------------------------
This communication may not represent the ACM or my employer's views,
if any, on the matters discussed.



--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
In reply to this post by Rich Salz via RT
On 10/05/2016 07:56 AM, Richard Levitte via RT wrote:

> To be noted, there's more in section 2:
>
>    Most extant parsers ignore blanks at the ends of lines; blanks at the
>    beginnings of lines or in the middle of the base64-encoded data are
>    far less compatible.  These observations are codified in Figure 1.
>    The most lax parser implementations are not line-oriented at all and
>    will accept any mixture of whitespace outside of the encapsulation
>    boundaries (see Figure 2).  Such lax parsing may run the risk of
>    accepting text that was not intended to be accepted in the first
>    place (e.g., because the text was a snippet or sample).
>
> I haven't looked enough in our code recently to remember if we're doing
> "standard" (figure 1) or "strict" (figure 3) parsing... what I hear is a
> request for us to move to "lax" (figure 2) parsing.
>

If I remember correctly, it's somewhere in between.  The core
PEM-parsing code is vintage EAY, and contains some "interesting"
behavior, like going to the end of the line/buffer that was read,
backtracking past any characters with ASCII value less than or equal to
that of <space>, and writing \n\0.  So, it seems like trailing
whitespace would be ignored, but leading whitespace would trip up the
"len == 65" check later on.

I refactored this stuff a while ago to add a flags field that would
force the temporary read buffer to be allocated from the secure heap; I
should really dig it up and clean it up for master.

-Ben

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
In reply to this post by Rich Salz via RT
One more reference: https://tools.ietf.org/html/rfc4648#section-3.3
describes the considerations for 'non-base64 characters'.

Short form: MIME requires that they be ignored. 7468 says SHOULD.
4648 says 'reject, unless the referencing spec says otherwise' (which
7468 does.)

I wrote previously that MIME's limit on line length is 72; according to
4648 3.1 it's actually 76.  Sorry.  The point is, it's NOT 64 (which is what
PEM specifies.).  (65 in OpenSSL must include the end-of-line.)

Note that all 3 constants are (deliberately) a multiple of 4, meaning that
the decoding of a byte can't span lines.  However, this is not true in
the wild;
end-of-line can appear anywhere. (Again, wrapping by MUAs, web browsers
and embedded devices are the most frequent offenders.)

Here's the full text of 3.3:

>    Base encodings use a specific, reduced alphabet to encode binary
>    data.  Non-alphabet characters could exist within base-encoded data,
>    caused by data corruption or by design.  Non-alphabet characters may
>    be exploited as a "covert channel", where non-protocol data can be
>    sent for nefarious purposes.  Non-alphabet characters might also be
>    sent in order to exploit implementation errors leading to, e.g.,
>    buffer overflow attacks.
>
>    Implementations MUST reject the encoded data if it contains
>    characters outside the base alphabet when interpreting base-encoded
>    data, unless the specification referring to this document explicitly
>    states otherwise.  Such specifications may instead state, as MIME
>    does, that characters outside the base encoding alphabet should
>    simply be ignored when interpreting data ("be liberal in what you
>    accept").  Note that this means that any adjacent carriage return/
>    line feed (CRLF) characters constitute "non-alphabet characters" and
>    are ignored.  Furthermore, such specifications MAY ignore the pad
>    character, "=", treating it as non-alphabet data, if it is present
>    before the end of the encoded data.  If more than the allowed number
>    of pad characters is found at the end of the string (e.g., a base 64
>    string terminated with "==="), the excess pad characters MAY also be
>    ignored.
>

Timothe Litt
ACM Distinguished Engineer
--------------------------
This communication may not represent the ACM or my employer's views,
if any, on the matters discussed.



--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4698] PEM parsing incorrect; whitespace in PEM crashes parser

Rich Salz via RT
In reply to this post by Rich Salz via RT
On 10/05/2016 09:15 AM, Kaduk, Ben via RT wrote:
> I refactored this stuff a while ago to add a flags field that would
> force the temporary read buffer to be allocated from the secure heap; I
> should really dig it up and clean it up for master.

That's https://github.com/openssl/openssl/pull/1700 , FWIW.

-Ben

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4698
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev