Security: Add permission check to ExecuteProcess API

[r3352]

Summary

• ExecuteProcess and ExecuteProcessReturnOutput in Utility.java call Runtime.getRuntime().exec() with no authorization check, allowing any connected client to execute arbitrary OS commands on the server
• Adds an opt-in property (security/block_remote_process_execution, defaults to false) that administrators can enable to block these calls over the network

Vulnerability Details

CWE-78 (OS Command Injection) / CWE-862 (Missing Authorization)

The ExecuteProcess function at Utility.java:1284 and ExecuteProcessReturnOutput at Utility.java:1371 pass caller-supplied command strings directly to Runtime.exec() without any authorization check. These functions are callable over the network via SageTVConnection.recvAction().

Changes

• SageTVConnection.java: Added a property check in recvAction() that looks at security/block_remote_process_execution (defaults to false). When set to true, ExecuteProcess and ExecuteProcessReturnOutput are blocked at the network entry point before reaching Catbert.evaluateAction(), returning null in the standard response format. Local UI calls are unaffected.

Backward Compatibility

Existing behavior is completely unchanged. The property defaults to false, so process execution over the network continues to work unless an administrator explicitly opts in to blocking it.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[r3352]

Hi — just checking if this set of security fixes (#511, #512, #513, #514) is on your radar. CLA is signed and passing. Happy to address any feedback.

[Narflex]

Yeah, I took a look at these but none of them are valid. AI doesn't actually understand the codebase, so these fixes it is suggesting are naive. Some of them will actually break the functionality (since it doesn't realize that I commented out code because it was actually implemented on both ends of a client/server connection). And the other 'security' fixes don't actually understand the SageTV security model. If you'd like me to go into detail as to specifics for your own understanding, just let me know...I'm happy to explain.

[Narflex]

