search

motumas / tunnelblick

Automatically exported from code.google.com/p/tunnelblick
0 stars 0 forks source link

Security problem (race condition in SUID code) #212

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Today on full-discloure an email appeared reporting an hole in tunnelblick.
Here's the link to a copy of the email: 
http://seclists.org/fulldisclosure/2012/Aug/122
It contains an exploit that uses a race condition in a tunnelbrick's SUID code 
to gain local root.
I'm not the original reporter of the bug, i'd just want to let you know this in 
the case you missed it.

Original issue reported on code.google.com by ctrlal...@gmail.com on 11 Aug 2012 at 9:41

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Also, this is not sufficient:

void errorExitIfAttackViaString(NSString * string)
{
    BOOL startsWithDot = [string hasPrefix: @"."];
    NSRange r = [string rangeOfString: @"/.."];
    if (   startsWithDot
        || (r.length != 0)  ) {
        fprintf(stderr, "Tunnelblick openvpnstart: Apparent attack detected; string being tested is %s\n", [string UTF8String]);
        [pool drain];
        exit(EXIT_FAILURE);
    }
}

Because of symlinks.

Which means it's probably possible to abuse the "deleteLogs" function to delete 
things as root.

What are we left with? killOneOpenVpn and killAllOpenVpn. In these, there's a 
race condition between checking process names and actually killing the process, 
so depending on what is to get killed, there could be some situations where an 
attacker could send SIGTERM to various ephemeral processes.

I guess I should also point out that allowing users to load OpenVPN as root 
with a user-controlled config file is also a vector, since OpenVPN itself can 
be told not to drop privs, and execute various things on various events.

Original comment by Jason.Donenfeld on 11 Aug 2012 at 3:19

GoogleCodeExporter commented 9 years ago 
So okay, a small summary for CVEs and whatnot:

1. A race condition in file permissions checking can lead to local root.

2. Insufficient checking of merely 0:0 744 can lead to local root on systems 
with particular configurations.

3. Insufficient validation of path names can allow for arbitrary kernel module 
loading, which can lead to local root.

4. Insufficient validation of path names can allow execution of arbitrary 
scripts as root, leading to local root.

5. Insufficient path validation in errorExitIfAttackViaString can lead to 
deletion of  files as root, leading to DoS.

6. Allowing OpenVPN to run with user given configurations can lead to local 
root.

I think at this point, rather than trying to put a band aid on each of these, 
it's probably best to rework the security architecture of the program.

Original comment by Jason.Donenfeld on 11 Aug 2012 at 3:23

GoogleCodeExporter commented 9 years ago 
Professional Cocoa Application Security, http://www.amazon.com/dp/0470525959. 
Graham Lee offers code to elevate privileges, check paths, etc when using the 
Cocoa framework.

Lee also has a blog with lots of sample code at 
http://blog.securemacprogramming.com/.

Original comment by noloa...@gmail.com on 11 Aug 2012 at 3:24

GoogleCodeExporter commented 9 years ago 
Left one out.

7. Race condition in process killing.

Original comment by Jason.Donenfeld on 11 Aug 2012 at 3:37

GoogleCodeExporter commented 9 years ago 
(I'm the current Tunnelblick developer, having taken over from Angelo Laub, the 
original author, several years ago.)

Thanks Jason, for your very helpful comments. And thanks to ctrlaltca for 
submitting this, and to noloaderfor the book reference.

I am not sure how to "rework the security architecture" but I'll look at 
noloader's book.

Everything (so far!) comes down to five problems:

1. Allowing unsecured paths to configuration and other files (even if the files 
themselves are 0:0 744, all their containing folders must also be writable only 
by root).
2. Insufficient validation of paths.
3. Race condition in process killing.
4. Bug in process killing
5. Bug in log deletion.

#1 will need more thinking, but I think that the only solution is properly 
secured configuration paths. This will mean that "private" configurations will 
need to use "shadow copies".

