Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security of packages #56

Open
sashahilton00 opened this issue Oct 7, 2014 · 16 comments
Open

Security of packages #56

sashahilton00 opened this issue Oct 7, 2014 · 16 comments

Comments

@sashahilton00
Copy link

Just noticed that the packages are being delivered over HTTP, which makes sense, but there is no verification of the actual file that is delivered. This could lead to MITM attacks, where the attacker loads his own package into the updater, then intercepts other computer's traffic and redirects to his updater, which, once installed, gives him full access to the user's computer. May I suggest that we use the Crypto plugin (http://nodejs.org/api/crypto.html) to implement DSA signing of packages, which ensures they are the original package, with the advantage of it still being possible to deliver it securely over HTTP. This feature is an absolute must if this updater is to ever be implemented as a service/process, and certainly in a program with many users.

@4ver
Copy link

4ver commented Oct 7, 2014

Is a nice idea- though wouldn't using SSL provide the same security without the need to generate any app specific keys etc?

@sashahilton00
Copy link
Author

Yes it would, though to a lesser extent (256bit vs. 2048bit) and it would also require people to buy SSL certificates, whereas the DSA signature is free and not subject to the same warnings/errors abt. Self signed certificates. Also, unless the HTTPS uses Extended Validation, it is possible to forge HTTPS certificates. With DSA, short of brute forcing the key, there is not much one can do to crack it. Though if we were to use HTTPS as opposed to DSA, then I suggest we implement HTTPS fingerprinting...

@majodev
Copy link

majodev commented Nov 10, 2014

I'm using node-webkit-updaterto create a desktop variant of Cryptocat (ticket), hence this issue is of high priority for me (I'm no security expert).

