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

Update Readme section on verifying signatures #6821

Closed
MylesBorins opened this issue May 17, 2016 · 26 comments
Closed

Update Readme section on verifying signatures #6821

MylesBorins opened this issue May 17, 2016 · 26 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. meta Issues and PRs related to the general management of the project. security Issues and PRs related to security. tools Issues and PRs related to the tools directory.

Comments

@MylesBorins
Copy link
Contributor

MylesBorins commented May 17, 2016

EDIT:

We now generate detached signatures for all release lines. There is no documentation on how to verify this. An update to the Readme would be great!


Original

The current process for verifying releases is outputting a warning

gpg: Signature made Thu May  5 17:56:43 2016 CDT using RSA key ID 4C206CA9
gpg: Good signature from "Evan Lucas <evanlucas@me.com>" [ultimate]
gpg:                 aka "Evan Lucas <evanlucas@keybase.io>" [ultimate]
gpg: WARNING: not a detached signature; file 'SHASUMS256.txt' was NOT verified!

A script to verify and output is included in this gist

@MylesBorins MylesBorins added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. security Issues and PRs related to security. labels May 17, 2016
@MylesBorins
Copy link
Contributor Author

/cc @rvagg @nodejs/build

@jasnell
Copy link
Member

jasnell commented May 17, 2016

@evanlucas

@evanlucas
Copy link
Contributor