#2 is solvable by fixing the code to properly validate argv[0] (i.e., make sure 
that the realpath to the executable is a secured path as in #1, and has a 
trailing '/Contents/Resources/openvpnstart'). The second part is necessary (and 
I think sufficient) to make sure that arbitrary OS X paths that are secure 
aren't used.

#3:The race condition I see is that if someone tells openvpnstart to kill a 
process, and, between checking that the process is named "openvpn" (assuming 
that check is added, see #4) and actually killing it, the user caused a 
different process to be running with that process number. That seems farfetched 
(does OS X actually reuse process numbers like that? Hard to believe!) but 
could be eliminated by changing the code to not use (and thus not make 
available) the "kill by process number" subcommand of openvpnstart. That is 
possible, although will be some situations (when openvpn isn't responding to 
management commands) where Tunnelblick won't be able to stop and OpenVPN 
process.

#4: There is not only a race condition in openvpnstart's kill subcommand, there 
is a bug, too -- openvpnstart should check that the process name is "openvpn" 
before killing it. But if the command is eliminated, that won't be necessary.

#5: Is a leftover from when logs could be in arbitrary places. The code could 
restrict them to the actual folder in which logs are kept.

I would REALLY LIKE to get as much help and input on this as I can get.

Original comment by jkbull...@gmail.com on 11 Aug 2012 at 6:19

GoogleCodeExporter commented 9 years ago 
> 1. Allowing unsecured paths to configuration and other files (even if the 
files themselves are 0:0 744, all their containing folders must also be 
writable only by root).

Even if you resolve symlinks, fix the race condition, and check every component 
of the path to see that it's root owned etc, there is still a more fundamental 
problem here: 744 is not an indication that it's safe to run something as root.

> #2 is solvable by fixing the code to properly validate argv[0]

Probably not. _NSGetExecutablePath is probably what you want, but even so, 
there are race conditions and symlink attacks to worry about too. 

> (does OS X actually reuse process numbers like that? Hard to believe!)

After you create enough processes, the PIDs cycle around.

> but could be eliminated by changing the code to not use

That's one solution. Another solution is to have openvpn always drop privs to a 
special openvpn user, and then seteuid(openvpn-uid) before calling kill(pid, 
term). This way, the kernel does the validation, not you.

--

Also:
* Kext issue.
* Running openvpn as root using user provided config files that can exec more 
things.

--

> I would REALLY LIKE to get as much help and input on this as I can get.

I think the most helpful thing I can tell you is: stop. Don't stop making 
Tunnelblicker -- it's great -- but don't go down the dark and scary road of 
trying to make root-SUID helpers secure. It's just really incredibly hard and 
error-prone, and nowadays there's usually a better alternative solution.

I believe OS X has some nice APIs for doing things in a privileged manner 
that's controlled by OS policies -- such as nice confirmation dialogues and 
whatnot. Try to work out a framework based on these frameworks, and not based 
on a SUID helper.

I know that OpenVPN can drop privs -- you should look into this too. But 
perhaps, even more so, you might look around and see if there's a way to 
administer the TUN device without needing root privs. Linux offers 
CAP_NET_ADMIN and related; maybe OSX has something similar?

Original comment by Jason.Donenfeld on 11 Aug 2012 at 6:39

GoogleCodeExporter commented 9 years ago 
Thanks, Jason. I appreciate your advice to not try to "patch" this SUID helper, 
but I'd like to explore that a bit before I give up.

I even agree that the best solution is to have a better way of doing things, 
but I can't see that happening very soon (as opposed to "fixing" the problems 
within the SUID helper framework).

> Even if you resolve symlinks, fix the race condition, and check every 
component of the
> path to see that it's root owned etc, there is still a more fundamental 
problem here:
> 744 is not an indication that it's safe to run something as root.

If it is owned by 0:0 with 744 that's not sufficient? How come?

>> #2 is solvable by fixing the code to properly validate argv[0]

