230351 - NTLM base64 decoder should tolerate extra '=' padding

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P4)

Description

[Darin Fisher]

NTLM base64 decoder should tolerate extra '=' padding.

IE is apparently quite happy to accept a NTLM Type 2 message that contains an
extra '=' sign, and so some server applications have grown up with a bug wherein
an extra '=' sign is appended.  We should be more tolerant of this kind of thing.

This is perhaps something we'd want to fix in PL_Base64Decode instead of special
casing it in places where PL_Base64Decode is called.

Comment 1

[benc]

Do we have a list of servers that do this? That would help Darin understand how
much of a problem this is.

Comment 2

[Darin Fisher]

Attachment: v1 patch

The docs for PL_Base64Decode state that an error is returned if the input is
not well-coded.  I'm not sure if that implies correct padding or not.  Changing
the behavior of PL_Base64Decode might have unintended side-effects if someone
is/was counting on the behavior of PL_Base64Decode to detect incorrectly padded
input.	So, maybe the least risky thing for us to do is to simply patch some of
the consumers of PL_Base64Decode in the tree... at least the ones that deal
with NTLM input (and SPNEGO for good measure).

Comment 3

[Darin Fisher]

one concern i have is with malformed input leading to the case where |len == 0|
... might want to protect against that better

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm, will NTLM messages always have a size that's a multiple of 3 (after
decoding)? Otherwise you are removing too much padding in some cases...

Comment 5

[Darin Fisher]

(In reply to comment #4)
> hmm, will NTLM messages always have a size that's a multiple of 3 (after
> decoding)? Otherwise you are removing too much padding in some cases...

No, NTLM messages are not going to be a multiple of 3 in size.  My reason for
stripping the padding is based on the documentation of PL_Base64Decode, which
says that "the source may either include or exclude any trailing '='
characters", so I am excluding them.

Note also that in all cases we're computing the allocation size of the output
buffer prior to stripping '=' chars.

I also confirmed that this algorithm works fine when applied to a decoded source
string with length that is not a multiple of 3.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #5)
> No, NTLM messages are not going to be a multiple of 3 in size.  My reason for
> stripping the padding is based on the documentation of PL_Base64Decode, which
> says that "the source may either include or exclude any trailing '='
> characters", so I am excluding them.

ah, ok, I didn't expect that behaviour of PL_Base64Decode.

Comment 7

[Christopher Nebergall]

Comment on attachment 155061 [details] [diff] [review]
v1 patch

Looks fine. Did you have a test case to try this against to make sure that the
case where the challenge was just a set of of equal signs didn't bother it?

Comment 8

[Darin Fisher]

(In reply to comment #7)
> (From update of attachment 155061 [details] [diff] [review])
> Looks fine. Did you have a test case to try this against to make sure that the
> case where the challenge was just a set of of equal signs didn't bother it?
> 

no, but i'll verify that before i commit this patch.

Comment 9

[Darin Fisher]

fixed-on-trunk

Comment 10

[Darin Fisher]

FWIW, the code seemed to handle the case of a challenge consisting of all '='
signs without any trouble.