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

schannel: make CAinfo parsing resilient to CR/LF #2592

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@dscho
Contributor

dscho commented May 21, 2018

OpenSSL has supported --cacert for ages, always accepting LF-only line
endings ("Unix line endings") as well as CR/LF line endings ("Windows
line endings").

When we introduced support for --cacert also with Secure Channel (or in
cURL speak: "WinSSL"), we did not take care to support CR/LF line
endings, too, even if we are much more likely to receive input in that
form when using Windows.

Let's fix that.

Happily, CryptQueryObject(), the function we use to parse the ca-bundle,
accepts CR/LF input already, and the trailing LF before the END
CERTIFICATE marker catches naturally any CR/LF line ending, too. So all
we need to care about is the BEGIN CERTIFICATE marker. We do not
actually need to verify here that the line ending is CR/LF. Just
checking for a CR or an LF is really plenty enough.

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

schannel: make CAinfo parsing resilient to CR/LF
OpenSSL has supported --cacert for ages, always accepting LF-only line
endings ("Unix line endings") as well as CR/LF line endings ("Windows
line endings").

When we introduced support for --cacert also with Secure Channel (or in
cURL speak: "WinSSL"), we did not take care to support CR/LF line
endings, too, even if we are much more likely to receive input in that
form when using Windows.

Let's fix that.

Happily, CryptQueryObject(), the function we use to parse the ca-bundle,
accepts CR/LF input already, and the trailing LF before the END
CERTIFICATE marker catches naturally any CR/LF line ending, too. So all
we need to care about is the BEGIN CERTIFICATE marker. We do not
actually need to verify here that the line ending is CR/LF. Just
checking for a CR or an LF is really plenty enough.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho

This comment has been minimized.

Show comment
Hide comment
@dscho

dscho May 21, 2018

Contributor

/cc @mcnulty

Contributor

dscho commented May 21, 2018

/cc @mcnulty

@bagder bagder added the SSL/TLS label May 21, 2018

@mcnulty

This comment has been minimized.

Show comment
Hide comment
@mcnulty

mcnulty May 21, 2018

Contributor

Nice find, thanks @dscho!

FWIW, the fix looks good to me.

Contributor

mcnulty commented May 21, 2018

Nice find, thanks @dscho!

FWIW, the fix looks good to me.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 22, 2018

Member

(the broken CI builds are unrelated to this PR and I'm trying to fix them in #2594 )

Member

bagder commented May 22, 2018

(the broken CI builds are unrelated to this PR and I'm trying to fix them in #2594 )

@jay jay closed this in aa0f41a May 22, 2018

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 22, 2018

Member

Thanks. I modified it slightly to remove inline which we don't use since it isn't always available. If compiler optimization is enabled it may do it on its own.

Member

jay commented May 22, 2018

Thanks. I modified it slightly to remove inline which we don't use since it isn't always available. If compiler optimization is enabled it may do it on its own.

@dscho

This comment has been minimized.

Show comment
Hide comment
@dscho

dscho May 22, 2018

Contributor

Thank you!

Contributor

dscho commented May 22, 2018

Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 20, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.