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

 
 

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

Submitted by:  None
Submitted on:  Sun 11 Jan 2009 06:14:45 AM UTC  
 
Category: Program LogicSeverity: 6 - Security
Priority: 7 - HighStatus: Need Investigation
Privacy: PublicAssigned to: None
Open/Closed: OpenRelease: None
Fixed Release: NonePlanned Release: 4.1.0
Work Required: None

Add a New Comment (Rich MarkupRich 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)

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>
Project 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>
Project 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>
Project Member
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>
Project Administrator
Sun 11 Jan 2009 06:19:39 AM UTC, comment #1:

The Ubuntu bug: <https://bugs.launchpad.net/screen/+bug/315993>

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 File(s):
   
   
Comment:
   

Attached Files
file #17240:  99rdancer_race_creating_temporary_file.dpatch added by None (3kB - 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
  • -unavailable- added by thoger (Posted a comment)
  • -unavailable- added by mls (Posted a comment)
  • -unavailable- added by micahcowan (Posted a comment)
  • -unavailable- added by jnweiger (Posted a comment)
  •  

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

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 11 Jan 2009 08:11:44 PM UTCmicahcowanCategoryNone=>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)
    Sun 11 Jan 2009 05:06:29 PM UTCjnweigerPrivacy(Error - Not Found)=>Public
    Sun 11 Jan 2009 06:14:45 AM UTCNoneAttached File-=>Added 99rdancer_race_creating_temporary_file.dpatch, #17240

    Back to the top


    Powered by Savane 3.1-cleanup