Security risk with scripts that write to /sys/class

What was actually said and its meaning

Quote from the linked post:

Set the permissions 755 owned by root. Then either edit your sudoers file to allow them to be run as root, or use chmod +s to set them SUID.

This sort of thing is considered a security risk, BTW, so make absolutely sure the permissions are set appropriately.

This talks about two cases:

• setting script and allow it to be executed as root without prompting for password
• setting SUID bit on scripts

What's the problem with scripts in general ? To paraphrase from Stephane Chazelaz's answer (which I strongly recommend reading), shell scripts can be a problem if run either in web servers or when privileged script runs something else. To a regular user who just has desktop - that means in most cases destroyed system - maybe attacker injected rm -rf / somewhere - or computer being overtaking by malware and being a part of botnet to attack commercial servers. But for commercial servers, this can mean anything from client credit card information being stolen to systems destroyed causing loss of money because system is down and customers go somewhere else. So when something is said to be a security issue, you need to know what that means for you. You also should know who could be your possible attacker - that defines what sort of approach they can use to compromise your system, and for someone interested in your credit card information or conversation with people, they'll likely to go after network traffic rather than the script, which means they'll likely go with MITM attack rather than the script.

What can be the problem in your specific case ?

• Your script executes 3 commands: echo $1 | tr, and cat. If attacker replaces cat or tr with malicious programs, it can mean either system destroyed, or leaking information every time you execute those commands. And because your script runs with root-level privilege - those commands also run with root level privilege. Since echo is a shell built-in, it's immune against attacks where /bin/echo is replaced ( unless you run env echo instead - that will call /bin/echo). Arguably, if you have someone capable of replacing system-level binaries, it means they already have root access, which is more important problem than just your script.

• The scripts live in /home/ioana/.config/i3/ with 0754 permissions. OK, that's fine. If your account is compromised attacker doesn't need root - they'll use your account to overwrite the script contents. What about permissions of the /home/ioana/.config/i3/ directory ? Deleting a file requires having write permissions on the directory where file lives, so if you have another user on your system and they do not have write permissions on script itself, if they have permissions to write into directory they can delete the script (not really a security issue, but a mini-DoS for ships and giggles).

• Another problem in theory can come from the command-line parameters. You have echo $1 | tr '[a-z]' '[A-Z]'. Say an attacker uses /*/*/*/*/../../../../*/*/*/*/../../../../*/*/*/* as $1 mentioned in Stephane's answer. The shell will need to convert those * into actual files,and all those expansions are expensive for CPU. That's a small way to make your computer lag, again mini-DoS.

• In case you're running outdated version of bash, it can be vulnerable to arbitrary code injection via exporting functions - aka shellsock. So one could export a malicious function before running your prvileged script.

• If an attacker is fond of animal cruelty, they can abuse cat

• Of course, all these things can be combined with wget to download something else that's malicious on your computer and execute with root privileges.

In the end, the fact that you're operating on /sys/class/ type of directory isn't the problem. Problem lies with the level of what shell scripts can do and that shell scripts have mechanisms that aren't perfect. But let's not get overly paranoid. Like I said, if someone got access to your account (which has sudo privileges) or root account - that's enough of a concern already.

See also

• Why is SUID disabled for shell scripts but not for binaries?

There are a few ways to significantly reduce the attack surface of your scripts. Note that I'm no Bash expert, so this isn't an exhaustive list of issues, just a few issues I can see with your existing code.

• Handle errors and unexpected inputs and conditions! Your code assumes everything will be working as planned.
• Don't require or pay attention to user input if you can avoid it. You can do this easily by making two scripts for keyboard brightness (e.g., kbd_brighter and kbd_dimmer). You can also make two scripts to raise and lower LCD brightness by 100.
• Do a sanity check on the /sys file before writing to it. Does it exist? Does it contain only a single integer? What should we do if that integer is unexpectedly a negative integer? Handle all possible cases properly.
• Always put double quotes around any expressions with a variable in them to avoid problems with variables filled with an empty string ([ $1 -eq 5 ] becoming [ -eq 5 ] is just asking for trouble) and to ensure that variables containing special characters like spaces or newlines are handled properly.
• Use the full path to the commands to avoid PATH attacks. For example, /bin/cat instead of cat. You can see the path with type cat.
• Use Bash features instead of external commands. For example VARIABLE=`< filename` instead of VARIABLE=`cat filename`. Also, use echo instead of /bin/echo to use the Bash builtin echo command. You can see that the builtin command is available with type echo.
• Separate the modification logic from the file-writing logic to avoid having to inspect several locations for file-writing bugs, especially when making changes to the file.

Summing up, here's an example kbd_brighter script. Note that I haven't tested it since I don't have an Asus. It might also still have security issues since I'm not an expert.

#!/bin/bash

BRIGHTNESS_FILE="/sys/class/leds/asus::kbd_backlight/brightness"

if [ ! -e "$BRIGHTNESS_FILE" ]; then
    >&2 echo 'ERROR: Asus keyboard backlight brightness file not found.'
    exit 1
fi

BRIGHTNESS="$( < "$BRIGHTNESS_FILE" )"

if [ "$BRIGHTNESS" -eq "$BRIGHTNESS" ] 2>/dev/null; then
    if [ "$BRIGHTNESS" -lt "1" ]; then
        BRIGHTNESS="1"
    elif [ "$BRIGHTNESS" -lt "3" ]; then
        BRIGHTNESS="$(( "$BRIGHTNESS" + 1 ))"
    else
        BRIGHTNESS="3"
    fi
else
    >&2 echo 'ERROR: Asus keyboard backlight brightness is not an integer.'
    exit 1
fi

echo "$BRIGHTNESS" > "$BRIGHTNESS_FILE"