157994 - Code in in nsDataObj.cpp prone to buffer overflow , and generally awful

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Simon Fraser [no longer active]]

The code in nsDataObj::BuildPlatformHTML(), which is called when copying to the
clipboard on Windows, has a slew of evil strcpy, strcat and wsprintf calls.

It's also bug-prone, if the copied content contains "<!--EndFrag".

Comment 1

[jag (Peter Annema)]

What ain't broke ...

Comment 2

[jag (Peter Annema)]

Actually, I take that back. This should be fixed. It's just not high on my list
of things to fix, so if there are any volunteers out there ... :-)

Comment 3

[Dean Tessman]

There's no overflow risk here.  We allocate 400 bytes _plus_ the length of the
HTML, which is more than enough to handle the start and end fragments.  The
concern about a pre-existing "<!-- EndFragment -->" is valid, and can certainly
be handled better than we are currently.

Comment 4

[Dean Tessman]

Actually, i think we do this wrong anyway.  StartFragment:<n> should point to
the first character _after_ "<!-- StartFragment -->".  If I copy text from Moz
and paste it into an HTML message in Outlook Express, the source contains "<!--
StartFragment -->".

Filed bug 158103 for that, to be fixed after this is fixed.

Comment 5

[Simon Fraser [no longer active]]

It would be so easy to rewrite the code to use simple, safe nsString routines,
and eliminate all the strcat, and strstr calls. This code will show up as a
hotspot in security code reviews (since the tools look for 'dangerous' routines
like strcpy, strcat, and routines in the printf family). Do yourself a favour;
rewrite the code, and get it off the radar.

Comment 6

[Mike Pinkerton (not reading bugmail)]

sure, rewrite it. it's nasty code anyway and i'm not terribly proud of it ;)

Comment 7

[Dean Tessman]

But if we re-write it, how will we blame Pinkerton for this nasty code?  :)

Comment 8

[timeless]

Attachment: draft

well, we can blame me for uglier code :)

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 91848 [details] [diff] [review]
draft

oh god no please

Comment 10

[Daniel Veditz [:dveditz]]

Is the crash in bug 158002 related? The stack pasted looks like garbage, but it
does mention pasting. Others have had difficulty reproducing it, though.

Comment 11

[timeless]

ok, sfraser wants this to use mozilla string classes. blah

Comment 12

[Simon Fraser [no longer active]]

Attachment: Untested patch

Untested patch more along the lines of what I was thinking.

Comment 13

[Dean Tessman]

+  const char* const numPlaceholder  = "00000000";
+  const char* const startHTMLPrefix = "Version:0.9\r\nStartHTML:";
+  const char* const endHTMLPrefix   = "\r\nEndHTML:";
+  const char* const startFragPrefix = "\r\nStartFragment:";
+  const char* const endFragPrefix   = "\r\nEndFragment:";
+  const char* const endFragTrailer  = "\r\n";

Why two consts?

Is there any chance of commenting the code that builds the final version.  It's
not ugly as the current version, but it's still a little confusing to try and
read through.

Comment 14

[Simon Fraser [no longer active]]

  const char* const testString = "foopy";
  testString ++;      // compile error
  *testString = 0;    // compile error

  const char* testString2 = "foopy";
  testString2 ++;     // OK!
  *testString2 = 0;   // compile error

Comment 15

[Dean Tessman]

  char* testString2 = "foopy";
  testString2 ++;     // OK!
  *testString2 = 0;   // OK!

Comment 16

[Simon Fraser [no longer active]]

  char* testString2 = "foopy";
  *testString2 = 0;   // OK!

yes, but this will probably crash your program. You might have got a compiler
error, or at least a warning, on the first line.

Comment 17

[jag (Peter Annema)]

|const char* testString = "foopy"| is what we typically use; since "foopy" may
end up in read-only memory, it's safest to reflect that in your pointer by
saying the pointer is of type |const char| and thus whatever it points to can't
be changed. However, the pointer itself could still be changed in this case,
e.g. to point to another literal. By marking it |const char* const testString =
"foopy"| you're not only saying that the literal text can't be changed, you're
also saying that you have no intention of changing the pointer. These |const|s
will help the compiler catch any mistakes you or someone else makes
(|*testString = 0;|) and it allows the compiler to optimize the pointer away
where possible.

Comment 18

[Simon Fraser [no longer active]]

That's exactly what I was trying to demonstrate by example. Thank you, jag!

Comment 19

[timeless]

Comment on attachment 91866 [details] [diff] [review]
Untested patch

so does this work, and are we ok with it?

Comment 20

[Dean Tessman]

Do you want to address bug 158103 at the same time?  All you'd need to do is
change how you're calculating startFragOffset.

Comment 21

[timeless]

well since jag isn't working on this, and dean responded first :)

Comment 22

[jag (Peter Annema)]

Comment on attachment 91866 [details] [diff] [review]
Untested patch

Couldn't we do something here with nsPrintfCString("%08u", len)?

Comment 23

[Daniel Atallah]

Attachment: Proposed patch to cleanup and fix the win32 HTML Clipboard population

At Dean Tessman's suggestion, I incorporated the bug fix for Bug 158103 as well
as the cleanup for this bug (Bug 157994) into a patch.

