bugGnash - The GNU Flash player - Bugs: bug #34903, CVE-2011-4328: gnash creates...

 
 

bug #34903: CVE-2011-4328: gnash creates world-readable cookies under /tmp with predictable filenames

Submitted by:  Gabriele Giacone <gg0>
Submitted on:  Sat 26 Nov 2011 03:40:28 AM UTC  
 
Category: pluginSeverity: 3 - Normal
Release: masterStatus: Fixed
Privacy: PublicAssigned to: Gabriele Giacone <gg0>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 06 Dec 2011 12:13:08 AM UTC, comment #9:

Pushed 8fc19a89.

pere, brad0: please install boost::iostreams on buildbot slaves.

Gabriele Giacone <gg0>
Project MemberIn charge of this item.
Thu 01 Dec 2011 01:47:14 AM UTC, comment #8:

> A "%s" is missing in the format string.

Fixed.

> When does the fd get closed?

Changed never_close_handle to close_handle. fd will be closed when sink will be.

> What happens if mkstemps fails? Boost doesn't appear to document what happens if an invalid fd is passed.

Added some checks. Quite sure code duplication can be avoided better.

cookiefile branch at git://github.com/gg0/gnash.git

Gabriele Giacone <gg0>
Project MemberIn charge of this item.
Mon 28 Nov 2011 09:06:11 AM UTC, comment #7:

Actually there are a couple more things:

When does the fd get closed?

What happens if mkstemps fails? Boost doesn't appear to document what happens if an invalid fd is passed.

Benjamin Wolsey <bwy>
Project Member
Mon 28 Nov 2011 08:39:34 AM UTC, comment #6:

I haven't tested whether it works, but the code looks good to me. The only thing left that I can see is:

gnash::log_error("Failed to open new file for standalone launcher: ", debugname);

A "%s" is missing in the format string.

Benjamin Wolsey <bwy>
Project Member
Sun 27 Nov 2011 09:17:27 PM UTC, comment #5:

Attached new version. Thanks for reviewing.

(file #24475)

Gabriele Giacone <gg0>
Project MemberIn charge of this item.
Sun 27 Nov 2011 07:53:18 AM UTC, comment #4:

mkstemps() modifies the passed argument. The return from stringstream::str().c_str() is a const char* for a good reason: it's not modifiable.

To do it properly you need to copy the string to a modifiable buffer and pass that instead, so that the buffer then contains the actual filename.

Also, the code needs to be in a separate function to avoid duplicating it and to make it easier for non-POSIX systems to implement.

Creating your own random filenames has lots of pitfalls (e.g. the lower-order bits are often much less random in rand(), so using the modulo operator like the patch does produces much more predictable values). That's why I'd prefer mkstemps.

Benjamin Wolsey <bwy>
Project Member
Sun 27 Nov 2011 07:25:41 AM UTC, comment #3:

Boost version is better. GNU extensions aren't portable.

Benjamin Wolsey <bwy>
Project Member
Sun 27 Nov 2011 02:33:58 AM UTC, comment #2:

Attached new patch w/o boost, with __gnu_cxx::stdio_filebuf.

(file #24472)

Gabriele Giacone <gg0>
Project MemberIn charge of this item.
Sat 26 Nov 2011 04:11:29 AM UTC, comment #1:

2nd one is broken, launcher doesn't contain correct cookie file name. Probably better working on umask+randomizing names.

Gabriele Giacone <gg0>
Project MemberIn charge of this item.
Sat 26 Nov 2011 03:40:28 AM UTC, original submission:

Forwarding http://bugs.debian.org/649384

Attached 2 patches. One moves cookies/launchers under $HOME/.gnash, proposed few days ago [0]. Second one doesn't move them, randomizes file names and makes them 600 with mkstemp(). I don't really like it cause I had to add boost libs to the plugin, but it's the best I've managed to do so far.
Please review and feel free to push better ones.

[0] http://bugs.debian.org/649384#49

Gabriele Giacone <gg0>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #24475:  mkstemp2.diff added by gg0 (4KiB - text/x-patch)
file #24472:  stdio_filebuf.diff added by gg0 (3KiB - text/x-patch)
file #24459:  movetohomedir.diff added by gg0 (3KiB - text/x-patch)
file #24460:  mkstemp.diff added by gg0 (4KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by bwy (Posted a comment)
  • -unavailable- added by gg0 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 15 Dec 2011 02:00:43 PM UTCgg0StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Tue 06 Dec 2011 12:13:08 AM UTCgg0StatusIn Progress=>Ready For Test
    Sun 27 Nov 2011 09:17:27 PM UTCgg0Attached File-=>Added mkstemp2.diff, #24475
      Assigned toNone=>gg0
    Sun 27 Nov 2011 02:33:58 AM UTCgg0Attached File-=>Added stdio_filebuf.diff, #24472
      StatusNone=>In Progress
    Sat 26 Nov 2011 03:40:28 AM UTCgg0Attached File-=>Added movetohomedir.diff, #24459
      Attached File-=>Added mkstemp.diff, #24460

    Back to the top


    Powered by Savane 3.1-cleanup1