84155 - Lengthening string in PSM causes N6.1 Beta to crash

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Rose Smyth]

Open file pipnss.properties (in pipnss.jar > PSM.xpi)
Lengthen the LibraryDescription= string by 6 characters.
Try to launch Netscape.
Product crashes with netscp6.exe application error.

This string handling error has serious implications for localisation as text
normally expands by up to 30 per cent when localised. See Bugscape bug no. 5733.

Comment 1

[Linda Baliman]

Adding to cc-list;  adding crash to keywords

Comment 2

[Jaime Rodriguez, Jr.]

Adding loeb and lord to cc: list.

This sounds pretty bad.

Daniel/Rose - Is this a stopper?

Comment 3

[Jeremy Loeb (gone)]

adding ddrinan

Comment 4

[David P. Drinan]

Is a fix for this needed for mozilla 0.9.1?

Comment 5

[David P. Drinan]

Adding ssaux and javi to cc-list.

Comment 6

[David P. Drinan]

Attachment: Patch is fix this crash. Javi, please review.

Comment 7

[Stephane Saux]

ddrinan I believe you fix essentially has the following sequence of calls:
  str = (char *)PR_Malloc(len+1);
	.... (cause tmplen to be determined)
  memcpy(str, tmpstr, tmplen);
  if (len > tmplen) {
That's not correct.
You should not copy tmplen to str if tmplen is > len
Shouldn't the PR_Malloc occur exactly before the memcopy
and the arg should  be tmplen+1.

Comment 8

[Stephane Saux]

Attachment: ssaux proposed patch.

Comment 9

[Javier Delgadillo]

The PKCS11 spec says these string have to be a certain length (the _len_ value
passed in).  That's the reason for the logic.

r=javi on ddrinan's patch.

Comment 10

[Stephane Saux]

I'm ok with is if (tmplen > len) == false always.
otherwise a test must be made to not cause a boundary violation on
the memcpy (ok technically boundary violation only appears if tmplen > len+1).

Comment 11

[David P. Drinan]

I did not catch the offending memcpy as the brower crashes in the 
memset function. The function is to return a string of length 'len'. If the 
string in the properties file is of length greater that 'len', then the function 
should return the first 'len' bytes of the string and ignore the remaining 
characters.

Updated patch to follow.

Comment 12

[David P. Drinan]

Attachment: Updated patch.

Comment 13

[Stephane Saux]

r=ssaux
javi please review too.

Comment 14

[Javier Delgadillo]

r=javi 

Comment 15

[Jaime Rodriguez, Jr.]

David/Rose - Is this a beta stopper? If so, we need to tell the PDT, and change
the TM really fast!

Comment 16

[Phil Peterson]

If the only way to make it crash is to edit this file by hand, it's not a stopper.

Comment 17

[danielmc]

No it is not a stopper for 0.9.1 as we can abreviate the text for German and 
French. 

However his could potentially break *any* localised build. Especially any using 
many accented characters as each accented character = 5 chars in escaped 
unicode. (\uXXXX)

Setting target milestone to 0.9.2

Comment 18

[Jaime Rodriguez, Jr.]

Thanks for the feedback Danny. I feel much better about mocing it to M0.9.2 now.

Comment 19

[Stephane Saux]

Correct version is 2.0
target -> 2.0, because this may result in crashes for RTM localized builds.

Comment 20

[David P. Drinan]

This got fixed by a previous checkin. Marking FIXED.

Comment 21

[John Unruh]

Verified per ddrinan's comment.