Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YaST Security Audit Fixes: Lessons Learned and Reminder #172

Closed
shundhammer opened this issue Dec 18, 2018 · 4 comments
Closed

YaST Security Audit Fixes: Lessons Learned and Reminder #172

shundhammer opened this issue Dec 18, 2018 · 4 comments
Labels

Comments

@shundhammer
Copy link
Contributor

shundhammer commented Dec 18, 2018

Problem Classes Found by the Audit

  • Calling external programs with only program name and without a path, thus relying on the $PATH environment variable

  • Un-escaped arguments to external programs

  • Mixing SCR .target.bash and system() or backtick-calling

  • Passwords as arguments to external commands visible in ps output and in /proc

  • Tmp files using predictable filename/path

Attack Vectors

Malicious Programs in $PATH

If $PATH contains any directory where unprivileged users may have write access or where they can mount (!) external media to (USB sticks, NFS / Samba shares), they might put malicious programs there that are named just like something that is called by us; somebody might put their own grep or cp or whatever there that may not only do what the original does, but exploit the privileges of the caller.

Remember that YaST is always running as root!

Whoever can make us call his binaries can get root access very easily.

Don't forget shell built-ins like echo and test, and remember that [ is just an alias for test. There is a counterpart for each of them in /bin or /usr/bin.

Malicious Programs Disguised as Plain Files

If we use the content of directories as part of a command line, we need to be safe against files that have names that are weird, but legal. In a Linux/Unix environment, there are few characters that are explicitly forbidden in file names; the slash (/) is definitely forbidden, everything else is in most cases just weird, but permitted.

A file may also be named something like

; rm -rf * or
"; rm -rf * or
"; chown root rootshell; chmod u+s rootshell

Of course this will make the original command fail, but the malicious command that follows will succeed, so the damage is already done by the time anybody notices that something is wrong.

Remember that it only takes one single of those commands to be executed with root permissions to cause damage. Somebody might plug his USB stick in with whole directories of such commands with various combinations of different types of quotes.

So if we read a directory's content with Ruby or with C++ or with one script and command and just use those names that we found as they are, we might easily build command lines that by accident execute any of those malicious commands.

Malicious Commands in Device Names / Mount Points

Any of the above can also happen with device names (OEM strings!) or mount points (volume labels!). A USB stick might contain this.

Just look at /dev/disk/by-label and /dev/disk/by-id to get an idea.

Volume labels are commonly used in udev rules as mount points.

Malicious Commands Hidden in Other Places

All files that we read, all data that we process that comes from the outside, all user input is stuff that cannot be trusted, so we need to make sure to properly escape all of that. The same attack vectors as with filenames apply to all those things.

Special Caveat: Escaping Single Quotes in the Shell

In most programming contexts, when you have a quote inside a quote, it only matters if it is the same kind of quote, and then you simply prefix it with a backslash:

"Value: \"#{value}"\" (changed)"

The shell is weird in that aspect. When you have a quote inside a quote, you need to terminate the string, then add the quote escaped with a backslash, then start a new string. So the above example would be

"Value: "\"value\"" (changed)"

This is very unexpected for most people, and so it is frequently done wrong.

Shell-Escaping vs. Regexp-Escaping

While the shell has special treatment for some types of characters, regular expressions have some other types of characters. Sometimes it's hard to get it right for both cases.

