search

willamowius / h323plus

H323Plus - H.323 development framework
https://www.h323plus.org
108 stars 42 forks source link

Several security issue abot h323plus #16

Closed Shark2016 closed 3 years ago

Shark2016 commented 3 years ago

HI, I've found several security issue about h323plus.

I don't think it is a good idea to config plugin directiory by environment variable, it may lead to several problem. And attackers may set malicious enviroment in some way like setenv, putenv, or something like shellshock, etc. There are a few issues related to it.

Buffer Overflow in dyna.cxx

h323plus/plugins/video/common/dyna.cxx


bool DynaLink::Open(const char *name)
{
  ...
  // try directories specified in PTLIBPLUGINDIR
  char ptlibPath[1024];
  memset(ptlibPath, 0, sizeof(ptlibPath));
  char * env = ::getenv("PTLIBPLUGINDIR");               << untrusted input
  if (env != NULL) 
    strcpy(ptlibPath, env);                              << strcpy here may buffer overflow
  ...
}

bool DynaLink::InternalOpen(const char * dir, const char *name)
{
  char path[1024];
  memset(path, 0, sizeof(path));

  // Copy the directory to "path" and add a separator if necessary
  if (strlen(dir) > 0) {
    strcpy(path, dir);                                   << buffer overflow possibily here
    if (path[strlen(path)-1] != DIR_SEPARATOR[0]) 
      strcat(path, DIR_SEPARATOR);                       << buffer overflow possibily here
  }
  strcat(path, name);                                    << buffer overflow possibily here

  if (strlen(path) == 0) {
    TRACE(1, _codecString << "\tDYNA\tdir '" << (dir != NULL ? dir : "(NULL)") << "', name '" << (name != NULL ? name : "(NULL)") << "' resulted in empty path");
    return false;
  }

#ifndef _WIN32
  strcat(path, ".so");                                   << buffer overflow possibily here
#endif
  ...
}

Suggestion：

Use secure function like strcpy_s/strcat_s instead of strcpy/strcat

Dynamic Link Library Hijack

Also, untrusted source of plugin paths may lead to Dynamic Link Library hijack.

If the PTLIBPLUGINDIR is setted to some other directory with malicious dll/so with same name in it, the code in it will have a chance to be execute.

bool DynaLink::InternalOpen(const char * dir, const char *name)
{
  ...
  // Load the Libary
#ifdef _WIN32
# ifdef UNICODE
  WITH_ALIGNED_STACK({  // must be called before using avcodec lib
     USES_CONVERSION;
    _hDLL = LoadLibrary(A2T(path));                      << may load library from untrusted path
  });
# else
  WITH_ALIGNED_STACK({  // must be called before using avcodec lib
    _hDLL = LoadLibrary(path);                           << may load library from untrusted path
  });
# endif /* UNICODE */
#else
  WITH_ALIGNED_STACK({  // must be called before using avcodec lib
    _hDLL = dlopen((const char *)path, RTLD_NOW);        << may load library from untrusted path
  });
#endif /* _WIN32 */
  ...
}

Suggestion： Don’t trust environment variable input. Using get module path or hardcoded plugin path to locate library may be much safer, or use reliable configure file instead.

Buffer Overflow in h264-x264.cxx/h264pipe_win32.cxx

h323plus/plugins/video/H.264/h264-x264.cxx h323plus/plugins/video/H.264/h264pipe_win32.cxx

bool H264EncCtx::findGplProcess()
{
  char * env = ::getenv("PWLIBPLUGINDIR");
  if (env == NULL)
    env = ::getenv("PTLIBPLUGINDIR");
  if (env != NULL) {
    const char * token = strtok(env, DIR_TOKENISER);
    while (token != NULL) {
      if (checkGplProcessExists(token))                            << token from untrusted environment variable
        return true;
      token = strtok(NULL, DIR_TOKENISER);
    }
  }
  ...
}

