DC++ 0.851 - Arbitrary code execution

[Kacper]

Details and PoC: http://kacperrybczynski.com/research/dcpp_851_arbitrary_code_execution/

By supplying an UNC path in the *.dcext plugin file or main/pm hub chat, a remote file will be automatically downloaded, which can result in arbitrary code execution.

[poy (poy)]

from what I understand by reading <http://blogs.technet.com/b/srd/archive/2015/02/10/ms15-011-amp-ms15-014-hardening-group-policy.aspx>, this is well mitigated by the "UNC Hardened Access" feature that has been introduced.

"Even 3rd party applications and services can take advantage of this new feature without additional code changes; simply add the necessary configuration details in Group Policy. If a UNC Provider is able to establish a connection to the specified server that meets the required security properties, then the application/service will be able to open handles as normal; if not, opening handles would fail, thus preventing insecure access to the remote server."

on the actual issue (whether DC++ should allow clicking on UNC paths), I have no opinion - maybe people in local networks actually enjoy pasting links to files stored on some shared network server?

this would input from others, but from what I have gathered, the security issue has been fixed in Windows itself so I see no reason to block these links as they can have legit uses.

[Kacper (kacper1964)]

Similar issue reported on Avast software, and fixed by Avast, https://code.google.com/p/google-security-research/issues/detail?id=546 a few days ago.

[Kacper (kacper1964)]

I think maybe it is worth to fix. Please tell me about whether it will be fixed, or not. Thanks

[poy (poy)]

you got the wrong link - that one is about HTML embedded in a field of certificates. ;)

what I wrote above is my opinion of this after a quick research - I would like to know what others in the DC++ team think.

[Kacper (kacper1964)]

Link is correct, Look to PoC:

certificate details:

CN=<a href="file:///c:/Windows/System32/calc.exe">\x0D\x0A<img src="http://www.troll.me/images/are-you-fucking-kidding-me/srsly.jpg" />\x0D\x0A</a>\x0D\x0A<h1><a href="file:///c:/Windows/System32/calc.exe">Click Here</a></h1>

Its the same PoC with file:// scheme.

[eMTee (realprogger)]

Exactly my thoughts poy.
Like the recent rar exploit that's been in the headlines absolutely unnecessarily; if you click a malicious link or download and install a malicious executable yourself, well, it has always been the users' responsibility.
Combined the chat link with unicode quirks can rather be called dangerous but that needs a fix somewhere else.

[Fredrik Ullner (ullner)]

I'll attempt to clarify a few things (after some testing).

It is possible create any form of link that will appear in DC++, e.g. writing "foo://bar" will cause DC++ to show a clickable link. The only way to execute that link from basic out-of-the-box behaviour is that the user is required to doubleclick this link for it to execute. What happens is that any unknown URI gets passed to ::ShellExecute, which is just letting Windows handle everything. This means that any Windows will then find the appropriate URI handler (in file://'s case, it's Windows Explorer) and let that take care of everything. So I do not think that this is specifically related to UNC paths or any particular one protocol.

In Chrome and Internet Explorer, following this type of link in a webpage (i.e. a href) will cause prompt dialogs for the user, asking them whether they want to keep or execute the file ("it may harm you"). After the user agrees to the security risk, the browsers does the same thing as DC++.

There is no such protection or prompts in DC++, which is (what I believe) is the true bug report here. It is very advisable to include such protection, even if it is simply a "are you sure you want to do this" type of dialog and then just continuing on. This prompt should be done ASAP in any case.

I could not see how a plugin is of any relevance, except for the fact that plugins may be implemented to ALSO do a non-check on link execution. I believe this is difficult to impossible to prevent. If plugin authors decide to implement (automatic) link management in such a way, it is up to them to prevent it as such.

[Kacper (kacper1964)]

Regarding to "UNC paths" its only example of exploit vector, not main problem.

What Fredrik wrote makes seanse. Lack of prompt on click maybe can solve the problem. But please remember that ::ShellExecute scheme/link have permissions from parent DC++ process (process explorer on screen post #1) which also may have a security risk for DC++ users.

I think good improvement is create scheme whitelist; ( http://,https://,dchub:// )

Regards

[Fredrik Ullner (ullner)]

I don't think a scheme whitelist is a good approach, we can't anticipate what users might input (and what protocols they will use). Testing with Chrome and IE, I get a prompt dialog for protocols it cannot possible know of (adc://). I believe a prompt dialog is sufficient for the most part here as well.

Regarding permissions; I don't know if we can change that, though. If there is a different API than a ShellExecute call that can be made, then we can of course perform that. I'm coming up empty on viable options at MSDN at least, but I've only quickly looked.

[Fredrik Ullner (ullner)]

This will add a prompting dialog that appear as http://imgur.com/8IgJLkZ

The text is a combination of Internet Explorer and Chrome warning texts.

Note that, per the patch, the following is excluded:
 * http:,
 *https:
 * www.
 * mailto:

[Kacper (kacper1964)]

I attach PoC for plugins without file:// handler. Link is direclty executing:

void WinUtil::openLink(const tstring& url) {
 ::ShellExecute(NULL, NULL, url.c_str(), NULL, NULL, SW_SHOWNORMAL);
}

[poy (poy)]

nice.
can you also add a setting to disable it? or even better, allow the whitelist to be user-editable...

[Kacper (kacper1964)]

Hub mainchat/PM:

PoC:

c://windows//system32//cmd.exe

[20:09] <lolek> c://windows//system32//cmd.exe

Also works without file://.

[Fredrik Ullner (ullner)]

This patch will add a whitelist settings box in "experts only".

[Fredrik Ullner (ullner)]

Kacper: can you share the source code of the plugin? So we are not overlooking anything.

[Kacper (kacper1964)]

Hi Fredrik,
My PoC for plugins have only 1 file, info.xml

in parameter: <Website> I can escape from html tag using quote " char, like XSS attack.

[Fredrik Ullner (ullner)]

This version will also validate that the plugin's website value is correct.

[poy (poy)]

looking good.

can you change the new function pointer to an std::function? that's what dwt uses in other places.

also (if it's not too much work), maybe look into slapping a StringListDlg over the white list field. ;)

[Fredrik Ullner (ullner)]

Update with std::function and a StringList.

[poy (poy)]

great!

just one thing: now that we have an std::function, the "!= nullptr" check can (or even should) be removed.

[poy (poy)]

Fixed in DC++ 0.860.