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

Submitter:  Gabriele Giacone <gg0>
Submitted:  Sat 26 Nov 2011 03:40:28 AM UTC
   
 
Category:  plugin Severity:  3 - Normal
Release:  master Status:  Fixed
Privacy:  Public Assigned to:  gg0
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
Sun 27 Nov 2011 09:17:27 PM UTC, comment #5: 

Attached new version. Thanks for reviewing.

(file #24475)

Gabriele Giacone <gg0>
Group Member
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>
Group Member
Sun 27 Nov 2011 07:25:41 AM UTC, comment #3: 

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

Benjamin Wolsey <bwy>
Group 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>
Group Member
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>
Group Member
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>
Group Member

 

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

Attach Files:
   
   
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
  • -email is unavailable- added by bwy (Posted a comment)
  • -email is unavailable- added by gg0 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-12-15 gg0 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2011-12-06 gg0 StatusIn Progress Ready For Test
    2011-11-27 gg0 Attached File- Added mkstemp2.diff, #24475
        Assigned toNone gg0
    2011-11-27 gg0 Attached File- Added stdio_filebuf.diff, #24472
        StatusNone In Progress
    2011-11-26 gg0 Attached File- Added movetohomedir.diff, #24459
        Attached File- Added mkstemp.diff, #24460

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code