> Probably not. _NSGetExecutablePath is probably what you want, but even so, 
there are
> race conditions and symlink attacks to worry about too.

Sorry, I should have been more specific, I meant using _NSGetExecutablePath and 
realpath. Clearly it is wrong to rely on argv[0]. But if Tunnelblick

1. Uses _NSGetExecutablePath and realpath to get the real path to the program 
that is executing;
2. Verifies that each folder in that path (starting with the root folder) is 
owned by root and not writable except by root; and
3. Verifies that the final path element (the executable) is owned by 0:0, and 
has 744 permissions

then what is the vulnerability?

> Also:
> * Kext issue.
> * Running openvpn as root using user provided config files that can exec more 
things.

Kext issue is the same as the exploit -- wrongheaded use of argv[0]. Fixed by 
same fix. (Well, I'll have to make sure that's really what the code does, but 
it should have the same solution in general terms.)

"Running openvpn as root using user provided config files that can exec more 
things" is fixed by not allowing that. This was what I meant by "properly 
secured configuration paths". (Of course, it is still necessary for the person 
installing Tunnelblick and installing configurations to be sure that nothing 
malicious is included in what they are installing.) Once a configuration has 
been "secured" by shadow copying (which requires admin username/password), the 
configuration will be secure. And it will contain only references to secured 
scripts, etc.

> I know that OpenVPN can drop privs -- you should look into this too. But 
perhaps,
>  even more so, you might look around and see if there's a way to administer 
the TUN
>  device without needing root privs. Linux offers CAP_NET_ADMIN and related; 
maybe
>  OSX has something similar?

Not that I've found. Anybody else?

Again, Jason, thanks for your helpful comments.

Original comment by jkbull...@gmail.com on 11 Aug 2012 at 7:11

GoogleCodeExporter commented 9 years ago 
> I appreciate your advice to not try to "patch" this SUID helper, but I'd like 
to explore that a bit before I give up.

You'll most likely spend more time trying to get the SUID helper right than you 
would doing things another way.

> If it is owned by 0:0 with 744 that's not sufficient? How come?
Many reasons, one of which is that this destroys the security of the nosuid 
mount flag, that's commonly used on network file systems and FUSE.

> 1. Uses _NSGetExecutablePath and realpath to get the real path to the program 
that is executing;
You have to be a bit careful, also, to see if it still exists. 
_NSGetExecutablePath will return paths to symlinks that can be removed before 
realpath is called.

> 2. Verifies that each folder in that path (starting with the root folder) is 
owned by root and not writable except by root; and
And is not a symlink. Key: lstat, not stat.

> 3. Verifies that the final path element (the executable) is owned by 0:0, and 
has 744 permissions
Nope. See above.

Original comment by Jason.Donenfeld on 11 Aug 2012 at 7:52

GoogleCodeExporter commented 9 years ago 
For the Cocoa equivalent of realpath(3), see stringByStandardizingPath 
(https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundati
on/Classes/NSString_Class/Reference/NSString.html#//apple_ref/occ/instm/NSString
/stringByStandardizingPath).

realpath could suffer truncations after PATH_MAX, and will return ENAMETOOLONG. 
I don't believe NSString's stringByStandardizingPath suffers the same.

Original comment by noloa...@gmail.com on 11 Aug 2012 at 9:27

GoogleCodeExporter commented 9 years ago 
Jason, thank you for all your input. You've given me a lot to think about.

It would have been nice to be notified of this in advance and have this 
discussion privately. Did I miss an attempt on your part to do that?

Original comment by jkbull...@gmail.com on 11 Aug 2012 at 10:51

GoogleCodeExporter commented 9 years ago 
> Did I miss an attempt on your part to do that?
No. I actually had meant to post it here before the mailing lists, but 
exhaustion struck and I got confused.

If you want to discuss anything privately, though, shoot me an email or instant 
message.

Original comment by Jason.Donenfeld on 11 Aug 2012 at 10:54

