Skip to content
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
255b45c
Provide X509 certificate manipulation functions
yadij May 17, 2022
6a0d479
fixup: Assert no nil LockingPointer dereference (a branch TODO)
rousskov May 17, 2022
707b56b
fixup: Renamed src/security/CertGadgets.* to .../Certificate.*
rousskov May 17, 2022
5e5f9a3
fixup: Moved certificate printing operator declaration to Certificate.h
rousskov May 17, 2022
527c752
fixup: Removed unnecessary Cert prefix from Certificate info getters
rousskov May 17, 2022
585dc84
fixup: Fixed OpenSSL error stack handling in new functions
rousskov May 17, 2022
37f5c4d
fixup: Do not report successfully extracted certificate fields
rousskov May 17, 2022
e64ea89
fixup: Formatted modified sources
rousskov May 17, 2022
10c24f1
fixup: Avoid Foo:: inside Foo
rousskov May 17, 2022
9a3f84d
fixup: Polished branch-added source code comments
rousskov May 17, 2022
7b19ec7
Use newly added Security::IsIssuedBy() and Security::IsSelfSigned()
rousskov May 17, 2022
651bdfd
Use newly added X509_NAME_oneline() replacements, where feasible
rousskov May 17, 2022
fdfcba3
fiuxp: Simplified conditional #includes
rousskov May 18, 2022
f77814e
fixup: Highlight that we print OpenSSL-saved errors on their own lines
rousskov May 18, 2022
d9061dc
fixup: const-correctness
rousskov May 18, 2022
4dc60c3
fixup: Polished variable names
rousskov May 18, 2022
2a3cac8
fixup: Undo unnatural squid_LDADD reordering
rousskov May 18, 2022
0dbbf52
fixup: Found better names for two new functions
rousskov May 18, 2022
9b02b6b
fixup: Polished new function descriptions
rousskov May 18, 2022
b511fb5
fixup: Removed known memory leaks in branch-added OpenSSL code
rousskov May 18, 2022
eadc2ba
fixup: Avoid duplicating UniqueCString-related code
rousskov May 18, 2022
4c16b6c
fixup: Rewrapped to avoid arguing; also fixed the now-modified debugs()
rousskov May 18, 2022
9846104
fixup: Removed a likely misleading TODO
rousskov May 18, 2022
6b13931
fixup: Fix build [-Wunused-parameter]
rousskov May 20, 2022
cd79f49
Merge branch 'master' into SQUID-642-cert-manipulators
rousskov May 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/client_side.cc
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
#include "proxyp/Header.h"
#include "proxyp/Parser.h"
#include "sbuf/Stream.h"
#include "security/Certificate.h"
#include "security/CommunicationSecrets.h"
#include "security/Io.h"
#include "security/KeyLog.h"
Expand Down Expand Up @@ -2423,10 +2424,10 @@ clientNegotiateSSL(int fd, void *data)

if (client_cert) {
debugs(83, 3, "FD " << fd << " client certificate: subject: " <<
X509_NAME_oneline(X509_get_subject_name(client_cert), 0, 0));
Security::SubjectName(*client_cert));

debugs(83, 3, "FD " << fd << " client certificate: issuer: " <<
X509_NAME_oneline(X509_get_issuer_name(client_cert), 0, 0));
Security::IssuerName(*client_cert));

X509_free(client_cert);
} else {
Expand Down
17 changes: 7 additions & 10 deletions src/format/Format.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "sbuf/Stream.h"
#include "sbuf/StringConvert.h"
#include "security/CertError.h"
#include "security/Certificate.h"
#include "security/NegotiationHistory.h"
#include "Store.h"
#include "tools.h"
Expand Down Expand Up @@ -1253,20 +1254,16 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
break;

case LFT_SSL_USER_CERT_SUBJECT:
if (X509 *cert = al->cache.sslClientCert.get()) {
if (X509_NAME *subject = X509_get_subject_name(cert)) {
X509_NAME_oneline(subject, tmp, sizeof(tmp));
out = tmp;
}
if (const auto &cert = al->cache.sslClientCert) {
sb = Security::SubjectName(*cert);
out = sb.c_str();
}
break;

case LFT_SSL_USER_CERT_ISSUER:
if (X509 *cert = al->cache.sslClientCert.get()) {
if (X509_NAME *issuer = X509_get_issuer_name(cert)) {
X509_NAME_oneline(issuer, tmp, sizeof(tmp));
out = tmp;
}
if (const auto &cert = al->cache.sslClientCert) {
sb = Security::IssuerName(*cert);
out = sb.c_str();
}
break;

Expand Down
137 changes: 137 additions & 0 deletions src/security/Certificate.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
* Copyright (C) 1996-2022 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/

#include "squid.h"
#include "debug/Stream.h"
#include "sbuf/SBuf.h"
#include "security/Certificate.h"

#if USE_OPENSSL
#include "ssl/gadgets.h"
#endif

#include <iostream>

inline
const char *
MissingLibraryError()
{
return "[need OpenSSL or GnuTLS]";
}

SBuf
Security::IssuerName(Certificate &cert)
{
SBuf out;

#if USE_OPENSSL
Ssl::ForgetErrors();
const auto name = Ssl::OneLineSummary(*X509_get_issuer_name(&cert));
if (!name) {
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate Issuer:" <<
Ssl::ReportAndForgetErrors);
return out;
}
out.append(name.get());

#elif USE_GNUTLS
gnutls_x509_dn_t issuer;
auto x = gnutls_x509_crt_get_issuer(&cert, &issuer);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate Issuer: " << ErrorString(x));
return out;
}

gnutls_datum_t name;
x = gnutls_x509_dn_get_str(issuer, &name);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot describe certificate Issuer: " << ErrorString(x));
return out;
}
out.append(reinterpret_cast<const char *>(name.data), name.size);
gnutls_free(name.data);

#else
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate Issuer: " << MissingLibraryError());
#endif

return out;
}