Let's assert that the most up-to-date application's package.json - which includes needed update manifest data - is hosted within a GitHub repository (requested via https://raw.githubusercontent.com/REPO/package.json) and releases are then downloaded via https://github.com/REPO/releases/download/VERSION/APP.zip.

How secure is this scenario with node-webkit-updater (if HTTPS fingerprinting ev. gets implemented)?

@sashahilton00
Copy link
Author

@majodev If it has EV, then it is pretty secure. The weakest link in this scenario would be if someone got hold of your git password and used it to upload a malicious version. This is why I suggested the DSA on top of HTTPS; even if someone gets hold of your password for git by some other means, as long as you generate to private key on your computer, make it a decent length (2048/4096 bit), and never transfer it over the internet, only store it on your computer/USB stick, then even if your git is compromised, the attacker will still be unable to push malicious software, as the package will need to be cryptographically signed with the private key that you hold in a safe place.

@majodev
Copy link

majodev commented Nov 10, 2014

@sashahilton00 OK, big thanks for providing this important information!

I think the Sparkle Update Framework handles signing + checking in a same manner as described by you.

So, to accomplish signed updates, a whole deployment process may look like this (given that someone actually contributes DSA checking capabilities to node-webkit-updater):

  1. Borrow Sparkles' sh scripts generate_keys.sh and sign_update.sh.
  2. Use generate_keys.sh to generate public (dsa_pub.pem) and private (dsa_priv.pem) DSA 4096bit key files.
  3. The file dsa_pub.pem must always get redistributed with the app binaries (hence include it within your node-webkit-builder task).
  4. The (securely kept) private key dsa_priv.pem will be used to sign the app's .zip via sign_update.sh. The generated value of this script must be included within the manifest update data (see the example update entry below).
  5. node-webkit-updater get's the DSA key from the manifest and downloads the new app .zip.
  6. Before unpacking the downloaded .zip, the DSA key entry from the remote manifest must be validated against the redistributed dsa_pub.pem and the newly downloaded .zip.
  7. If OK, proceed with unpacking + installing.

Example update entry with DSA keys in manifest:

"packages": {
  "mac": {
    "url": "http://localhost:3000/releases/updapp/mac/updapp.zip",
    "dsa": "MCwCFFPIOejIR0uCackc1QcthLju/apTAhQed6mytl+bddBn+fqSRBAWL7dnCg=="
  },
  ...
}

Right? I really like this discussion so far. 👍

@sashahilton00
Copy link
Author

@majodev Yes, that is pretty much the flow that I was talking about, and this is pretty much the workflow I use for applications with a built-in updater. This is probably unnecessary, but if you want to safeguard it further, you might want to include the dsa_pub.pem in the actual zip update, so that if the private key was ever to be compromised, then you could hopefully push out a new public key before any damage was done, thus making the compromised private key useless, though this may be a bit extreme. The solution you have proposed above is still very secure. If you want to make it slightly more secure, I would suggest also including the file checksum in the manifest and to verify that also, though it is almost impossible that two different files could share the same signature, so this is probably unnecessary too.

@majodev
Copy link

majodev commented Nov 12, 2014

If anyone is interested in contributing this to node-webkit-updater (or securing your own application), I've implemented DSA signing and verifying today in my fork of Cryptocat.

The DSA Signature gets verified after node-webkit-updater has successfully downloaded an update. The process is very straightforward (but currently synchronous) done here.

How-to:

  • Use bin/generate_keys.sh from Sparkle to generate your private and public key files
  • Bundle the public key file dsa_pub.pem with your applications.
  • Add a verification step after an update is completely downloaded (like here and here).
  • After packaging: Use bin/sign_update.sh (again from Sparkle) to get the DSA signatures of your zipped apps.
  • Host the urls of your zipped updates + its DSA signatures the within your remote package.json (e.g. like here).

@sashahilton00
Copy link
Author

Looks good, are you planning on integrating it into his repo?

@majodev
Copy link

majodev commented Nov 13, 2014

@sashahilton00 Currently not, but I'll change my mind maybe if I have some free time in the future (and nobody had time to contribute it already). To integrate it here, it'll have to be fully test covered, more decoupled and well documented, also I have no idea if this kind of (optional?) security checking is in the interest of @edjafarov. Furthermore I've had problems running the tests of this repo on my current machine, hence I don't want to push anything that has flaws / I can't guarantee is working.

@edjafarov
Copy link
Collaborator

node-webkit-updater gives low level api to update apps. I am not sure we should add features in package. This could be implemented separately like @majodev did. Though I think this thread is really awesome and should be highlighted in readme/wiki.
@adam-lynch what do you think?

@majodev
Copy link

majodev commented Nov 13, 2014

I also felt that providing such a implementation is a bit over the top within this package. It depends on too many side factors + I don't want to force users to use DSA only. Furthermore, the most comfortable way to compute these DSA signatures is during a product's build-task (e.g. after bundling via node-webkit-builder and compressing the new releases via grunt-contrib-compress) + appending them to package.json (or update-manifest) you are about to host publicly. However, this is totally out of the scope of this package.

@edjafarov One thing that would be "nice-to-have" is exposing your platform-variable (e.g. a public getter) from your app/updater.js, so I don't have to borrow it like here. This way one could easily provide more sub-fields (e.g. my dsa at the mac and winsubfield within the packages-field like here) in the update-manifest and safely get the additional needed ones from the remote manifest during the update procedure.

@4ver
Copy link

4ver commented Dec 2, 2014

@sashahilton00 @majodev Popcorntime seem to have implemented dsa signing of packages in their updater. Copy, paste, pr 😄

https://git.popcorntime.io/stash/projects/PT/repos/popcorn-app/browse/src/app/lib/updater/index.js#137

@sashahilton00
Copy link
Author

They have, I have actually done a bit of work on that updater myself actually :) The entire updater is a bit buggy, but the verifying part is fine, I will take a look at making a PR if I can find the time, applying to Uni atm.

@adam-lynch
Copy link
Collaborator

@sashahilton00 I'd love to pick your brain on that updater (about the other steps). I just had a look around too.

@sashahilton00
Copy link
Author

What would you like to know? (I didn't do all of it, it was a collaborative effort, but will try to answer as best I can :)

@adam-lynch
Copy link
Collaborator

@sashahilton00 see #63. Thanks! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants