[Vulnerability Bug] Used blacklisted dangerous function call that can lead to RCE

[Aju100]

Hi Opensource enthusiast,

After looking through the whole codebase, I get to identify a few vulnerabilities on the fury repository.

Impact

• urlib not only opens http:// or https:// URLs, but also ftp:// and file://. With this it might be possible to open local files on the executing machine which might be a security risk if the URL to open can be manipulated by an external user.
• It can lead to self-RCE, and also general RCE if we can trigger it for the remote end-user.

Vulnerable code

github_tools.py

from urllib.request import urlopen, Request

... some code here
def fetch_url(url):
    req = Request(url)
    if GH_TOKEN:
        req.add_header('Authorization', 'token {0}'.format(GH_TOKEN))
    try:
        print("fetching %s" % url, file=sys.stderr)
        # url = Request(url,
        #               headers={'Accept': 'application/vnd.github.v3+json',
        #                        'User-agent': 'Defined'})
        f = urlopen(req)

Patches

github_tools.py

from urllib.request import Request
...... some codes
req = urllib.request.Request('urlhere.com')

def fetch_url(url):
    req = Request(url)
    if GH_TOKEN:
        req.add_header('Authorization', 'token {0}'.format(GH_TOKEN))
    try:
        print("fetching %s" % url, file=sys.stderr)
        # url = Request(url,
        #               headers={'Accept': 'application/vnd.github.v3+json',
        #                        'User-agent': 'Defined'})
        f = urlopen(req)

Vulnerable code

fetcher.py

from urllib.request import urlopen
.... some code

def _get_file_data(fname, url):
    with contextlib.closing(urlopen(url)) as opener:
        try:
            response_size = opener.headers['content-length']
        except KeyError:
            response_size = None

        with open(fname, 'wb') as data:
            if response_size is None:
                copyfileobj(opener, data)
            else:
                copyfileobj_withprogress(opener, data, response_size)

Patches

fetcher.py

import urllib.request
... some code 

url = urllib.request.Request('urlhere.com')
def _get_file_data(fname, url):
    with contextlib.closing(urlopen(url)) as opener:
        try:
            response_size = opener.headers['content-length']
        except KeyError:
            response_size = None

        with open(fname, 'wb') as data:
            if response_size is None:
                copyfileobj(opener, data)
            else:
                copyfileobj_withprogress(opener, data, response_size)

For more info. You can also go through bandit where it has listed dangerous calling functions. For the URL we have to insert in Request('here')

References: Stackoverflow

[Nibba2018]

Hi @Aju100 , thanks for creating an Issue. This file is used to fetch contributor's information from GitHub for the documentation website and is not directly part of FURY's actual code base, so regular users shouldn't be affected by it. Also, I don't understand the patches that you have suggested can you please explain it a bit more? It looks like you have called the same method twice.

A proof of concept on how this could be exploited would help us understand the situation. Thanks.

[skoudoro]

Thanks for letting us know.

+1, I don't see or understand your fixes/patches in your comment. it seems there is no change.

Please, feel free to create a PR, the change will be more explicit. Thank you

[Aju100]

Hi, @skoudoro @Nibba2018 sorry for my delayed response due to a few personal work. The functions that code is written are deprecitated and vulnerable to Remote Execution code. For having URL in order to use it, we have hard coded it.

[Aju100]

Fixes #357

[amit-chaudhari1]

I read up on this, and there's a couple of ways to fix this:

• store the landing pages domain name "https:my_awesome_site.com" in the url, and then only append the remaining url, thereby we only fetch urls which we intended to. this also an answer at this SO question... ( it's not much, but its honest work! )

• Clarify the protocol. this is much open ended IMO, cause we can specify that we want to only communicate over http/https. and the "RCE" will go poof (unless we are running code pieces from invalidated requests. hope fully won't find them in the code base, I've not looked enough)

I think @Aju100 was trying to implement the first way. (although I doubt there was any need for using global vars, and personally I don't like to do it that way)

---

What we do we need to do?

We just need to add a check to fetch url, return an error if someone tries to communicate over any other protocol other than http: or https: any valid url is bound to contain these...

After change, our little fetch_url(url) will look something like:

def fetch_url(url):
  req = Request(url)
  if GH_TOKEN:
      req.add_header('Authorization', 'token {0}'.format(GH_TOKEN))
  try:
      print("fetching %s" % url, file=sys.stderr)
      # url = Request(url,
      #               headers={'Accept': 'application/vnd.github.v3+json',
      #                        'User-agent': 'Defined'})
      if url.lower().startswith('http'):
          f = urlopen(req)
  except Exception as e:
      print(e)
      print("return Empty data", file=sys.stderr)
      return {}

  return f

Will this be correct? Will it fix the issue?

I hope it will fix the issue. @Aju100 Could I get a couple of valid & invalid req to test this against? I need to confirm it works before I commit.

---

I will fix this in the next PR, pinky promise !!!

[Aju100]

Hey @amitchaudhari9121 , yes it will fix the issue. Make PR and let the maintainer review the code.

[skoudoro]

Fixed by #399 , closing