Fortunately, regexps typically consist of a fixed part (that's where usually the characters go that have special meaning for the regexps) and sometimes of a variable part.

So, for most grep calls, we did something like this:

/usr/bin/egrep '^[0-9]+\s+'#{foo.shellescape}'bar$'

So if our foo variable contains characters that need to be shell-escaped, they are properly escaped, and the egrep command receives one single string as the argument. Remember that the shell consumes the quotes and any backslashes outside of quotes, so even if the complete command expands to something like

/usr/bin/egrep '^[0-9]+\s+'very\ weird\'stuff'bar$'

the egrep command gets

^[0-9]+\s+very weird'stuffbar$

which is the expected thing.

Common Fixes

Minimize Using External Commands

It should be obvious, but still we see it in many places in the code:

Don't use external commands that have a good and easy-to-use Ruby counterpart. I.e. don't pipe the output of one command to any of grep, sed, tr, cut. Simply use plain Ruby for those things.

Don't use ls or find to find files. Use Ruby Dir and Find instead.

Use YaST String::Quote

Use shellescape from Ruby Shellwords

If most cases, using single quotes ('foo bar') is the appropriate thing; but this does not help if an argument may contain a single quote (six o'clock), in which case it gets weird (see explanations below).

So, one good approach is to remove existing previous single quotes and use Ruby shellescape() instead:

"/usr/bin/foo -p '" + arg + "'"
is now
"/usr/bin/foo -p " + arg.shellescape

Remember to require "shellwords" !

https://ruby-doc.org/stdlib-2.5.1/libdoc/shellwords/rdoc/Shellwords.html

You can either use Shellwords.escape or shellescape. The latter is an extension of the Ruby string class that this Shellwords class does. For classes other than string, you might have to use to_s to convert to string.

Use Unpredictable Filenames/Paths for Temporary Files

Use Ruby Dir.mktmpdir to create a tmp dir with an unpredictable name and restrictive permissions. Inside that directory you can safely use a fixed filename.

http://ruby-doc.org/stdlib-2.0.0/libdoc/tmpdir/rdoc/Dir.html

Don't use Ruby Tempfile to just generate a filename for use in external commands; this is still insecure:

# WRONG - Don't do this!
require "tempfile"
...
tmpfile = Tempfile.new("yast2-") # Will create something like /tmp/yast2-20181219-1245
tmpfile_name = tmpfile.path # save because the name will be zeroed after close/unlink
tmpfile.close
tmpfile.unlink
# Now we have a tmp filename that was unique one moment ago

# This is the chance for an attacker to set up the same file with his permissions
result = Yast.Execute("/usr/bin/foo-keygen >#{tmpfile_name}")
# Here the attacker can read the content of that file
result = Yast.Execute("/usr/bin/foo --keyfile #{tmpfile_name}")
FileUtils.rm(tmpfile_name)

Executing External Programs in a Safe Way

Different methods are used in YaST to run external commands:

  • With ruby Kernel methods like system or %x() (backticks).
  • With Yast::SCR.execute.
  • With Cheetah.run.

If properly used, there is no important drawback with using any of these methods. For example, with Kernel#system, %x and Yast::SCR.execute the command arguments need to be escaped in advance. With Kernel#system and %x nothing is logged into YaST log when the command fails, and no exception is raised.

Yet, it is preferable to have just one common way of running commands, so arguments are automatically shell escaped, errors and warnings are logged to the YaST logs and exceptions are raised when the command fails. And this is what Yast::Execute does. That class uses Cheetah under the hood, so all command arguments are automatically escaped and it also sets the YaST log as default Cheetah log. Moreover, using Yast::Execute (well, Cheetah actually) is also safer due to it runs the command directly without shell expansion, as Kernel#system and %x do.

So, in general, try to always use Yast::Execute to run external commands instead of other possible methods. Also, when possible and for the sake of uniformity, try to replace current Yast::SCR.execute calls. For example:

Yast::SCR.execute(path(".target.bash_output"), "ls -l #{dir.shellescape}"))

can be replaced by

Yast::Execute.locally!("ls", "-l", dir)

or

Yast::Execute.on_target!("ls", "-l", dir) when trying to run the command in a chroot

Note: with Yast::Execute bang methods an exception might be raised when the command fails (this does not happen with Yast::SCR.execute), so make sure that you rescue the exception to mimic SCR behavior.

Further Reading

Security Tips at our YaST Doc Hub

@jreidinger
Copy link
Member

I think it would be good to sumarize why is bad: Mixing SCR .target.bash and system() or backtick-calling

For common fixes we maybe can also mention Yast::Execute which is even more secured as it does not invoke any shell.

One caveeat of shellescape is that it escaped also globs, so echo /tmp/* return just /tmp/* and not all stuff in /tmp which some code uses.

Also what we can mention as helping for shell escaping is having constant as real constant and not as some kind of variable, as it is much harder to see what is our string and what is user input, fs content, etc.

So far that is what I have in my mind.

@jreidinger
Copy link
Member

btw in that section about escaping single quotes, it is not much visible that it is single quotes :)

@joseivanlopez
Copy link
Contributor

I have added a new section "Execute external programs in a safe way".

@shundhammer
Copy link
Contributor Author

shundhammer commented Dec 19, 2018

I think it would be good to sumarize why is bad: Mixing SCR .target.bash and system() or backtick-calling

That would be something for the next section: Attack Vectors. Do we know any attack vector for that?

One caveeat of shellescape is that it escaped also globs, so echo /tmp/* return just /tmp/*

Yes, of course, that's the whole point of that. But that would be a problem only if any code used globs in a variable. Such code would be fundamentally broken and very insecure and would urgently need fixing.

In cases where globbing is used, that should always be in fixed strings which we don't shell-escape. That's also not a security problem because the shell only interprets that stuff once, so if globbing is used, any code injection attempt described above would not work.

and not all stuff in /tmp which some code uses.

Also what we can mention as helping for shell escaping is having constant as real constant and not as some kind of variable, as it is much harder to see what is our string and what is user input, fs content, etc.

True. But that's not specific to security issues, it's a general Ruby programming thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants