search

lastpass / lastpass-cli

LastPass command line interface tool
GNU General Public License v2.0
2.85k stars 289 forks source link

add root CA to pins; fix cert pinning check #660

Closed milo-minderbinder closed 2 months ago

milo-minderbinder commented 10 months ago

Previously, the cert pinning checks in verify_callback() were redundant, because verify_callback() is called for every cert in the chain, starting with the root and progressing through to the peer cert (see docs & example: https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_verify.html), and the entire chain was checked against the pinned certs with each invocation of verify_callback(). This change ensures the chain is checked against the pinned certs only after each cert in the chain has been validated using the normal platform validation. It also adds the current root CA to the pinned certs.

milo-minderbinder commented 10 months ago

Incidentally, I'm not sure if this was actually the intended behavior of the cert pinning checks in the first place. If the intent was that every cert in the chain should match one of the pinned certs, neither this fixed version, nor the previous version, actually did that. I have an alternate version of this fix which does validate that every cert in the chain matches one of the pinned certs, so let me know if I should apply those changes to the PR as well.

mateusmartins-lp commented 1 month ago

Regrettably, your PR submission was unintentionally closed during an operation, before we could complete our review and respond accordingly. Unable to revert it to 'Open' status, we invite you to resubmit your contribution at your earliest convenience. We apologize for this mishap. Rest assured, we value all contributions and remain dedicated to providing transparency and closure to the community. Thank you for your understanding.