bug#10536: 23.3; Make base64-decode more fault tolerant

bug#10536: 23.3; Make base64-decode more fault tolerant

[Wolfram Gloger]

Hello,

Not a bug in Emacs, but I have received base64-encoded mails from a
not-so-small company which could not be decoded.  It turns out that
there was a missing padding character, i.e. the last quartet was
"xy=" rather than the proper "xy==".

I would suggest that base64-decode should tolerate this, like with
the appended patch.

Regards,
Wolfram.

--- src/fns.c~	2011-04-05 05:46:44.000000000 +0200
+++ src/fns.c	2012-01-17 13:59:26.000000000 +0100
@@ -3590,7 +3590,8 @@
 
       if (c == '=')
 	{
-	  READ_QUADRUPLET_BYTE (-1);
+	  /* Be tolerant against missing final padding '='.  */
+	  READ_QUADRUPLET_BYTE (e-to);
 
 	  if (c != '=')
 	    return -1;

bug#10536: 23.3; Make base64-decode more fault tolerant

[Lars Ingebrigtsen]

Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> writes:

> Not a bug in Emacs, but I have received base64-encoded mails from a
> not-so-small company which could not be decoded.  It turns out that
> there was a missing padding character, i.e. the last quartet was
> "xy=" rather than the proper "xy==".
>
> I would suggest that base64-decode should tolerate this, like with
> the appended patch.
>
> Regards,
> Wolfram.
>
> --- src/fns.c~	2011-04-05 05:46:44.000000000 +0200
> +++ src/fns.c	2012-01-17 13:59:26.000000000 +0100
> @@ -3590,7 +3590,8 @@
>
>        if (c == '=')
>  	{
> -	  READ_QUADRUPLET_BYTE (-1);
> +	  /* Be tolerant against missing final padding '='.  */
> +	  READ_QUADRUPLET_BYTE (e-to);

It probably won't harm anything to add this patch...  On the other hand,
it's not very common to have base64 encoded data that fails in this
manner.

What do the rest of you people think?  (I think I'm slightly for
applying the patch.  "Be liberal in what you receive" and all that.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#10536: 23.3; Make base64-decode more fault tolerant

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> writes:

>> there was a missing padding character, i.e. the last quartet was
>> "xy=" rather than the proper "xy==".
>>
>> I would suggest that base64-decode should tolerate this, like with
>> the appended patch.

>>        if (c == '=')
>>  	{
>> -	  READ_QUADRUPLET_BYTE (-1);
>> +	  /* Be tolerant against missing final padding '='.  */
>> +	  READ_QUADRUPLET_BYTE (e-to);

> What do the rest of you people think?  (I think I'm slightly for
> applying the patch.  "Be liberal in what you receive" and all that.)

Makes sense to me.

bug#10536: 23.3; Make base64-decode more fault tolerant

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 18 Apr 2018 00:22:42 +0200
> Cc: 10536@debbugs.gnu.org
> 
> > --- src/fns.c~	2011-04-05 05:46:44.000000000 +0200
> > +++ src/fns.c	2012-01-17 13:59:26.000000000 +0100
> > @@ -3590,7 +3590,8 @@
> >
> >        if (c == '=')
> >  	{
> > -	  READ_QUADRUPLET_BYTE (-1);
> > +	  /* Be tolerant against missing final padding '='.  */
> > +	  READ_QUADRUPLET_BYTE (e-to);
> 
> It probably won't harm anything to add this patch...  On the other hand,
> it's not very common to have base64 encoded data that fails in this
> manner.
> 
> What do the rest of you people think?  (I think I'm slightly for
> applying the patch.  "Be liberal in what you receive" and all that.)

Could this "omission" be a sign of malicious stuff in there?  If so,
maybe it's better to introduce a variable that would allow this to be
tolerated, and by default fail with a message telling the user that if
they trust the source of the data, set the variable and retry?

bug#10536: 23.3; Make base64-decode more fault tolerant

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Wed, 18 Apr 2018 00:22:42 +0200
>> Cc: 10536@debbugs.gnu.org
>> 
>> > --- src/fns.c~	2011-04-05 05:46:44.000000000 +0200
>> > +++ src/fns.c	2012-01-17 13:59:26.000000000 +0100
>> > @@ -3590,7 +3590,8 @@
>> >
>> >        if (c == '=')
>> >  	{
>> > -	  READ_QUADRUPLET_BYTE (-1);
>> > +	  /* Be tolerant against missing final padding '='.  */
>> > +	  READ_QUADRUPLET_BYTE (e-to);
>> 
>> It probably won't harm anything to add this patch...  On the other hand,
>> it's not very common to have base64 encoded data that fails in this
>> manner.
>> 
>> What do the rest of you people think?  (I think I'm slightly for
>> applying the patch.  "Be liberal in what you receive" and all that.)
>

That principle is starting to change: "Be intolerant in what you
receive, else malicious actors will exploit your tolerance" is
starting to become more common.

> Could this "omission" be a sign of malicious stuff in there?  If so,
> maybe it's better to introduce a variable that would allow this to be
> tolerated, and by default fail with a message telling the user that if
> they trust the source of the data, set the variable and retry?

You mean that someone would deliberately send incorrect base64 in the
hope that interim attachment scanners would ignore it, but that the
final recipient's software would be tolerant and decode it? That seems
a little farfetched [1], but if it really is uncommon, then a variable
would not do much harm.

Robert

Footnotes:
[1]  Then again, someone just hacked a casino via an aquarium
     thermometer, so who am I to judge whatʼs farfetched.

bug#10536: 23.3; Make base64-decode more fault tolerant

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  wmglo@dent.med.uni-muenchen.de,  10536@debbugs.gnu.org
> Date: Wed, 18 Apr 2018 11:42:52 +0200
> 
> > Could this "omission" be a sign of malicious stuff in there?  If so,
> > maybe it's better to introduce a variable that would allow this to be
> > tolerated, and by default fail with a message telling the user that if
> > they trust the source of the data, set the variable and retry?
> 
> You mean that someone would deliberately send incorrect base64 in the
> hope that interim attachment scanners would ignore it, but that the
> final recipient's software would be tolerant and decode it?

No, I mean that this omission is either due to a bug at the malicious
end or is somehow related to the malicious part itself (i.e. it is
part of the scam).

bug#10536: 23.3; Make base64-decode more fault tolerant

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > --- src/fns.c~	2011-04-05 05:46:44.000000000 +0200
>> > +++ src/fns.c	2012-01-17 13:59:26.000000000 +0100
>> > @@ -3590,7 +3590,8 @@
>> >
>> >        if (c == '=')
>> >  	{
>> > -	  READ_QUADRUPLET_BYTE (-1);
>> > +	  /* Be tolerant against missing final padding '='.  */
>> > +	  READ_QUADRUPLET_BYTE (e-to);

[...]

> Could this "omission" be a sign of malicious stuff in there?

Hm...  I don't think so.  This is about the very last characters of a
base64-encoded text.  The standard says that if there are too few bytes
in the original text, then the base64-encoded thing should be padded
with = signs.

Here's an example:

(base64-encode-string "hel")
=> "aGVs"
(base64-encode-string "hell")
=> "aGVsbA=="
(base64-encode-string "hello")
=> "aGVsbG8="
(base64-encode-string "helloh")
=> "aGVsbG9o"

The proposed patch is that base64-decode should handle the end-padding
and missing end-padding equivalently: That is, both "aGVsbA==" and
"aGVsbA" should decode to "hell".

Unless I'm missing something, I don't think there's much room for
maliciousness here...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#10536: 23.3; Make base64-decode more fault tolerant

[Lars Ingebrigtsen]

Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> writes:

> -	  READ_QUADRUPLET_BYTE (-1);
> +	  /* Be tolerant against missing final padding '='.  */
> +	  READ_QUADRUPLET_BYTE (e-to);

We discussed this a bit last year, but didn't come to any conclusion,
but I think on the whole, having base64-decode be strict or non-strict
here doesn't make much practical difference (because it's very uncommon
to strip the trailing = padding characters), but theoretical paranoia
about this somehow leading to subsequent errors...  somehow...  may be
warranted.

So I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#10536: 23.3; Make base64-decode more fault tolerant

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> writes:
>
>> -	  READ_QUADRUPLET_BYTE (-1);
>> +	  /* Be tolerant against missing final padding '='.  */
>> +	  READ_QUADRUPLET_BYTE (e-to);
>
> We discussed this a bit last year, but didn't come to any conclusion,
> but I think on the whole, having base64-decode be strict or non-strict
> here doesn't make much practical difference (because it's very uncommon
> to strip the trailing = padding characters), but theoretical paranoia
> about this somehow leading to subsequent errors...  somehow...  may be
> warranted.

We did meanwhile get a base64url-* which has an optional NOPAD argument,
and base64-decode-* functions got an optional BASE64URL argument which
allows decoding unpadded base64, by the way.

bug#10536: 23.3; Make base64-decode more fault tolerant

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> We did meanwhile get a base64url-* which has an optional NOPAD argument,
> and base64-decode-* functions got an optional BASE64URL argument which
> allows decoding unpadded base64, by the way.

Ah, yeah, I'd forgotten about that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no