42782 - basic auth sends password with a trailing null

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Anthony Baxter]

Basic auth sends the base64 encoded username:password with a trailing
null. Naughty.

For username 'anthony', password 'anthony', older mozillas and
netscape send:
YW50aG9ueTphbnRob255
anthony:anthony

But mozilla sends
YW50aG9ueTphbnRob255AA==
anthony:anthony\0

This has probably been able to escape detection because most
servers are written in C :)


This is in 2000061510, and a number of others from the last two weeks.

Comment 1

[Anthony Baxter]

I think the problem's in nsBasicAuth, why is it +2, not +1? 

Comment 2

[Martijn Pieters]

Shaver changed the call PL_Base64Encode from 

  PL_Base64Encode(tempBuff, 0, nsnull); 

to 

  PL_Base64Encode(tempBuff, length, nsnull);

thus not letting PL_Base64Encode figure out the string length with PL_strlen. As
'length' is the length allocated for the 'user:password' string, this includes
the trailing null. Either above call should revert to

  PL_Base64Encode(tempBuff, 0, nsnull);

again, or it should be called as

  PL_Base64Encode(tempBuff, length - 1, nsnull); // Length minus trailing null

Or there should be a check for the trailing null, adjusting the length
appropriatly.

CC-ing shaver for feedback on change.

Comment 3

[Mike Shaver (:shaver emeritus)]

I'll take this one, and attach a compiles, should-work, haven't yet tested patch
for review.

Can someone put an URL in this bug that I can use to test it?

Comment 4

[Martijn Pieters]

URL with basic auth (lowercase 'basic' =)):

  http://www.zopatista.com/manage

user basic_auth, password basic_auth

Comment 5

[Mike Shaver (:shaver emeritus)]

OK, I'm going to attach a patch to 42008 that should fix this as well.  Thanks,
MJ!

Comment 6

[Gagan]

*** Bug 43010 has been marked as a duplicate of this bug. ***

Comment 7

[Martijn Pieters]

Set dependency to remind ppl of the fact that the patch for this bug is attached
to bug #42008.

Comment 8

[Anthony Baxter]

This is now fixed (the patch applied for 42008 also fixes
this). Can/Should I mark it as fixed, or is that for the
QA types to do?

Comment 9

[Mike Shaver (:shaver emeritus)]

Long dead, thanks to blizzard.

Comment 10

[Tom Everingham]

verified