GoogleCodeExporter commented 9 years ago 
Issue 213 has been merged into this issue.

Original comment by jkbull...@gmail.com on 12 Aug 2012 at 1:08

GoogleCodeExporter commented 9 years ago 
The following CVEs have been assigned to this issue.

==================
1. A race condition in file permissions checking can lead to local
root. - TOCTOU

Please use CVE-2012-3483 for this issue

==================
2. Insufficient checking of merely 0:0 744 can lead to local root on
systems with particular configurations.

Please use CVE-2012-3484 for this issue

==================
3. Insufficient validation of path names can allow for arbitrary
kernel module loading, which can lead to local root.
4. Insufficient validation of path names can allow execution of
arbitrary scripts as root, leading to local root.
5. Insufficient path validation in errorExitIfAttackViaString can lead
to deletion of files as root, leading to DoS.

Please use CVE-2012-3485 for these issues

==================
6. Allowing OpenVPN to run with user given configurations can lead to
local root.

Please use CVE-2012-3486 for this issue

==================
7. Race condition in process killing. - TOCTOU

Please use CVE-2012-3487 for this issue

Original comment by Jason.Donenfeld on 14 Aug 2012 at 1:47

GoogleCodeExporter commented 9 years ago 
WARNING -- JASON'S EXPLOIT DELETES TUNNELBLICK CONFIGURATIONS!

I have removed Jason's comment #1, and reproduce it below without the link to 
his exploit program. I don't know if it is due to carelessness or maliciousness 
by Jason, but his exploit program deletes all of your private (i.e., not Shared 
or Deployed) Tunnelblick configurations.

Note: I left in the link to the second, "for kids", exploit, because -- as far 
as I can tell -- it doesn't seem to do anything malicious.

Sorry to have not noticed this earlier; I just had a chance to look at the 
exploit code in detail.

==================================================================
Here is Jason's comment (previously comment #1), without the link to the expoit:
==================================================================

Comment 1 by Jason.Donenfeld , Aug 11, 2012
This particular exploit affects this code:

                if (  checkOwnerAndPermissions(scriptPath, 0, 0, @"744")  ) {

                    fprintf(stderr, "'%s' executing...\n", [scriptName UTF8String]);
                    returnValue = runAsRoot(scriptPath, [NSArray array]);
                    fprintf(stderr, "'%s' returned with status %d\n", [scriptName UTF8String], returnValue);

                }

checkOwnerAndPermissions runs before runAsRoot, so it's easy to swap out 
symlinks or hardlinks in between, so that runAsRoot executes a program other 
than the one validated by checkOwnerAndPermissions. Sure, one fix would be to 
open a fd, and then just use fstat and fexecve, but really this whole security 
model is broken. 0:0 and 744 is not a sufficient check.

The exploit code for that is here:
[LINK REMOVED BY MODERATOR 2011-08-21]

Actually, there are more problems with the SUID helper. Check this one out:

in main:
       execPath = [[NSString stringWithUTF8String:argv[0]] stringByDeletingLastPathComponent];

loadKexts:
        NSString * tapkext = [@"tap" stringByAppendingString: TunTapSuffixToUse([execPath stringByAppendingPathComponent: @"tap"])];
        [arguments addObject: [execPath stringByAppendingPathComponent: tapkext]];

    becomeRoot();

    int status;
    int i;
    for (i=0; i < 5; i++) {
        NSTask * task = [[[NSTask alloc] init] autorelease];

        [task setLaunchPath:@"/sbin/kextload"];

        [task setArguments:arguments];

        [task launch];
        [task waitUntilExit];

So here, we can load any kernel module that we please. 

Here's another one. There is no proper checking, again, of argv[0] (which isn't 
really the right way to do things anyway), and so we can play an easy symlink 
game with the version function, with no race required at all, for an easy root:

Exploit code: 
http://git.zx2c4.com/Pwnnel-Blicker/tree/pwnnel-blicker-for-kids.sh

