electron icon indicating copy to clipboard operation
electron copied to clipboard

docs: remove claim that HTTPS authenticates the remote server

Open Prinzhorn opened this issue 2 years ago • 1 comments

Description of Change

Removed the claim that HTTPS authenticates the remote server. This is not a feature of HTTPS. This would require certificate pinning. It has been in the security docs since https://github.com/electron/electron/commit/2db125890cf901f939ab60b550dee54a091d57a4

Related

https://github.com/electron/electron/issues/3330 https://www.npmjs.com/package/electron-ssl-pinning https://cheatsheetseries.owasp.org/cheatsheets/Pinning_Cheat_Sheet.html

Checklist

  • [x] relevant documentation is changed or added

Release Notes

Notes: none

Prinzhorn avatar Aug 31 '22 16:08 Prinzhorn

should we add a note about cert pinning here?

I don't know if any Electron apps actually successfully employ certificate pinning and how relevant it is for desktop applications (I think mobile/wifi was historically the most vulnerable). From the top of my head I think the most critical part in an Electron app for certificate pinning would be the auto updater. But I think signing already solves this? (Someone would need my private key that I use to sign releases, it's not enough to compromise the update server).

Also I can already see the news articles like "Slack down globally" because they deployed a new certificate without updating the desktop app to include it in the trusted list. Pinning comes with a host of new problems and is hard to get right I think (see https://en.wikipedia.org/wiki/HTTP_Public_Key_Pinning for example).

But I think that's material for a different discussion? Or maybe you can ping someone more knowledgeable?

I really tend to write too much :smile: . Maybe we can just link to https://owasp.org/www-community/controls/Certificate_and_Public_Key_Pinning and tell people it's out of scope for Electron to get into more detail.

Prinzhorn avatar Sep 13 '22 08:09 Prinzhorn

Merging as CI failure unrelated to PR change.

jkleinsc avatar Sep 21 '22 20:09 jkleinsc

No Release Notes

release-clerk[bot] avatar Sep 21 '22 20:09 release-clerk[bot]