Not all coders are created equal. But there should be a line where we collectively just say, "Please stop".

Would like to say found two critical issues in two Python packages, but it's obvious to a six year old. Not sure can claim credit for such in your face obvious issues.

And since it's so obvious, responsible disclosure kinda got kicked to the curb.

These libraries pin the required dependencies and test only one python interpreter, so lets just say had lowered expectations going in.

get-gecko-driver and get-chrome-driver look like they are unneeded since both selenium and webdriver-manager can download selenium webdrivers. In the later two packages, web browser support is sparse; there is room for more flexibility. For example, support for waterfox, librewolf, and mullvad-browser.

Issues summary:

  1. downloader module can send a GET request to any URL. There is no URL whitelist. These packages can be used for cover when making arbitrary GET requests.

  2. downloader can save anywhere on the file system. So can be used for other purposes besides downloading selenium webdriver.

  3. no permission checks before saving/writing the file.

  4. get_gecko_driver.downloader and get_chrome_driver.downloader are the exact same module.

I lack confidence the author will respond, will be very pleasantly surprised if the author fix these issues in a timely manner. Nor confidence he'd do a good job. But at least these issues are disclosed and the ball is in his court.

For your entertainment:

All these coding errors are unforgivable and obvious to even a novice coder, a laymen, or a random drunk. It takes talent not to see it. If bothered to do unit testing, would be unavoidable to not see it.

These issues are both CRITICAL SECURITY issues.

Obvious is obvious, don't kill the messenger, instead lets just fix this issue. The correct action is to quickly fix it and then just agree never to mention it again and pray there is no Darwin award for coders.

you are viewing a single comment's thread
view the rest of the comments
[–] 2 points 5 months ago (4 children)

You can do exactly that with curl, requests, telnet, netcat or even plain bash. Doesn't make it high severity.

The code you provided is actually behaving exactly as expected. You called the internal download function and it did it.

If you have a way to get it to download arbitrary files from its intended api that would be much more serious, but that's not what your showing.

from get_gecko_driver import GetGeckoDriver

# Install the driver:
# Downloads the latest GeckoDriver version
# Adds the downloaded GeckoDriver to path
get_driver = GetGeckoDriver()
get_driver.install()

Do you have a technique to make the above code download an arbitrary URL to an arbitrary file location?

  • source
  • parent
  • hideshow 4 child comments
  • [–] [S] 1 point 5 months ago* (last edited 5 months ago) (3 children)

    To get the same effect from GetGeckoDriver().install()

    from unittest.mock import patch
    
    from get_gecko_driver.get_driver import GetGeckoDriver
    
    target_package_path = "../../../.venv/lib/Python3.11/site-packages"
    # e.g. 'geckodriver/linux64/0.36.0'
    remove_subfolders = "../../.."
    output_path = f"{target_package_path}/somepackage/{remove_subfolders}"
    url_arbritrary = "https://maliciousurl.com/_index.js"
    with patch(
        "get_gecko_driver.get_driver.GetGeckoDriver.version_url",
        return_value=url_arbritrary,
    ):
        get_driver.install(output_path=output_path)
    

    The URL limiting and dest folder limiting aren't hardcoded within get_gecko_driver.downloader.download (module level func), making it unpatchable. Instead the GetGeckoDriver is patch friendly.

    Assumes all Python coders have experience writting unittest or pytest. So usage of unittest.mock.patch is common knowledge and second nature. So absolutely no one would struggle writing the above code.

    To protect against patching, the base URL cannot be within get_gecko_driver.constants and then import by another module. Although not DRY, the base URL must be hardcoded minimally within get_gecko_driver.downloader.download (in the whitelist) and optionally also within get_gecko_driver.get_driver.GetGeckoDriver.install (to raise an exception with a meaningful and actionable message).

  • source
  • parent
  • hideshow 3 child comments
  • [–] 3 points 5 months ago (2 children)

    Anything you do to fix this "issue" can also be defeated by mock.patch.

    You could just mock.patch the entire download function to do anything at all.

  • source
  • parent
  • hideshow 2 child comments
  • [–] [S] 1 point 5 months ago

    Within a docker container, mitmproxy can sit filtering network traffic by URLs, rather than IP and port. Ignore in this example only one URL is allowed.

    In pyproject.toml,

    [project.scripts]
    webdriver_urls_filter = "mypackage.somefolder.mitmproxy_runner:main"
    

    In mypackage.somefolder.mitmproxy_filters,

    import re
    from typing import TYPE_CHECKING
    
    from mitmproxy import ctx
    
    if TYPE_CHECKING:
        from mitmproxy import http
    
    
    def request(flow: "http.HTTPFlow") -> None:
        """Run this proxy.
    
        :type flow: mitmproxy.http.HTTPFlow
        """
        url = flow.request.pretty_url
    
        # Rather than exact, interested in limiting the base URL
        ALLOWED_PATTERN = re.compile(r"^https://github/\.com/myorg/myrepo/releases/download/v1\.2\.3/.*$")
        
        # Check if URL matches the allowed release pattern
        
        try:
            if ALLOWED_PATTERN.match(url):
                ctx.log.info(f"Download allowed: {url}")
            else:
                ctx.log.error(f"Download blocked: {url}")
                flow.kill()
        except Exception as e:
            # FAIL SECURE: If inspection fails, kill the connection to prevent bypass
            ctx.log.error(f"Script error, blocking flow: {e}")
            flow.kill()
    
    

    In mypackage.somefolder.mitmproxy_runner,

    from mitmdump import DumpMaster
    from mitmproxy import options
    
    from . import mitmproxy_filters  # addon module
    
    def main() -> None:
        """Rather than calling `mitmdump -s myscript.py`."""
        opts = options.Options(listen_port=8080)
        master = DumpMaster(opts)
    
        if hasattr(mitmproxy_filters, 'addons'):
            # explicit format which defines a class then appends an instance to
            # :code:`addons = []`
            master.addons.add(*mitmproxy_filters.addons)
        else:
            # Module itself acts as addon
            master.addons.add(mitmproxy_filters)
        
        master.run()
    
    

    The docker container has one network proxy which has web access. Everything else has no web access instead traffic is directed thru the proxy. The proxy calls, webdriver_urls_filter.

  • source
  • parent
  • [–] [S] 1 point 5 months ago

    Had to research how to go about running untrusted code safely.

    In-process there are actions that can be taken to deter mock.patch, but it wouldn't defeat a determined adversary. A subprocess isn't a sufficient security layer although it would prevent mock.patch a module.

    To deter a determined adversary, recommend to use Docker or Firecracker MicroVMs.

    So glad we are having this conversation. Instead of jumping in with a fix that is not actually effective.

  • source
  • parent