Opened 5 years ago

Closed 5 years ago

#15289 closed defect (fixed)

exchndl.dll issues

Reported by: ioerror Owned by: datallah
Milestone: 2.10.7 Component: winpidgin (gtk)
Version: 2.10.6 Keywords: security
Cc:

Description

exchndl.dll is shipped with the Windows pidgin ( http://developer.pidgin.im/static/win32/pidgin-inst-deps-20100315.tar.gz). I think the source for that dll is from http://pidgin.im/~datallah/exchndl.c

I did a quick audit and in general, it appears that it has a few issues.

CVE-2010-x+n seems to ( http://blog.zoller.lu/2010/08/cve-2010-xn-loadlibrarygetprocaddress.html ) apply. Specifically, it looks like ( http://www.exploit-db.com/exploits/14741/ ) a specifically crafted DLL could really screw things up at crash time ( http://www.securityfocus.com/bid/1699/discuss http://msdn.microsoft.com/en-us/library/ms684175%28VS.85%29.aspx ). It might even be possible

Change History (9)

comment:1 Changed 5 years ago by ultramegaman

It seems that the calls to LoadLibrary? would be exploitable if:

  • Pidgin is currently closed
  • Pidgin was opened via a file association
  • The associated file's location contained a IMAGEHLP.DLL file in the same directory
  • The functions within exchndl.dll containing LoadLibrary? calls were invoked

comment:2 Changed 5 years ago by ioerror

Some other comments on other issues in the code...

rprintf() has an internal static buffer ( TCHAR szBuff[4096] ) and it does not bounds check at all. In theory, this may be safe because none of the calls should be able to give it more than 512bytes, such as this:

BdfDemangleSymName(szSymName, szSymName, 512);
...
rprintf( _T("  %s"), szSymName);

However, if we look at BfdDemangleSymName?() we see that it uses lstrcpyn() which shouldn't be used ( http://msdn.microsoft.com/en-us/library/windows/desktop/ms647491(v=vs.85).aspx ) as Microsoft says Warning Do not use. Consider using StringCchCopy? instead. See Remarks.

In one case lstrcpyn is used in in BfdDemangleSymName?() exactly as it shouldn't be used: The lstrcpyn function has an undefined behavior if source and destination buffers overlap.

In another case in BfdDemangleSymName?() is takes the result of cplus_demangle() and puts it into res. res should be considered totally untrusted as a result of the lack of validation and the issues with LoadLibrary?.

It isn't clear to me that rprintf() in that case is actually safe in all cases. The bounds checking for rprintf() depends on functions that are declared dangerous and specifically bad to use as they are used...

comment:3 Changed 5 years ago by ultramegaman

Also, this worries me. The OnStartup?() routine looks like this:

       if(GetModuleFileName(NULL, szLogFileName, MAX_PATH))
        {
                LPTSTR lpszDot;
                if((lpszDot = _tcsrchr(szLogFileName, _T('.'))))
                {
                        lpszDot++;
                        _tcscpy(lpszDot, _T("RPT"));
                }
                else
                        _tcscat(szLogFileName, _T(".RPT"));
        }

Suppose that the full qualified path of the module contains a "." in the directory, such as C:\Users\myuser\pidgin.testing\module.exe; this code would create a file at C:\Users\myuser\pidgin.RPT, which is not what's expected.

A better example is when the path of the module approaches MAX_PATH in length and the GetModuleFileName? call truncates the response and the truncation chops off the "." in the file name extension. The result is that szLogFileName will be MAX_PATH in length, which is the size of its allocated buffer. The "." is not found, so ".RPT" is added past the end of the buffer. This will surely result in corruption of the stack.

comment:4 Changed 5 years ago by datallah

pidgin.exe isn't registered for any file associations.

It is registered for various URIs, but when it's invoked for those, the exception handler isn't used.

I don't think the LoadLibrary thing is a real problem.

For the OnStartup stuff:

  • I don't think the comment about a "." being in the directory is correct - it's _tcsrchr, so it's looking for the last instance, so unless a module doesn't have an extension (which would be really odd on Windows, although perhaps not impossible?), it'll be ok.
  • For a really long path to a module, that's potential problem. I think it's really unlikely; it would require the user to have decided to install Pidgin somewhere down a crazy path. The effect probably would be an crash on startup and I don't think it could be exploited. Ideally it'd be fixed though.

For rprintf() buffer, I think it actually is possible to exceed the buffer in the example you posted, but only by a couple bytes (the spaces in the " %s" pattern).

We can't use the "new" "safe" MS string functions (such as StringCchCopy); with mingw gcc, we use the MSVCRT runtime, not one of the newer C runtimes.

I see what you mean about the overlapping buffers being passed to lstrcpyn, that's clearly something that should be fixed (although I suspect that since they're actually the same pointer it coincidentally works - that's the case our code hits every time since it isn't C++ and I don't remember seeing any odd behavior).

comment:5 follow-up: Changed 5 years ago by ioerror

I think LoadLibrary? vulnerable to CVE-2010-x+n but I don't see an obvious way to exploit it. That is a classic problem with CVE-2010-x+n, of course.

I'm not sure about [_tcsrchr] but I'm sure ultramegaman will let us know if it is an issue.

Regarding rprintf(), yes, I think I agree regarding the ability to overflow the static buffer. That can be easily fixed by checking the length of the string.

I'd probably just rewrite a lot of the possibly problematic code to be safe. I opened this ticket with the developers: http://code.google.com/p/jrfonseca/issues/detail?id=73

As I'm still not sure of the final file's contents, I'm not clear if this would be a CVE only for exchndl.dll proper or both exchndl.dll and pidgin's derivative exchndl.dll. I think it is in both - though it isn't clear in what scenarios it would be exploitable by a malicious party, so it isn't terribly critical for all issues. Though the rprintf() issue seems like it will be triggered exactly when a malicious attacker has taken it upon themselves to mess with things...

comment:6 in reply to: ↑ 5 Changed 5 years ago by datallah

Replying to ioerror:

I think LoadLibrary? vulnerable to CVE-2010-x+n but I don't see an obvious way to exploit it. That is a classic problem with CVE-2010-x+n, of course.

I'm not sure about [_tcsrchr] but I'm sure ultramegaman will let us know if it is an issue.

Regarding rprintf(), yes, I think I agree regarding the ability to overflow the static buffer. That can be easily fixed by checking the length of the string.

I'd probably just rewrite a lot of the possibly problematic code to be safe. I opened this ticket with the developers: http://code.google.com/p/jrfonseca/issues/detail?id=73

If you were to do that, that would certainly be welcome.

As I'm still not sure of the final file's contents, I'm not clear if this would be a CVE only for exchndl.dll proper or both exchndl.dll and pidgin's derivative exchndl.dll. I think it is in both - though it isn't clear in what scenarios it would be exploitable by a malicious party, so it isn't terribly critical for all issues. Though the rprintf() issue seems like it will be triggered exactly when a malicious attacker has taken it upon themselves to mess with things...

The source of the exchndl.dll that we distribute is the upstream git repo at revision db7edd55a561, with the exchndl_daa4.diff patch in pidgin-inst-deps-20100315.tar.gz applied.

comment:7 Changed 5 years ago by datallah

  • Milestone set to 2.10.7

I've imported forked the code into a hg repo to facilitate easier maintenance.

I also believe that I've addressed the issues identified here in 005749a4d596 and b17553e9dc94.

I haven't yet updated the windows build scripts to use a new exchndl.dll yet, but the plan is to do so before the next release. I'd appreciate some input on these changes

comment:8 Changed 5 years ago by ioerror

For the release scheduled for the next few days, will it use this improved version of exchndl.dll?

comment:9 Changed 5 years ago by datallah

  • Resolution set to fixed
  • Status changed from new to closed

Yes, the change to use this was committed as [8c11f90243e4]. I forgot to mention the ticket in the commit message.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!