Original comment by jkbull...@gmail.com on 21 Aug 2012 at 7:31

GoogleCodeExporter commented 9 years ago 
I have prepared modifications to Tunnelblick to fix the security problems 
raised in this Issue.

I would like people to test the new version and report back problems/successes.

TESTERS SHOULD BE VERY CAREFUL!

THIS IS AN EXPERIMENTAL BUILD. It has not been tested very thoroughly. A lot of 
code that deals with configurations has changed -- there is the possibility of 
losing configurations (or losing access to them). BACK UP BEFORE USING IT.

As a minimum, back up the following three folders:
     /Applications/Tunnelblick.app
     /Library/Application Support/Tunnelblick
     ~/Library/Application Support/Tunnelblick

At the end of this post is a list of some of the changes in this version.

I have uploaded a .zip of this new version (along with the source code); you 
can download it from

http://code.google.com/p/tunnelblick/downloads/detail?name=Tunnelblick_3.3beta21
_Experimental_Unsigned.zip&can=2&q=

PLEASE GIVE ME FEEDBACK about what works and what doesn't. Try everything you 
can -- creating new configurations, sharing, privatizing, duplicating, 
renaming, etc. (You can even try connecting them :)

Please email directly to me at jkb ull ard at gmail.

============================

Some of the changes include:

 * Tunnelblick.app must now be in /Applications. (A side effect of this is that you are no longer able to use multiple "Deployed" versions of Tunnelblick located in different places on the boot volume.)

 * "Private" configurations may only be "Tunnelblick VPN Configurations" (.tblk). ("Bare" .ovpn or .conf configurations may continue to be used in Deployed configurations.)

 * "Private" configurations are no longer secured. Previously secured private configurations will be "un-secured" (changed so they are owned by the user, etc.).

 * "Shadow copies" of private configurations are *always* used, and the checkbox on the Preferences panel to use them or not has been removed.

 * Updating a Deployed version no longer invalidates the new copy's digital signature. (The Deploy data is no longer copied into the application bundle itself.)

 * The "Quit All OpenVPN Processes" button has been removed from the "Utilities" panel.

 * "Flush DNS after connecting/disconnecting" is only available for configurations using "Set name server".

============================

I hope to include some additional features in a future release:

 * Enhancements for installing a .tblk -- fix subfolder references, detect CR-LF in certificates and keys (because OpenSSL rejects them), detect missing certificate and key files, etc.

 * Some way of converting a .conf or .ovpn configuration to a .tblk. (Perhaps a a "gear" menu item on the Configurations panel, or maybe automatically create them on first launch, or maybe provide a method via drag and drop or something.) Suggestions are welcome!

 * A "Revert to Last Saved" gear menu item on the Configurations panel. This would replace a private configuration (which is now unprotected, remember) with the last shadow copy that was created for that configuration. This would *not* require admin access. If a non-admin user modifies a VPN configuration, this will allow them to "undo" the changes and connect to the VPN.

Original comment by jkbull...@gmail.com on 23 Aug 2012 at 9:14

Attachments:

GoogleCodeExporter commented 9 years ago 
Hi,

Thanks for spending the time to fix this. A lot of people are not as motivated 
or hard working as you are when it comes to security things. Unfortunately 
there are still many significant issues. The one that jumped out at me 
immediately is the ability to make gOkIfNotSecure always be true.

zx2c4@Jasons-MacBook ~ $ cat give-up-on-suid-helpers.sh 
#!/bin/sh
rm -rf /tmp/pwn
mkdir -vp /tmp/pwn/Tunnelblick.app/Contents/Resources/
sed 's/NO/YES/' /Applications/Tunnelblick.app/Contents/Info.plist > 
/tmp/pwn/Tunnelblick.app/Contents/Info.plist
ln /Applications/Tunnelblick.app/Contents/Resources/openvpnstart 
/tmp/pwn/Tunnelblick.app/Contents/Resources/
exec /tmp/pwn/Tunnelblick.app/Contents/Resources/openvpnstart

