Skip to content

imap-send: explicitly verify the peer certificate#1886

Closed
dscho wants to merge 1 commit intogitgitgadget:masterfrom
dscho:imap-send-verify-peer-certificate
Closed

imap-send: explicitly verify the peer certificate#1886
dscho wants to merge 1 commit intogitgitgadget:masterfrom
dscho:imap-send-verify-peer-certificate

Conversation

@dscho
Copy link
Copy Markdown
Member

@dscho dscho commented Mar 24, 2025

No description provided.

It is a bug to obtain the peer certificate without verifying it.

Having said that, from my reading of
https://www.openssl.org/docs/man1.1.1/man3/SSL_set_verify.html, it would
appear that Git is saved by the fact that it calls
`SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL)` already early on.

In other words, that `SSL_VERIFY_PEER` combined with the `NULL`
parameter (i.e. no overridden callback) would _already_ verify the peer
certificate.  The fact that we later call `SSL_get_peer_certificate()`
is mistaken by CodeQL to mean that that peer certificate still needs to
be verified, but that had already happened at that point.

Nevertheless, it is better to verify the peer certificate explicitly
than to rely on some side effect that is really hard to reason about
(and that took me more than one business day to analyze fully). It also
makes it easier for static analyzers to validate the correctness of the
code.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho self-assigned this Mar 24, 2025
@dscho
Copy link
Copy Markdown
Member Author

dscho commented Mar 24, 2025

/submit

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Mar 24, 2025

Submitted as pull.1886.git.1742819282360.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1886/dscho/imap-send-verify-peer-certificate-v1

To fetch this version to local tag pr-1886/dscho/imap-send-verify-peer-certificate-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1886/dscho/imap-send-verify-peer-certificate-v1

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Mar 25, 2025

This patch series was integrated into seen via git@702f63f.

@gitgitgadget gitgitgadget Bot added the seen label Mar 25, 2025
@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Mar 28, 2025

This branch is now known as js/imap-send-peer-cert-verify.

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Mar 28, 2025

This patch series was integrated into seen via git@f397b7c.

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Mar 28, 2025

This patch series was integrated into next via git@69df4dd.

@gitgitgadget gitgitgadget Bot added the next label Mar 28, 2025
@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Mar 29, 2025

This patch series was integrated into seen via git@54684bf.

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Apr 7, 2025

There was a status update in the "Cooking" section about the branch js/imap-send-peer-cert-verify on the Git mailing list:

Will merge to 'master'.
source: <pull.1886.git.1742819282360.gitgitgadget@gmail.com>

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Apr 7, 2025

This patch series was integrated into seen via git@7b420ef.

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Apr 7, 2025

This patch series was integrated into master via git@7b420ef.

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Apr 7, 2025

This patch series was integrated into next via git@7b420ef.

@gitgitgadget gitgitgadget Bot added the master label Apr 7, 2025
@gitgitgadget gitgitgadget Bot closed this Apr 7, 2025
@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented Apr 7, 2025

Closed via 7b420ef.

@dscho dscho deleted the imap-send-verify-peer-certificate branch April 9, 2025 05:59
webstech added a commit that referenced this pull request Nov 5, 2025
…pes/node-22.15.3

build(deps-dev): bump @types/node from 22.14.1 to 22.15.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant