bugGNU Screen - Bugs: bug #25296, [Patch] Insecure creation of...

 
 

bug #25296: [Patch] Insecure creation of /tmp/screen-exchange (symlink attack)

Submitter:  None
Submitted:  Sun 11 Jan 2009 06:14:45 AM UTC
   
 
Category:  Program Logic Severity:  6 - Security
Priority:  * 7 - High Status:  Need Investigation
Privacy:  Public Assigned to:  None
Open/Closed:  Open Release:  None
Fixed Release:  None Planned Release:  4.1.0
Work Required:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 02 Apr 2009 03:28:09 PM UTC, comment #6: 

Ok, so the debian folks created a security hole which they have to fix. Not our issue, though.

If you set the screen exchange files to something different than the "public" file, you should make sure that it's not a public writeable directory. The same is true for all files screen writes, like screen dumps. $(HOME)/screen-exchange would be a good idea.

IMHO the debian folks should just put
    bufferfile $HOME/screen-exchange
in their /etc/screenrc file and not mess with the source.

Regarding the symlink attack: yep, but that's really theoretical. Maybe we should use O_NOFOLLOW where available.

Michael Schröder <mls>
Group administrator
Thu 02 Apr 2009 02:44:39 PM UTC, comment #5: 

One thing you may be missing is Debian bug:
  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=433338

Which resulted in addition of the following:

+/**
+ * I consider this 'public exchange file' feature a bad idea, at least
+ * if this is the default case. I really don't like the idea of an admin
+ * copying configuration file snippets around that might get changed
+ * in that buffer file.
+ * Drop the notion of a 'public' exchange file. If someone explictly wants
+ * to share his copy buffer contents, he is free to chmod it after its
+ * creation.
+ * Jan Nordholz <hesso@pool.math.tu-berlin.de>, 2007/08/07
       public = !strcmp(fn, DEFAULT_BUFFERFILE);
+ */

This was to avoid having exchange file with permission 0666, but accidentally introduced symlink issue for default exchange file, as the checks are less strict for non-public exchange files.

Thinking more about this, even without this change, one can possibly shoot himself in the foot by setting bufferfile to some non-default value, that is in a world-writeable directory (e.g. /tmp or /var/tmp).  So the file will be treated as non-public, and fewer checks will be done before opening/creating it.

There also seem to be a theoretical race between lstat and open in the public code path.  If attacker unlinks his screen-exchange file, victim creates new file on same filesystem, it may get the same dev/inode number as the original attacker's screen-exchange.  If attacker symlinks it, test comparing lstat and fstat output may succeed, even the file is different.  All those steps (unlink, create, symlink) need to happen in between lstat and open, while the attacker does not have a control over "create" and the created file is not likely to be a valuable target.

Tomas Hoger <thoger>
Wed 01 Apr 2009 12:21:02 PM UTC, comment #4: 

I really don't see a security issue:

1) The code in question opens the file just with O_WRONLY, thus it is neither created (missing O_CREAT) nor changed (missing O_TRUNC). Afterwards it is made sure that the file is the same as returned in the previous lstat().
2) The other path uses O_EXCL|O_CREAT to make sure that a new file is created
3) mode 666 is only used for the "public" file used for collaboration. If you don't like that behavior, just change the file to $HOME/.screen-exchange. Screen will not use mode 666 in that case.

But maybe I'm missing something else?

Michael Schröder <mls>
Group administrator
Sun 11 Jan 2009 08:11:44 PM UTC, comment #3: 

I don't know about any new use cases. I don't think this is anything to do with what the intended use cases are anyway (except that the original use case renders (a) not a bug, AFAICT).

If I understand correctly, the point is that if screen is susceptible to symlink attacks, then someone could set up /tmp/screen-exchange as a symlink to /usr/bin/ssh (say), and screen would happily overwrite the real /usr/bin/ssh (if screen is setuid root) with, say, a shell script that logs passwords. Or even just delete a sensitive write-protected file somewhere.

Micah Cowan <micahcowan>
Sun 11 Jan 2009 05:06:29 PM UTC, comment #2: 

/tmp/screen-exchange was intended to be an exchange buffer for multiple users working on the same machine.

Therefore we implemented:
a) a predictable filename
b) a world read/write-able file.

Apparently it now used for different purposes. Unfortunatly, documentation was not very clear about the intended use case.
It only indicates once that we have a 'public accessible screen-exchange file'.


Juergen Weigert <jnweiger>
Group administrator
Sun 11 Jan 2009 06:19:39 AM UTC, comment #1: 
Anonymous
Sun 11 Jan 2009 06:14:45 AM UTC, original submission:  

The code responsible for the error does: open("/tmp/screen-exchange", O_WRONLY, 0666);

(a) the file is created with default permissions, depending on the user umask value, which means world-readable under default install settings
(b) Symlink attack is possible. There used to be a race condition, but it seems it was removed when #433338 was closed, as the fix didn't really fix the problem. Now there is no race condition, and any pre-existing symbolic link will result in a file overwrite/creation.

I have created a patch that fixes the problem. The changelog shows three previous patches related to /tmp/screen-exchange. I have commented the source so that future readers can understand better what is going on, and there is hopefully no regression.

I have tested the patched version, and it works under all conditions. The patch applies with an offset to current Debian unstable version (4.0.3-11), and I can only presume it would work.

The patch is against the Ubuntu package, manual merge will need to be done against the upstream, because it is applied over the Debian and Ubuntu patches.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #17240:  99rdancer_race_creating_temporary_file.dpatch added by None (3KiB - application/octet-stream - Create file securely, set 0600 permissions. Patch against the Ubuntu sources)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by thoger (Posted a comment)
  • -email is unavailable- added by mls (Posted a comment)
  • -email is unavailable- added by micahcowan (Posted a comment)
  • -email is unavailable- added by jnweiger (Posted a comment)
  •  

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-01-11 micahcowan CategoryNone Program Logic
        Severity3 - Normal 6 - Security
        Priority5 - Normal 7 - High
        StatusNone Need Investigation
        Planned ReleaseNone 4.1.0
        SummaryInsecure creation of /tmp/screen-exchange (symlink attack) [Patch] Insecure creation of /tmp/screen-exchange (symlink attack)
    2009-01-11 jnweiger PrivacyNone Public
    2009-01-11 None Attached File- Added 99rdancer_race_creating_temporary_file.dpatch, #17240

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code