I did look at it again...and I don't think any of the changes are actually breaking (I didn't notice the zero length checks for if the auth property wasn't configured)....but again, none of these security fixes are actually useful because the security model wasn't designed to block things like this. It assumes everything on the LAN is trusted (and that's way too big a hole to close at this point).

[r3352]

Thanks for taking the time to review these, Jeff — I appreciate the detailed feedback.

A few points I'd like to address:

On the AI characterization: These findings came from a manual source code audit and were validated against a live instance. I used tooling to help generate the fix patches (as noted in your second comment, they're actually non-breaking), but the vulnerabilities themselves were identified through hands-on analysis of the codebase — tracing recvAction() through to Runtime.exec(), walking the Blowfish key derivation in Sage.java, testing the MediaServer and EncodingServer protocols over the wire, etc. Dismissing the findings because the patches had AI assistance doesn't address the underlying security issues.

On the LAN-trust model: I understand SageTV was originally designed for trusted home networks. However, a few things undermine that assumption today:

1. There are internet-facing SageTV instances right now. I confirmed the vulnerabilities against a live instance on a public IP. The server binds 0.0.0.0 by default — anyone who port-forwards or runs this on a cloud VM (which people do) is fully exposed.
2. LAN-trust hasn't been a defensible security posture in a long time. Compromised IoT devices, guest networks, lateral movement from phishing — the attack surface on a home LAN is real. A media server that gives unauthenticated Runtime.exec() to anything on the network is a serious escalation vector.
3. The EncodingServer auth wasn't "implemented on both ends" — it was commented out on the server side (lines 232-241 of EncodingServer.java). The client may send credentials, but the server never checks them. That's not a design decision, that's a gap.

On "too big a hole to close": I hear you that retrofitting auth into a protocol that never had it is a big lift. But these PRs are incremental hardening — permission checks on the most dangerous APIs, re-enabling auth that was already written but disabled, input validation on shell-executed paths. They don't require rearchitecting the protocol.

I'd genuinely welcome the detailed technical explanation you offered — if there are specific cases where these checks would break legitimate functionality, I'm happy to adjust the patches. The goal here is to reduce risk for the people running SageTV, not to create churn.

[Narflex]

Glad to hear they were manually done for the most part. :)

Regarding the security model, anybody who runs SageTV on a public IP without any other kind of VPN/tunneling protection shouldn't be doing that. It was never designed for that except for through the locator service, port 31099...any other ports should be blocked off from public access.

And yes, I know LAN trust isn't really a thing anymore...but it was back when this was written. I'm far more concerned about changes breaking this code base that is very solid and reliable, than trying to update the whole thing for modern security principles.

I'll go through the individual patches and comment on them.

[Narflex]

I don't fully remember how this works, but IIRC, the Permission contexts were only for mini/placeshifter clients. I don't think they apply to full SageTV clients (because it's linked to the UIContext in execution, and I don't think there is one on the server for full clients). Did you come across how that part worked to refresh my memory?

[r3352]

You're right — I traced through it and the permission model is scoped to mini/placeshifter clients. In Permissions.hasPermission(), when uiMgr is null it returns true (the comment at line 119 says this explicitly). And in SageTVConnection.recvAction(), uiContext stays null on the server side since Sage.client is false — so for full client connections, stack.getUIMgr() is null and the check is a no-op.

The check as written would only gate mini/placeshifter clients that have a UIManager created via MiniClientSageRenderer.createAltUI(). For ExecuteProcess specifically, one option would be to invert the default for this permission — deny when there's no security context rather than allow. Something like:

if (uiMgr == null || !Permissions.hasPermission(PERMISSION_EXECUTEPROCESS, uiMgr))
    return null;

That would restrict ExecuteProcess for all callers unless they have an explicit security profile that allows it. But I understand that changes the permission model's semantics in a way you may not want. Happy to adjust — what approach would you prefer here?

[Narflex]

It's definitely not OK to add the permission and default it to not allowed, that's a breaking change. I should also mention the whole Permission thing is not about internal security...it was entirely designed to limit what the UI could do in an STV-agnostic way. The main usage is for things like when somebody is house-sitting for you and you want to let them watch TV, but give them no ability to mess with your own usage of SageTV while you are away. ExecuteProcess was considered out of scope for that, since it was a generic thing not linked to any specific UI functionality.

So overall, I don't have any recommendation for how to implement this in a way that would block this. I also believe there are likely various other paths where this same technique of server side process execution can be achieved, so putting this in and saying it's been blocked wouldn't be true. If you're looking for more of an example of what I'm thinking, there's calls for file upload that can be invoked through various routes, and then properties can be changed to load external Java classes...and then those could do whatever they want.

[Narflex]

I'm also curious...what's your motivation for doing these changes?

[r3352]

That's a fair point about the permission system not being the right mechanism here. After digging deeper I agree, it was designed for UI restrictions, not network security.

A cleaner approach might be to gate it at the network entry point instead. recvAction() in SageTVConnection.java passes method names straight to Catbert.evaluateAction() with no filtering. A property-based check there, something like allow_remote_process_execution defaulting to false, would block ExecuteProcess over the network without touching the permission system at all. Local UI calls wouldn't be affected. If someone explicitly needs remote process execution they can flip the property. I can rework the PR around that approach if you're open to it.

I also noticed while tracing this that ReflectionFunctionTable in Catbert.java allows arbitrary class reflection over the network, so in theory someone could call java_lang_Runtime_exec directly through recvAction(). Separate issue but figured it was worth flagging.

As for motivation, Google acquired SageTV back in 2015 and it's in their open source portfolio, which put it on my radar. I work in appsec and penetration testing and this seemed like an interesting challenge given how stable the codebase is and the relatively small number of reported issues. The architecture is genuinely unique and it was a fun audit.

[Narflex]

I do appreciate the research into this, but I don't want to block ExecuteProcess by default over the network. But if you wanted to add a property (which defaults to false) where recvAction in SageTVConnection.java blocks ExecuteProcess and ExecuteProcessAndReturnOutput, then I'd be fine with that in case anybody wanted to harden things that way.

[r3352]

Thanks Jeff — reworked per your feedback.

Removed the Permissions.java and Utility.java changes entirely. Instead, added a property-based check in recvAction() in SageTVConnection.java that looks at security/block_remote_process_execution (defaults to false). When an admin flips it to true, ExecuteProcess and ExecuteProcessReturnOutput get blocked at the network entry point before they hit Catbert.evaluateAction(), returning null in the standard response format. Local UI calls aren't affected since this only gates the recvAction path, and the permission model is untouched.

So existing behavior stays exactly the same unless someone explicitly opts in.