search

singpolyma / openpgp-php

OpenPGP.php is a pure-PHP implementation of the OpenPGP Message Format (RFC 4880).
http://singpolyma.github.io/openpgp-php/
The Unlicense
179 stars 69 forks source link

Fix OpenPGP unarmor should return false if armor contain invalid Base64 char #126

Closed stripthis closed 1 year ago

stripthis commented 2 years ago

Hello Stephen,

Here is a small PR concerning an issue that was reported to us by our auditing team. They recommended to always enable the strict mode of base64_decode() by setting the second argument to true.

While inspecting the PGP message parsing code in passbolt_pro-passbolt_pro_api-
bf1c3e91031c, Cure53 discovered that the internal method unarmor() performs 
insufficient validation of the input. PGP messages can
be “armored”, that is ASCII-encoded, and this is typically done for easier transmission
within regular emails. This ASCII-encoded format consists of a Base64-encoded binary
blob with a special prefix and suffix that indicate the type of the PGP message (generic
message, public key etc.). Passbolt uses armored PGP messages throughout its API to
transmit keys and other PGP-signed messages.

The unarmor() method is used to convert an ASCII-encoded PGP message back to its
binary format. As shown below, the implementation performs rudimentary streamlining of
the input and then extracts the Base64-encoded segment after the ASCII header.
Afterwards, the result is fed to the PHP’s  base64_decode()1  function. If not explicitly
specified, as is the case here, this function will default to a non-strict mode. As a result, it
will ignore invalid Base64 characters, then silently drop them during decoding. This ends
in the function almost always accepting the input and never returning  false. As a
consequence, unarmor() will never return false when a PGP message contains invalid
Base64 characters.

I quickly checked the RFC rfc4880 section 6.4: characters other than those in the table, line breaks, and other white space probably indicate a transmission error, about which a warning message or even a message rejection might be appropriate under some circumstances.. So that behavior seems reasonable to me, but feel free to let us know if you have different views.

Also I wanted to ask, we wanted to add tests for this, but I couldn't find where would be the right place to place them. Can we create another file under tests/openpgp.php to test specific methods of the OpenPGP class?

Thanks again for time and your work. Best regards,

singpolyma commented 1 year ago

test added in 8dfac4bc20b53049529ec68b8655426485846390