This is based on timeless' patch with changes to fix 158103 and to use the
cleaner nsPrintfCString() that jag suggested.

Comment 24

[Dean Tessman]

Comment on attachment 158507 [details] [diff] [review]
Proposed patch to cleanup and fix the win32 HTML Clipboard population

>+/*
>+  nsDependentCString headerString(
>+        "Version:0.9\r\n"
>+        "StartHTML:00000000\r\n"      // offset of <html>
>+        "EndHTML:00000000\r\n"        // offset of end of </html>
>+        "StartFragment:00000000\r\n"  // offset of end of <!--StartFragment-->
>+        "EndFragment:00000000\r\n"    // offset of <!--EndFrag
>+      );
>+*/

Remove this.

>+  nsCAutoString numString;
>+  numString.AppendInt(startHTMLOffset);

This isn't used anywhere.

>+  nsCString   clipboardString;

One too many spaces here.

>+  *outPlatformHTML = ToNewCString(clipboardString);printf("outPlatformHTML='%s'\n", outPlatformHTML);

Remove the printf.

Other than that it looks much better than what we have.  r=me

Comment 25

[Daniel Atallah]

Attachment: Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

I incorporated the changes suggested by Dean.
(the printf wasn't supposed to be in the final version of the previous patch!)

Comment 26

[Dean Tessman]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

Carrying forward flags.

Comment 27

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

+  nsDependentCString htmlHeaderString("<html><body>\r\n");

NS_NAMED_LITERAL_CSTRING(htmlHeaderString, "<html>...."); is slightly faster
(calculates length at compile time)

Comment 28

[Dean Tessman]

Jag, are you OK with the patch if the change in comment 27 is made?

Comment 29

[Dean Tessman]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

Do we want this on the aviary branch as well?

Comment 30

[Daniel Atallah]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

I think that this is important to also get into the Aviary-1.0 branch and
SeaMonkey, particularly because it fixes Bug 158103.

Comment 31

[Asa Dotzler [:asa]]

we may still approve the patch, but not going to block on this. 

Comment 32

[Asa Dotzler [:asa]]

jst says this isn't critical for the branch. dean, what's the risk here?

Comment 33

[Dean Tessman]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

(In reply to comment #32)
> jst says this isn't critical for the branch. dean, what's the risk here?

I agree that it's not critical, but I thought I'd ask.	Clearing branch request
flags.

Comment 34

[Daniel Atallah]

I'm not familiar with the criteria for determining what is considered critical
and what isn't, but I would venture to say that in my opinion, this fix should
go into the aviary branch because the current win32 HTML copying functionality
is somewhat broken.

Bug 158103 describes the issues that it suffers from in detail, but basically,
if what is placed into the HTML copy buffer is pasted directly, there is a
garbage "<!--StartFragment-->" at the beginning of the pasted segment. 
Thankfully most applications just ignore it because it is a HTML comment, but it
is still pasted.

Don't we want to have fully functional win32 copy/paste functionality in FireFox
1.0?

Comment 35

[Dean Tessman]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

> Don't we want to have fully functional win32 copy/paste functionality in
> FireFox
1.0?

I forgot about that part of the fix.  I was concentrating only on the potential
overflow.

The copied HTML doesn't mess up the HTML output at all, since
<!--StartFragment--> is a self-contained comment.  That being said, I don't
think we want to get lambasted for propogating that comment into web pages,
emails, etc.

Putting back approval requests to let someone with decision-making power flex
said power.

Comment 36

[Mike Kaply [:mkaply]]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

a=mkaply for the branches.

Don't forget to dupe the other bug to this.

Has this patch gone in on the trunk?

Comment 37

[Dean Tessman]

(In reply to comment #36)
> Has this patch gone in on the trunk?

Not that I've seen.

Comment 38

[timeless]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

mozilla/widget/src/windows/nsDataObj.cpp 1.64
mozilla/widget/src/windows/nsDataObj.cpp 1.59.16.1

Comment 39

[Daniel Atallah]

Bump... (reminder that this still needs to be committed/merged to the AVIARY
branch).

(FF 1.0 release date is fast approaching and i'm looking forward to having this
in there)

Comment 40

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

> mozilla/widget/src/windows/nsDataObj.cpp 1.59.16.1

since this fixed this on the 1.7 branch, adding keyword.

should this bug be marked fixed now that the patch is checked in, or is there
more to do?

Comment 41

[Dean Tessman]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

This hasn't gone in on the aviary branch.  We had approval once but didn't get
it checked in in time.	Re-requesting approval for 1.0.

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 158549 [details] [diff] [review]
Update of proposed patch to cleanup and fix the win32 HTML Clipboard population

too late for 1.0

Comment 43

[Daniel Atallah]

(In reply to comment #41)
> (From update of attachment 158549 [details] [diff] [review])
> This hasn't gone in on the aviary branch.  We had approval once but didn't get
> it checked in in time.	Re-requesting approval for 1.0.
> 

Can this fixe be merged to the aviary branch now so that this bug and Bug 158103
can be closed? (or do we want to just close them because it has been committed
to HEAD?)

Comment 44

[Daniel Atallah]

The fix has been committed to everywhere that it needs to be at this time, as
far as I can tell.