bool H264EncCtx::checkGplProcessExists (const char * dir)
{
  struct stat buffer;
  memset(gplProcess, 0, sizeof(gplProcess));
  strncpy(gplProcess, dir, sizeof(gplProcess));                    << strncpy here

  if (gplProcess[strlen(gplProcess)-1] != DIR_SEPERATOR[0]) 
    strcat(gplProcess, DIR_SEPERATOR);                             << strcat here may overflow
  strcat(gplProcess, VC_PLUGIN_DIR);                               << strcat here may overflow

  if (gplProcess[strlen(gplProcess)-1] != DIR_SEPERATOR[0]) 
    strcat(gplProcess, DIR_SEPERATOR);                             << strcat here may overflow
  strcat(gplProcess, GPL_PROCESS_FILENAME);                        << strcat here may overflow
  ...
}

Suggestion solution： Don’t trust environment variable input. Hardcoded plugin path is more safer, or use reliable configure file instead. Don’t use unsafe function like strcat. snprintf_s is recommanded.

Command Injection in h264-x264.cxx/h264pipe_win32.cxx

h323plus/plugins/video/H.264/h264-x264.cxx h323plus/plugins/video/H.264/h264pipe_win32.cxx

bool H264EncCtx::findGplProcess()
{
  char * env = ::getenv("PWLIBPLUGINDIR");
  if (env == NULL)
    env = ::getenv("PTLIBPLUGINDIR");
  if (env != NULL) {
    const char * token = strtok(env, DIR_TOKENISER);
    while (token != NULL) {

      if (checkGplProcessExists(token))                 << token from environment variable
        return true;

      token = strtok(NULL, DIR_TOKENISER);
    }
  }
  ...
}

bool H264EncCtx::checkGplProcessExists (const char * dir)
{
  struct stat buffer;
  memset(gplProcess, 0, sizeof(gplProcess));
  strncpy(gplProcess, dir, sizeof(gplProcess));        << may lead to command injection

  if (gplProcess[strlen(gplProcess)-1] != DIR_SEPERATOR[0]) 
    strcat(gplProcess, DIR_SEPERATOR);
  strcat(gplProcess, VC_PLUGIN_DIR);

  if (gplProcess[strlen(gplProcess)-1] != DIR_SEPERATOR[0]) 
    strcat(gplProcess, DIR_SEPERATOR);
  strcat(gplProcess, GPL_PROCESS_FILENAME);
  ...
}

void H264EncCtx::execGplProcess() 
{
  unsigned msg;
  unsigned status = 0;
  if (execl(gplProcess,"h264_video_pwplugin_helper", dlName,ulName, NULL) == -1) {   << exec command
  ...
}

Suggestion： Don’t trust environment variable input. Using get module path or hardcoded plugin path to locate library may be much safer, or use reliable configure file instead.

willamowius commented 3 years ago

The code you reference is only executed inside the H.263 and H.264 plugin which are shared libraries themselves and don't have and provision for any config settings.

I fixed the one strcpy() and added a security note that if one uses these plugins the environment needs to be secured which probably isn't very difficult in most server installations.

If you see a way to use the plugins without any trusted environment variables, please provide a patch.

Shark2016 commented 3 years ago

Oh yes. It's a shared library for developers, and have no any other config settings. In that way, I think that using current module path may be another optional way to locate the plugin path.

I think the developers could put the plugins and other shared libraries into the same directory with their executable file, which can be wrote to the tutorial document. In this way, GetModuleName could be used to get current executable module path in Windows platform. And in linux, readlink( "/proc/self/exe", path, PATH_MAX) can get current executable module path.

// Windows
TCHAR szPath[MAX_PATH];
GetModuleFileName(NULL, szPath, MAX_PATH));

// Linux
char path[MAX_PATH];
readlink("/proc/self/exe", path, MAX_PATH);

Hardcoded path could be always a second option way as you use in your code:

// Windows
InternalOpen("C:\\H323plus\plugin", name);  // just an example path for windows

// Linux
InternalOpen("/usr/local/lib", name);

Anyway, what I mentioned above is just my advice for that. Environment variable is still widily used, and it's ok if only developers make sure that the environment variable is unable to be modified by others.

However the buffer overflow issue is nessussary to be fixed by using strcpy_s\strcat_s instead.