notepad-plus-plus / nppPluginList

The official collection of Notepad++ plugins.
1.05k stars 351 forks source link

Redesign the stuff #31

Closed KOLANICH closed 5 years ago

KOLANICH commented 5 years ago

I think it is an extremily bad idea to hardcode plugins list into a dll; dll is an executable code, to minimize the profits of injecting malicious code there the lib should be updated as rarely, as makes sense. So I think it's better to keep the info in a signed (with a good enough signature scheme, no obsolete stuff like md5, which is still advertised by notepad-plus-plus.org) JSON downloaded from a repo, and allow users to use any count of repos.

dinkumoil commented 5 years ago

@KOLANICH The plugin list DLL file is signed, have a look at the Properties dialog of the file in Windows Explorer.

Nevertheless things could be done more secure. I had a look into pluginsAdmin.cpp. In the method PluginsAdminDlg::updateListAndLoadFromJson() the DLL file with the plugin list gets loaded by a call to LoadLibrary, followed by GetProcAddress(hLib, "getList"). This shows that the DLL is not a resource-only DLL, it contains executable code.

It would also be possible to include the JSON file as a resource into the DLL. Then it could be loaded with LoadLibraryEx with parameter dwFlags set to LOAD_LIBRARY_AS_DATAFILE | LOAD_LIBRARY_AS_IMAGE_RESOURCE and the JSON could be retrieved with calls to FindResource and LoadResource.

Since dwFlags set to the value mentioned above avoids all the steps special to loading a DLL file containing executable code, the whole process would be more secure. Security would not only rely on a code signing certificate but on the way of loading the DLL itself.

KOLANICH commented 5 years ago

But why to use of the dll at all? Why not just sign data files?

chcg commented 5 years ago

@donho What do you think about the suggestions of dinkumoil above?

donho commented 5 years ago

@dinkumoil @chcg

It would also be possible to include the JSON file as a resource into the DLL. Then it could be loaded with LoadLibraryEx with parameter dwFlags set to LOAD_LIBRARY_AS_DATAFILE | LOAD_LIBRARY_AS_IMAGE_RESOURCE and the JSON could be retrieved with calls to FindResource and LoadResource.

Nice idea. I'll modify it this evening.

donho commented 5 years ago

@KOLANICH

But why to use of the dll at all? Why not just sign data files?

There's no standard way so far to sign JSON. And since there's code signing certificate for Notepad++, the choice is obvious.

KOLANICH commented 5 years ago

There's no standard way so far to sign JSON.

You can sign a file using openssl and your x.509 cert. gpg can also be used for that.

favorini commented 5 years ago

How about using JSON Web Signature (JWS)? Also see Signing a json document or string with x509 certificate.

donho commented 5 years ago

@KOLANICH @favorini with a code signing certificate ?

donho commented 5 years ago

@dinkumoil @chcg Sorry for the delay, but it's been done at least; https://github.com/notepad-plus-plus/notepad-plus-plus/commit/fbffdd8825ecafb7f351a2db6f094cfe4e821288

favorini commented 5 years ago

@donho Not sure what certs work, but I guess JWS has some issues. PASETO is a newer alternative. There is a .NET implementation. Seems like pulling the JSON data off the web somewhere would be better than a static file included with the executable.

KOLANICH commented 5 years ago

with a code signing certificate ?

Yes, with the private key associated with it.

p0358 commented 5 years ago

Even pulling updated DLL for plugin list update regularly would be much better than just hardcoding it forever and without auto-updates with particular Notepad++ version

p0358 commented 5 years ago

@donho I understand you're talking about costs. Code signing certificate requires to be signed by a Certificate Authority in order to be trusted by Windows (Windows has its trusted CA store). With custom private/public key signature verification, this is not needed. You only need to include your public key in executable, which could be used to verify downloaded list with a signature created using secret private key. Bundled public key's integrity would be protected by executable being signed with code signing cert (that's omitting that if somone can patch executable locally, they can just as well do other things right away instead of playing around with patching Notepad++).