SBuf
Security::SubjectName(Certificate &cert)
{
SBuf out;

#if USE_OPENSSL
Ssl::ForgetErrors();
const auto name = Ssl::OneLineSummary(*X509_get_subject_name(&cert));
if (!name) {
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate SubjectName:" <<
Ssl::ReportAndForgetErrors);
return out;
}
out.append(name.get());

#elif USE_GNUTLS
gnutls_x509_dn_t subject;
auto x = gnutls_x509_crt_get_subject(&cert, &subject);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate SubjectName: " << ErrorString(x));
return out;
}

gnutls_datum_t name;
x = gnutls_x509_dn_get_str(subject, &name);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot describe certificate SubjectName: " << ErrorString(x));
return out;
}
out.append(reinterpret_cast<const char *>(name.data), name.size);
gnutls_free(name.data);
Comment thread
yadij marked this conversation as resolved.

#else
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate SubjectName: " << MissingLibraryError());
#endif

return out;
}

bool
Security::IssuedBy(Certificate &cert, Certificate &issuer)
{
#if USE_OPENSSL
Ssl::ForgetErrors();
const auto result = X509_check_issued(&issuer, &cert);
if (result == X509_V_OK)
return true;
debugs(83, DBG_PARSE_NOTE(3), issuer << " did not sign " << cert << ":" <<
Debug::Extra << "X509_check_issued() result: " << X509_verify_cert_error_string(result) << " (" << result << ")" <<
Ssl::ReportAndForgetErrors);
#elif USE_GNUTLS
const auto result = gnutls_x509_crt_check_issuer(&cert, &issuer);
if (result == 1)
return true;
debugs(83, DBG_PARSE_NOTE(3), issuer << " did not sign " << cert);
#else
debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot determine certificates relationship: " << MissingLibraryError());
#endif
return false;
}

std::ostream &
operator <<(std::ostream &os, Security::Certificate &cert)
{
const auto name = Security::SubjectName(cert);
if (name.isEmpty())
os << "[no subject name]";
else
os << name;
return os;
}

43 changes: 43 additions & 0 deletions src/security/Certificate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (C) 1996-2022 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/

#ifndef SQUID_SRC_SECURITY_CERTIFICATE_H
#define SQUID_SRC_SECURITY_CERTIFICATE_H

#include "security/forward.h"

// The accessing/testing functions below require a non-constant Certificate when
// it is modified by an underlying library implementation (e.g., GnuTLS).

namespace Security
{

/// The SubjectName field of the given certificate (if found) or an empty SBuf.
SBuf SubjectName(Certificate &);

/// The Issuer field of the given certificate (if found) or an empty SBuf.
SBuf IssuerName(Certificate &);

/// Whether cert was (correctly) issued by the given issuer.
/// Due to complexity of the underlying checks, it is impossible to clearly
/// distinguish pure negative answers (e.g., two independent certificates)
/// from errors (e.g., the issuer certificate lacks the right CA extension).
bool IssuedBy(Certificate &cert, Certificate &issuer);

/// Whether the given certificate is self-signed.
inline bool SelfSigned(Certificate &c) { return IssuedBy(c, c); }

} // namespace Security

// Declared outside Security because all underlying Security::Certificate types
// are declared inside global namespace.
/// reports a one-line gist of the Certificate Subject Name (for debugging)
std::ostream &operator <<(std::ostream &, Security::Certificate &);

#endif /* SQUID_SRC_SECURITY_CERTIFICATE_H */

