639220 - Replace the strcpy function with strlcpy function to avoid possible overflow

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chen Liu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; zh-CN; rv:1.9.1.16) Gecko/20101130 Firefox/3.5.16 (.NET CLR 3.5.30729)
Build Identifier: 

Please take a look at the following code at layout\generic\nsFrame.cpp:


  nsIAtom*    mType;
  char        mNameAbbrev[16];
  char        mName[32];
  nsVoidArray mRules;
};

DR_FrameTypeInfo::DR_FrameTypeInfo(nsIAtom* aFrameType, 
                                   const char* aFrameNameAbbrev, 
                                   const char* aFrameName)
{
  mType = aFrameType;
  strcpy(mNameAbbrev, aFrameNameAbbrev);
  strcpy(mName, aFrameName);
  MOZ_COUNT_CTOR(DR_FrameTypeInfo);
}

The use of two strcpy functions look dangerous. Not sure whether this is exploitable. The trivial patch is to replace them with strlcpy, which makes the code look safer.

Reproducible: Always

Comment 1

[Chen Liu]

Attachment: Replace the strcpy use with strlcpy

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is this supported on all our targets?  My Linux box doesn't seem to have it.

Also, this is debug only code (only compiled in debug builds, and needing some non-default settings to trigger even there) operating on a known fixed set of strings.  So this is not exploitable.

Comment 3

[Chen Liu]

(In reply to comment #2)
Thanks for replying. I'm not sure whether this is supported on all targets. 

Yes, this is debug only code. But I think a simple fix is at least not a bad idea: )

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The right function to use is probably PL_strncpyz.

Comment 5

[Chen Liu]

Attachment: Fix with PL_strncpyz

Thanks. The fix with PL_strncpyz.

Comment 6

[Chen Liu]

Would someone like to take a look at this report?

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's in David's review queue.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The patch looks fine; I just want to leave it in my review queue until I have a chance to land it.  Otherwise I'm more likely to forget.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 517217 [details] [diff] [review]
Fix with PL_strncpyz

r=dbaron

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/5b21d83b089a

Thanks for the patch.

In the future, though, you should develop using a much more up-to-date copy of the Mozilla source code.  See https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial .  We're no longer using CVS, and haven't been for years; it was quite lucky that the patch still applied.