Navidrome: Security improvements on transcoding command input

[ElleshaHackett]

The "command" input bothers me a bit. I understand admins should have the freedom to make fancy custom commands. But I would suggest to make it harder to do so, or at least warn users for that. Maybe hide the option and only show it when "advanced" is enabled? Even better would be to only use the default options provided and entirely disable the feature unless Navidrome is started with an "advanced" or other flag specifically to enable the use of custom commands. For example when I use docker we could introduce an extra environment variable for this such as allow_insecure_commands=TRUE while the default if FALSE. This greatly improves default security.

Otherwise, since whitelisting isn't possible and blacklisting/validating command is a terrible idea, to add a big fat warning that users should know what they are doing if they change this.

In addition I would add a check to see what under what User Navidrome is running. If it's running as root I suggest to add an additional warning on that page saying that Navidrom should not run as root for it is highly irresponsible to do so when the command input simply passes and runs everything that's dropped in there (with a single/very simple auth mechanism in between that people probably don't setup with proper SSL). Meaning an attacker would sniff the Navidrome password out of the air in seconds, login and be root on the machine through Navidrome.

[ElleshaHackett]

I thought some more about this. I think being able to disable "command input" to begin with at startup is a very good idea. Or better: being able to enable it and disable it by default.

[jvoisin]

I like the idea of hardcoding commands in navidrome, since I don't think that anyone will ever change them.

[ElleshaHackett]

I think it's still a good idea to keep the options open for advanced users. You want a broad audience to use this tool. Just I think the default settings should be idiot-proof and as secure as possible.

[deluan]

Navidrome currently ships with two pre-defined encodings, mp3 and opus. They have different commands, and they can be tweaked by advance users with a ton of options. I don't think hardcoding is an options, specially because if you want to add extra transcodings (ex: aac) , you'll need to enter the command, which could be anything and not only ffmpeg (ex: lame, flacenc, etc, etc...)

Hardcoding just for these two predefined ones also does not make sense, I think it makes the experience inconsistent.

Also hiding and only showing in "advanced mode" is not practical, as I don't see someone entering this view without wanting to see or change the command, the most important piece of configuration in the view.

I think a warning is enough, and if the admin, the owner of the instance, wants to tweak with it, let them.

I may be missing something, but I'm not sure how adding a configuration to enable it would make it more secure as it could still be enabled. I think it would just be more annoying..

[ElleshaHackett]

Got it. So, the two warnings are one thing (warning "running as root" and "danger command execution", that is always good to warn about but doesn't add to security.

I didn't mean at all to hardcode anything. Two broader thoughts on this matter:

1. I understand you want to hit a big audience so it's always hard to find a balance between non-technical users (make it idiot-proof and very user friendly and easy) and technical users (give them freedom and advanced options to play with). For the matter of adding extra encoding options, I would simply suggest why not adding them directly? This way everyone can enjoy the benefits and Navidrome does what it's meant for (automatically) stream/transcode as many formats as possible when needed. If there are copyright issues with Lame (I thought that was the case once) than maybe we can use alternatives. Also, if you don't want to add all dependencies to keep it small and fast, we could still implement it as optional and make it easy to "install" the extra encoding options.

2. My point about adding the "allow-commands", "allow-insecure-commands", "allow-custom-commands" flag (whatever you name it) is that the default (and probably average user) will not use it and probably won't need it either. Meaning that through the Web UI it simply isn't possible to get system access, running commands (which is even worse in case Navidrome runs as root). But for an advanced user is takes only one argument to enable that in the Web GUI and he can play as much as he wants. But the thing is, adding that flag, which should be off by default, makes both advanced users aware of the potential risk as they need to manually enable it, and keeps the average unaware user safe from a very serious attack. Imagine some idiot sets up Navidrome with "admin:admin" on the login, there you go, (root) system access through Navidrome Web UI. It's just a massive risk, free unfiltered and invalidated user input directly executed. Also an advanced user could for example choose to run Navidrome with the flag, setup the proper transcoding commands, restart Navidrome without the flag and voila, no way to change it anymore in the Web GUI (way more secure). I would suggest to still be able to see all configured commands even without the flag set, but just to disable editing it by default, unless the instance was started with a special flag.

I personally would definitely let that feature be disabled unless I really needed to change something specific and them temporarily enable it. That flag should of course also be passed into Docker in case Docker is used. And luckily Docker does some isolation but it is still containerization not virtualization. So even with Docker, I would not feel comfortable having this enabled and accessible in my local network. Lastly, if you are really against this idea I suggest the reverse option: passing a "disable-command-edit" or whatever flag. But my point it, it should be configurable and probably by default in the secure way.

[deluan]

Ok, I see your point.

If the main concern is to avoid running any command as Navidrome's user, we could do the same Subsonic does: Only allows executing commands that are put in the special transcode folder. That way the user would have to add anything they want to run in that folder, which would contain ffmpeg by default.

This would make distributing Navidrome binaries more cumbersome, though, as it would have to include ffmpeg....

[ElleshaHackett]

I'm honestly not sure if that is the right approach. I have to checkout how Subsonic does that.

But how do you see that practically happen? exec(input) becomes exec('/prefixed_folder/' + input)? That would still be totally insufficient and easy to bypass in many ways.

Also I would also not add any binaries if not necessary indeed.

[deluan]

We could change the PATH to only that folder (just to be safe), check if the executable file exists in that folder, and then execute that specific file with the sanitized transcoding arguments

ffmpeg is not optional for Navidrome. If we look at a specific folder for it and we don't bundle it in Navidrome, a lot of user issues would pop up (scanning is not working, where do I put ffmpeg?, etc...)

[ElleshaHackett]

Ok, I suppose if you take that approach and the sanitized transcoding arguments are indeed properly sanitized and don't allow any bypassing (I hope some Go library has that implemented and well tested) then that could work somewhat, but it's still a risk. My point is to remove this specific risk entirely (or at least being able doing so) and your option is to decrease the risk somewhat.

This way you only move the trust-level a bit, because if you take the approach you suggested you'll be completely depending on the proper sanitation of the arguments and the implementation of it. The specified folder where the binaries should be in (assuming that stuff like ../ or / isn't possible), would also not limit that risk when the argument sanitization fails cause then things like ... && shutdown are possible.

You have basically four options regarding the "command edit" feature:

• Feature entirely off = _not an option, we apparently want this feature_
• Feature always on and allows all input = _now the case_ (quite insecure, only the authentication mechanism of Navidrome is in the way to get (root) system access. Which stores and likely sends the passwords in clear-text (if users don't setup SSL) so can be sniffed in the network easily._
• Feature always on but limited use < _what you suggested_ (still risky, fully trusting on sanitation quality and implementation of it)
• Feature configurable < _my suggestion (and by default off and read-only or alternatively the ability to disable entirely)_

Making it configurable (nonmatter what the default behavior is) removes this specific risk instead of decreasing it. I suggest off by default (secure for most users), but on by default with an option to disable is still better than having it always on. I would even actually suggest to do both. Make the feature configurable (on/off by flag) and if on, use the method you suggested. With security it's better to be safe than sorry.

[deluan]

I'll add the configuration to disable it, as it is the simplest solution and one of the most secure (I never disagreed with you ;)) and will think more about a solution to disable it by default but at the same time make it simple enough for users to configure it. Maybe transcoding could only be configurable via command line....

Regarding sanitization, it is already sanitized. The exec.Command method used does not call a shell to execute the command, so no && and other shell symbols/substitutions are possible EDIT: are not possible. Also we split the input to separate the command from the args. So only the command would have to be verified if it is in the /transcode folder.

Anyway, I think this adds some issues with distributing (ffmpeg binary has to be placed in that folder), and as you said, it is not the most secure solution

[deluan]

I added an option to enable transcoding editing and disabled it by default: Also added a not in the documentation: https://www.navidrome.org/docs/usage/configuration-options/#-security-considerations