21 changes: 9 additions & 12 deletions src/security/ErrorDetail.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "html_quote.h"
#include "sbuf/SBuf.h"
#include "sbuf/Stream.h"
#include "security/Certificate.h"
#include "security/ErrorDetail.h"
#include "security/forward.h"
#include "security/Io.h"
Expand Down Expand Up @@ -561,16 +562,14 @@ Security::ErrorDetail::verbose(const HttpRequestPointer &request) const
const char *
Security::ErrorDetail::subject() const
{
#if USE_OPENSSL
if (broken_cert.get()) {
static char tmpBuffer[256]; // A temporary buffer
if (X509_NAME_oneline(X509_get_subject_name(broken_cert.get()), tmpBuffer, sizeof(tmpBuffer))) {
if (broken_cert) {
auto buf = SubjectName(*broken_cert);
if (!buf.isEmpty()) {
// quote to avoid possible html code injection through
// certificate subject
return html_quote(tmpBuffer);
return html_quote(buf.c_str());
}
}
#endif // USE_OPENSSL
return "[Not available]";
}

Expand Down Expand Up @@ -614,16 +613,14 @@ Security::ErrorDetail::cn() const
const char *
Security::ErrorDetail::ca_name() const
{
#if USE_OPENSSL
if (broken_cert.get()) {
static char tmpBuffer[256]; // A temporary buffer
if (X509_NAME_oneline(X509_get_issuer_name(broken_cert.get()), tmpBuffer, sizeof(tmpBuffer))) {
if (broken_cert) {
auto buf = IssuerName(*broken_cert);
if (!buf.isEmpty()) {
// quote to avoid possible html code injection through
// certificate issuer subject
return html_quote(tmpBuffer);
return html_quote(buf.c_str());
}
}
#endif // USE_OPENSSL
return "[Not available]";
}

Expand Down
22 changes: 8 additions & 14 deletions src/security/KeyData.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "squid.h"
#include "anyp/PortCfg.h"
#include "fatal.h"
#include "security/Certificate.h"
#include "security/KeyData.h"
#include "SquidConfig.h"
#include "ssl/bio.h"
Expand Down Expand Up @@ -97,10 +98,8 @@ Security::KeyData::loadX509ChainFromFile()
}

#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain
if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) {
char *nameStr = X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0);
debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << nameStr);
OPENSSL_free(nameStr);
if (SelfSigned(*cert)) {
debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << *cert);
} else
#endif
{
Expand All @@ -109,29 +108,24 @@ Security::KeyData::loadX509ChainFromFile()
CertPointer latestCert = cert;

while (const auto ca = Ssl::ReadOptionalCertificate(bio)) {
// get Issuer name of the cert for debug display
char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca.get()), nullptr, 0);

#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain
// self-signed certificates are not valid in a sent chain
if (X509_check_issued(ca.get(), ca.get()) == X509_V_OK) {
debugs(83, DBG_PARSE_NOTE(2), "CA " << nameStr << " is self-signed, will not be chained: " << nameStr);
OPENSSL_free(nameStr);
if (SelfSigned(*ca)) {
debugs(83, DBG_PARSE_NOTE(2), "CA certificate is self-signed, will not be chained: " << *ca);
continue;
}
#endif
// checks that the chained certs are actually part of a chain for validating cert
const auto checkCode = X509_check_issued(ca.get(), latestCert.get());
if (checkCode == X509_V_OK) {
debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << nameStr);
if (IssuedBy(*latestCert, *ca)) {
debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << *ca);
// OpenSSL API requires that we order certificates such that the
// chain can be appended directly into the on-wire traffic.
latestCert = CertPointer(ca);
chain.emplace_back(latestCert);
} else {
debugs(83, DBG_PARSE_NOTE(2), certFile << ": Ignoring non-issuer CA " << nameStr << ": " << X509_verify_cert_error_string(checkCode) << " (" << checkCode << ")");
debugs(83, DBG_PARSE_NOTE(2), certFile << ": Ignoring non-issuer CA " << *ca);
}
OPENSSL_free(nameStr);
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/security/LockingPointer.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef SQUID_SRC_SECURITY_LOCKINGPOINTER_H
#define SQUID_SRC_SECURITY_LOCKINGPOINTER_H

#include "base/Assure.h"
#include "base/HardFun.h"

#if USE_OPENSSL
Expand Down Expand Up @@ -100,6 +101,7 @@ class LockingPointer
bool operator ==(const SelfType &o) const { return (o.get() == raw); }
bool operator !=(const SelfType &o) const { return (o.get() != raw); }

T &operator *() const { Assure(raw); return *raw; }
T *operator ->() const { return raw; }

/// Returns raw and possibly nullptr pointer
Expand Down
2 changes: 2 additions & 0 deletions src/security/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ libsecurity_la_SOURCES = \
BlindPeerConnector.cc \
BlindPeerConnector.h \
CertError.h \
Certificate.cc \
Certificate.h \
CommunicationSecrets.cc \
CommunicationSecrets.h \
Context.h \
Expand Down
Loading