[bugzilla-daemon@xxxxxxxxxxxxx]

http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2495

           Summary: Wireshark does not respect user's umask; inconsistent
                    file permissions when saving
           Product: Wireshark
           Version: SVN
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: Normal
          Priority: Low
         Component: Wireshark
        AssignedTo: wireshark-bugs@xxxxxxxxxxxxx
        ReportedBy: vandry@xxxxxxxxx


Build Information:
[The following build can reproduce the problem but the code looks like it is
the same in SVN]

wireshark 0.99.6

Copyright 1998-2007 Gerald Combs <gerald@xxxxxxxxxxxxx> and contributors.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiled with GTK+ 2.12.0, with GLib 2.14.1, with libpcap 0.9.7, with libz
1.2.3.3, with libpcre 7.4, without Net-SNMP, with ADNS, without Lua, with
GnuTLS
1.6.3, with Gcrypt 1.2.4, with MIT Kerberos, with PortAudio <= V18, without
AirPcap.

Running on Linux 2.6.22-14-server, with libpcap version 0.9.7.

Built using gcc 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2).

--
PROBLEM:

In different circumstances, wireshark will save capture files with
at least three different combinations of file permissions. This
inconsistency leads to results that users cannot predict very well
and will not expect. Furthermore, only one of these combinations
of file permissions is actually correct: the one that respects the
permissions mandated by the user's environment (umask).

Since wireshark is often used in combination with utilities like
"su" because it required privileged access to sniff, users often
attempt to read capture files as regular users that were written
as root. Wrong file permissions can easily interfere with being
able to do this.

BACKGROUND:

When opening a live capture to a temporary file, the try_tempfile()
function from file.c is used. This function manipulates the umask so
the file permissions are always 0600 (owner can read&write,
everybody else can neither read nor write). This is reasonable and
correct: temporary files located in possibly public temporary
directories should use restrictive permissions rather than respecting
the process umask. But when saving a capture that is stored in a
temporary file to a new file, the temporary file will be renamed if
possible, thus preserving these file permissions. They aren't
apropriate for a file which the user wants to keep.

When saving a capture to a given file, the file might be copied with
copy_binary_file() from file.c, which specifies permissions 0644 (which
will be combined with the umask) if no processing is needed. Else, the
file will be opened using wtap_dump_file_open() from
wiretap/file_access.c which uses default unspecified) file permissions.

RESULT:

1. If the result of a live capture is saved to a file without any
filtering and renaming the temporary file was possible, the permissions
will be -rw-------.
2. If the result of a live capture is saved to a file without any
filtering and renaming the temporary file was impossible, the
permissions will be (-rw-r--r--)&(umask).
3. In every other case, the permissions will be (-rw-rw-rw-)&(umask).

Using a typical permissive umask of 022, this leads to
(1) -rw-------, (2) -rw-r--r--, (3) -rw-r--r--

Using a typical restrictive umask of 077, this leads to
(1) -rw-------, (2) -rw-------, (3) -rw-------

Using a very permissive umask of 000, this leads to
(1) -rw-------, (2) -rw-r--r--, (3) -rw-rw-rw-

This last umask is not very common, but it demonstrates that three
different combinations of permissions can be obtained.

TO REPRODUCE:

1. set umask as desired (to 000 for best demonstration)
2. start wireshark
3. capture some packets then stop the capture
4. save the result to a file in a directory on the same filesystem as
   the temporary file. You will obtain case (1) above.
5. save the result again to another file. You will obtain case (2) above.
6. save the result again but this time specify some filtering (such as
   "Specified packet only"). You will obtain case (3) above.

You now have three saved captures. If you used umask 000, they will have
three different sets of file permissions.

RESOLUTION:

There are two bugs.

First bug: copy_binary_files() should create files with permissions that
respect the process umask.

Here is a patch for the first bug:

Index: file.c
===================================================================
--- file.c      (revision 25148)
+++ file.c      (working copy)
@@ -3939,7 +3939,7 @@
      may open the file in text mode, not binary mode, but we want
      to copy the raw bytes of the file, so we need the output file
      to be open in binary mode. */
-  to_fd = eth_open(to_filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
0644);
+  to_fd = eth_open(to_filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
0666);
   if (to_fd < 0) {
     open_failure_alert_box(to_filename, errno, TRUE);
     eth_close(from_fd);

Second bug: Saved capture files should not be given the same file
permissions as temporary files, even if the saved capture file is
just a renamed temporary file.

There are a couple of options to solve this bug. I will implement the
solution but I would like to present the options and find out which
solution the wireshark developers prefer first.

Second bug, option 1: At the moment the file is renamed, use chmod() to
change the permissions to what they should have been if the file had
been created with default permissions to begin with.

Option 1 pro: no change in how temporary files are created and handled, so
easiest, least intrusive.

Option 1 con: reading the process umask() is racy, and explicitely
manipulating file permissions is cumbersome.

Second bug, option 2: Change the way temporary files are handled.
Do not use mkstemp() anymore to create them. The software shall create a
temporary directory with mode 0700 and place all temporary files inside
this directory. The directory itself, just like the temporary files
inside it, shall be deleted when the program exits. The temp files
created inside this directory can be created with the correct permissions
for saving right away, so there is no need to worry about changing them
after rename().

Option 2 pro: clean, and no more playing with the umask. The umask does
its thing and this is transparent to wireshark like it is to most software.

Option 2 con: temporary files created and handled differently. Probably no
users depend on Wireshark's temporary file creation scheme, but if they
did, the change would break them.

My recommendation is option 2.

-Phil


-- 
Configure bugmail: http://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.