yea, I noticed this earlier. It isn't just mine though. Every release I've run against has shown the same thing (although I haven't gone back too much)

@rvagg
Copy link
Member

rvagg commented May 18, 2016

OK, so here's the thing that's going on here: when you do a --verify, gpg will look at the file being verified and if there's an equivalent non-signed file then it'll assume you're working with a detached signature, which we don't do. This is happening here because you're also downloading SHASUMS256.txt and have it beside SHASUMS256.txt.asc. Try removing or renaming the former and you'll get a different result because gpg will decide it's a cleartext signed document with the sig inside the doc rather than detached from it.

But this raises an interesting point because detached signatures offer a bit more safety than we offer and maybe we should switch our signing mechanism to use them instead, or as well. The reason is that gpg will only verify the contents between -----BEGIN PGP SIGNED MESSAGE----- and -----BEGIN PGP SIGNATURE----- using the signature found within. But we are recommending a simple grep. So someone wishing to insert a bad build onto nodejs.org could just add a new line outside of the validated block with their new shasum and the filename and the file would still get verified and the invalid shasum would be grepped just fine. i.e. we don't have enough steps on our README to properly handle this case so it's actually not all that secure.

Detached signatures give you a signature for an entire file in a separate file. If we shipped a SHASUMS256.txt.sig as a detached signature then you'd download that as well as SHASUMS256.txt and gpg --verify would check the sig against the original and verify the whole thing. Then our instructions on the README about using grep against SHASUMS256.txt would be perfectly acceptable.

@nodejs/crypto @jbergstroem can you --verify my logic above ^?

@bnoordhuis
Copy link
Member

@rvagg Yes, that's right. I assume the .asc is created with gpg --clearsign --sign <file>? Changing that to gpg -b <file> would produce a detached signature.

@jasnell
Copy link
Member

jasnell commented May 18, 2016

Yep, spot on explanation. I definitely think moving to the detached signature would be the best choice here.

@MylesBorins
Copy link
Contributor Author

thanks for digging in. +1 on detached signature as well

@mhdawson
Copy link
Member

+1 for detached signatures.

@evanlucas
Copy link
Contributor

Works for me

@Fishrock123
Copy link
Contributor

Detached seems better to me, will doing this impact anyone?

@jasnell
Copy link
Member

jasnell commented May 19, 2016

Not likely. The verification step is essentially the same.

@MylesBorins
Copy link
Contributor Author

What needs to be done to move to detached signatures?

@bnoordhuis
Copy link
Member

@thealphanerd #6821 (comment), basically.

@rvagg
Copy link
Member

rvagg commented Jun 8, 2016

I'd like to handle this one pls

@MylesBorins
Copy link
Contributor Author

@rvagg any movement on this?

@rvagg
Copy link
Member

rvagg commented Oct 13, 2016

fix @ #9071

@rvagg
Copy link
Member

rvagg commented Nov 3, 2016

Will leave this issue open as a reminder to update the README with instructions on using the new sigs. They are live for all releases now, e.g. https://nodejs.org/download/release/v7.0.0/

ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/) for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Also note that this PR also updates the base download URL from "http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that before this PR (or asdf-vm#16 which is not merged), binaries where downloaded over plain legacy HTTP! (those binaries where later executed by the user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/) for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Also note that this PR also updates the base download URL from "http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that before this PR (or asdf-vm#16 which is not merged), binaries where downloaded over plain legacy HTTP! (those binaries where later executed by the user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Also note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this issue Feb 20, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
@lots0logs
Copy link

@rvagg Would it be possible to backport this to the current LTS branch?

@jasnell
Copy link
Member

jasnell commented Mar 11, 2017

Ping @nodejs/lts

@MylesBorins
Copy link
Contributor Author

@lots0logs the fix landed in v4.6.2 and v6.9.1

@MylesBorins MylesBorins added the good first issue Issues that are suitable for first-time contributors. label Apr 26, 2017
@MylesBorins MylesBorins changed the title Warning when following instruction in verifying release Update Readme section on verifying signatures Apr 26, 2017
@MylesBorins
Copy link
Contributor Author

I've changed the title of this issue to make it a good first contribution. We need to update the Readme with instructions on how to verify releases.

@neeharv
Copy link
Contributor

neeharv commented Apr 26, 2017

Would be happy to work on this if no one else has already done so!

@bthemukul
Copy link

Hi @MylesBorins , @neeharv ,
Even I would like to contribute in any way possible and wanted to work on this.
So please share & let me know, how can I help. Thank you!
Cheers!

@Trott Trott added the doc Issues and PRs related to the documentations. label Aug 4, 2017
@maclover7
Copy link
Contributor

@MylesBorins Should this be closed? The readme contains instructions about verifying via SHASUMS256.txt.asc

@rvagg
Copy link
Member

rvagg commented Aug 23, 2017

This is still an active TODO. We need to switch to recommending use of SHASUM256.txt and SHASUM256.txt.sig rather than the .asc files for reasons I stated above when making the case for detached signatures. Since we've been doing detached signatures for many months now it's time to update the README with the safer recommendation.

maclover7 added a commit to maclover7/node that referenced this issue Sep 12, 2017
It is more secure to verify SHASUMS256.txt files via SHASUMS256.txt.sig
than SHASUMS256.txt.asc.

[This comment](nodejs#6821 (comment)) does the best job at explaining the issue.

Refs: nodejs#6821, nodejs#9071
rvagg pushed a commit that referenced this issue Sep 13, 2017
It is more secure to verify SHASUMS256.txt files via SHASUMS256.txt.sig
than SHASUMS256.txt.asc.

This comment does the best job at explaining the issue:
  #6821 (comment)

Refer: #6821
Refer: #9071
PR-URL: #15107
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell pushed a commit that referenced this issue Sep 20, 2017
It is more secure to verify SHASUMS256.txt files via SHASUMS256.txt.sig
than SHASUMS256.txt.asc.

This comment does the best job at explaining the issue:
  #6821 (comment)

Refer: #6821
Refer: #9071
PR-URL: #15107
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 17, 2017
It is more secure to verify SHASUMS256.txt files via SHASUMS256.txt.sig
than SHASUMS256.txt.asc.

This comment does the best job at explaining the issue:
  #6821 (comment)

Refer: #6821
Refer: #9071
PR-URL: #15107
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 25, 2017
It is more secure to verify SHASUMS256.txt files via SHASUMS256.txt.sig
than SHASUMS256.txt.asc.

This comment does the best job at explaining the issue:
  #6821 (comment)

Refer: #6821
Refer: #9071
PR-URL: #15107
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nurupo
Copy link

nurupo commented May 27, 2019

It might be worth mentioning that one shouldn't rely on the exit code of gpg --verify, as it exits with 0, i.e. success, even if the key has been revoked or expired, as long as the signature was made with that key. So if someone steals nodejs's release key, makes a malicious release with it and nodejs people revoke it, gpg --verify would still return 0 even with gpg knowing that the key is revoked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. meta Issues and PRs related to the general management of the project. security Issues and PRs related to security. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging a pull request may close this issue.