From here, we can choose our own adventure with other exploitation attacks. 
Maybe some 744 link magic, maybe something else; it doesn't matter -- it's easy 
from here.

***PLEASE GIVE UP ON MAKING AN SUID HELPER***

I respectfully beg you to invest your time in saner approaches using Cocoa 
security frameworks as discussed above. You're clearly an able and intelligent 
programmer, but SUID helpers will nearly always be troublesome. Not to mention, 
there are very few cases when SUID helpers are absolutely necessary and this is 
not one of them.

***I will not audit any subsequent patches you release for SUID helpers.*** By 
pursuing the SUID route, you put your Macintosh users at great risk and pave 
the way for a myriad of security issues in the future from the extremely 
brittle SUID codebase.

I like the work you've done, and you're by no means lacking talent, but this is 
not the place for SUID helpers. For the good of your users, please give up with 
the SUID route and do something properly.

Thank you,
Jason

Original comment by Jason.Donenfeld on 24 Aug 2012 at 3:09

GoogleCodeExporter commented 9 years ago 
Today I uploaded a new version which addresses more security problems, 
including the problem Jason pointed out in comment #17.

It may be downloaded from
https://code.google.com/p/tunnelblick/downloads/detail?name=Tunnelblick_3.3beta2
1_Experimental_2_Unsigned.zip&can=2

All the warnings in my earlier comment 16 still apply: this is experimental; 
backup before using it, etc.

=======================================

New comments:

I do plan to use a more sophisticated security scheme in future versions of 
Tunnelblick, but this one still uses a SUID helper.

Special thanks to noloader for his very helpful comments and advice. (I haven't 
taken all of his advice -- all problems are my fault, not his!)

Original comment by jkbull...@gmail.com on 31 Aug 2012 at 11:25

GoogleCodeExporter commented 9 years ago 
Again, please give up with making an SUID helper. Spend your time working a 
better solution. You are wasting time that should be spent on a more secure 
solution.

You're doing your users a grievous disservice. You put Mac users at unnecessary 
risk. You will not succeed in securing this code base; your design is flawed. 
Your negligence and gross disregard for the safety of your users is shocking, 
and I can no longer recommend that anyone uses Tunnelblick or any of the code 
you write that runs privileged.

Despite several pleas, you have continued to pursue a design that will never 
succeed.

There are still holes in the most recent source you posted. In congruence with 
my previous message, I will not post any details here, in order to motivate you 
to drop the SUID approach completely.

Original comment by Jason.Donenfeld on 31 Aug 2012 at 3:55

GoogleCodeExporter commented 9 years ago 
It's been a while, have this issue been addressed?

Original comment by t.mikae...@gmail.com on 1 Apr 2014 at 11:48

GoogleCodeExporter commented 9 years ago 
t.mikael.d --

Depending on what you mean by "this issue", yes or no.

All of the specific vulnerabilities raised on posts in this Issue have been 
fixed (and so have many others).

However, I have left this issue in "Started" status for two reasons:

     1. Tunnelblick still uses an SUID binary. Jason says that that, in itself, is a vulnerability.

     2. Jason says "There are still holes in the most recent source you posted" (as of 8/31/12) but that he will not disclose details. Subsequent to his comment, several potential vulnerabilities were fixed but it is impossible to know if the "holes" he says he knows about were fixed since he has not disclosed them.

Original comment by jkbull...@gmail.com on 1 Apr 2014 at 12:17

GoogleCodeExporter commented 9 years ago 
I have committed changes to the Tunnelblick source code that implement a 
launchd daemon instead of using an SUID helper on OS X 10.5 ("Lion") and 
higher. The changes will be available in the next beta release.

A "snapshot" (pre-release version of Tunnelblick) is available that includes 
the changes. Email me privately at my Gmail address, jkbullard, for a link to 
download the snapshot.

Original comment by jkbull...@gmail.com on 14 Jan 2015 at 9:17