[Bug]: Security checks too strict for custom handlers

[JohnPaton]

What happened?

If you pass the path to a custom handler and it contains the substring os anywhere, the handler will fail to load "for security reasons". This is very prone to false positives, e.g. having your handler in a directory tree containing git_repos will trigger it.

Example for having a directory in the tree called tempos below:

litellm_settings:
  custom_provider_map:
  - custom_handler: /Users/jpaton/temp/tempos/test-litellm-proxy/my_llm.my_llm

yields

ERROR:    Traceback (most recent call last):
  File "/Users/jpaton/temp/tempos/test-litellm-proxy/.venv/lib/python3.12/site-packages/litellm/proxy/types_utils/utils.py", line 18, in get_instance_fn
    security_checks(module_name=module_name)
  File "/Users/jpaton/temp/tempos/test-litellm-proxy/.venv/lib/python3.12/site-packages/litellm/proxy/types_utils/utils.py", line 77, in security_checks
    raise ImportError(
ImportError: Importing from module /Users/jpaton/temp/tempos/test-litellm-proxy/my_llm is not allowed for security reasons

The checks were introduced in 441c727

Relevant log output

Are you a ML Ops Team?

Yes

What LiteLLM version are you on ?

1.69.2

Twitter / LinkedIn details

No response

[ishaan-jaff]

How would you propose checking this @JohnPaton ?

[palotasb]

The first thing you need to check is whether the POST /config/update route is properly protected or not? Based on what I understand it's intended to be used for admins via the Admin UI and it's a dangerous API, since there may be many ways to exploit it. Either way it's either already a correctly protected endpoint or it is not.

• If the POST /config/update route is already properly protected and only accessible to admins but not regular users, the original vulnerability report may have been a false positive (not actually a security issue), since they already had admin rights and they used the admin rights to reconfigure the LiteLLM proxy. That's not really a privilege escalation.
• If the POST /config/update route is not already properly protected and it's also accessible to regular users and not just admins, then it's a more serious issue with the LiteLLM proxy. It would mean that any user can reconfigure the LiteLLM proxy, and you have only partially fixed one of the many way in which regular users can do harm by reconfiguring the proxy.

Either way, you need to make sure only admins can reconfigure LiteLLM and regular (or anonymous, unauthenticated) users can not. Once that's done, I think the changes in 441c727 can be safely reverted since no one will be able to trigger the reported vulnerability in LiteLLM, only admins. But admins are already expected to have full privileges, so arbitrary code execution is something they can already do, it would not be an issue, not a privilege escalation.

[palotasb]

Just to make this explicit, blacklisting a small number of "dangerous" modules doesn't actually fix the arbitrary code execution vulnerability anyway. E.g., hackers could still call other dangerous modules you weren't aware of, including 3rd party modules. This type of check is akin to banning <script> in HTML form submissions in web apps, it prevents only the most trivial exploits, but the real fix is to sanitize whatever gets inserted into HTML pages.

Again in LiteLLM's case the key is making sure that only admins can reconfigure the LiteLLM proxy at all. Once that's implemented, it doesn't matter if they can reconfigure it to execute arbitrary code with post_call_hooks or whatever since they are admins they can already do whatever they want.

[ishaan-jaff]

cc @wagnerjt high priority issue ^

[nightshiba]

+1. This is a textbook case of security by obscurity. I even had to add the missing o renaming post_call_rules.ensure_non_empty to pst_call_rules.ensure_non_empty just to get past the filter. Blocking any module name that contains os is the wrong approach; security checks belong in the API layer, not in a module‐name blocklist that's trivial to bypass (as it was mentioned before).

[krrishdholakia]

Had another user report this as well

[krrishdholakia]

will investigate today

[krrishdholakia]

Confirmed - /config/update is a protected endpoint. I agree with @palotasb and @nightshiba - if this is an admin only endpoint, then i do not view the admin executing arbitrary code as a vulnerability. We should look to roll back these changes given the false flags it raises.