Skip to content

Commit fd3c87e

Browse files
panvamcollina
authored andcommitted
crypto: harden X.509 conversion and formatting
Check X.509 ASN.1, BIO, time, OID, and DER conversion results. Preserve invalid-time diagnostics, reject incomplete BIO writes, format RSA exponents atomically, and avoid truncating long object identifiers. Preserve valid siblings in legacy name objects by omitting individual non-text AttributeValues. Continue treating structural failures and failed conversion of supported string types as conversion failures. Reject empty or malformed SAN and AIA extensions, and prevent TLS hostname checks from falling back to commonName after a malformed SAN. Guard missing public keys and non-NIST curve names. Scope fallible backend probes without consuming callback errors, parse checkIP() inputs to exact address bytes, and preserve unrelated backend errors. Refs: https://hackerone.com/reports/3910015 Refs: https://hackerone.com/reports/3824331 Refs: https://hackerone.com/reports/3944830 Refs: https://hackerone.com/reports/3944847 Assisted-by: Codex Co-authored-by: Matteo Collina <hello@matteocollina.com> Signed-off-by: Matteo Collina <hello@matteocollina.com> Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: nodejs-private/node-private#966 Refs: https://hackerone.com/reports/3910015 Refs: https://hackerone.com/reports/3824331 Refs: https://hackerone.com/reports/3944830 Refs: https://hackerone.com/reports/3944847 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 0392385 commit fd3c87e

8 files changed

Lines changed: 1072 additions & 200 deletions

File tree

‎deps/ncrypto/ncrypto.cc‎

Lines changed: 322 additions & 153 deletions
Large diffs are not rendered by default.

‎deps/ncrypto/ncrypto.h‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -917,10 +917,37 @@ class BIOPointer final {
917917

918918
static int Write(BIOPointer* bio, std::string_view message);
919919

920+
// Writes the complete message and returns false if any bytes were not
921+
// written.
922+
static bool WriteAll(const BIOPointer& bio, std::string_view message);
923+
924+
// Returns the number of bytes written, or a negative value on error.
925+
template <typename... Args>
926+
static int Printf(BIOPointer* bio, const char* format, Args... args) {
927+
if (bio == nullptr || !*bio) return -1;
928+
return BIO_printf(bio->get(), format, std::forward<Args>(args)...);
929+
}
930+
931+
// Formats and writes the complete message, returning false if formatting
932+
// fails or any bytes were not written.
920933
template <typename... Args>
921-
static void Printf(BIOPointer* bio, const char* format, Args... args) {
922-
if (bio == nullptr || !*bio) return;
923-
BIO_printf(bio->get(), format, std::forward<Args...>(args...));
934+
static bool PrintfAll(const BIOPointer& bio,
935+
const char* format,
936+
Args... args) {
937+
if (!bio) return false;
938+
939+
BIOPointer formatted = NewMem();
940+
if (!formatted) return false;
941+
int length =
942+
BIO_printf(formatted.get(), format, std::forward<Args>(args)...);
943+
if (length < 0) return false;
944+
945+
BUF_MEM* mem = formatted;
946+
if (mem == nullptr || mem->length != static_cast<size_t>(length)) {
947+
return false;
948+
}
949+
if (length == 0) return true;
950+
return WriteAll(bio, std::string_view(mem->data, mem->length));
924951
}
925952

926953
private:
@@ -1573,6 +1600,13 @@ class SSLPointer final {
15731600

15741601
class X509Name final {
15751602
public:
1603+
// A missing Entry means the name entry could not be inspected. A missing
1604+
// value marks an AttributeValue that the legacy UTF-8 view cannot represent.
1605+
struct Entry {
1606+
std::string key;
1607+
std::optional<std::string> value;
1608+
};
1609+
15761610
X509Name();
15771611
explicit X509Name(const X509_NAME* name);
15781612
NCRYPTO_DISALLOW_COPY_AND_MOVE(X509Name)
@@ -1593,7 +1627,7 @@ class X509Name final {
15931627
operator bool() const;
15941628
bool operator==(const Iterator& other) const;
15951629
bool operator!=(const Iterator& other) const;
1596-
std::pair<std::string, std::string> operator*() const;
1630+
std::optional<Entry> operator*() const;
15971631

15981632
private:
15991633
const X509Name& name_;
@@ -1632,15 +1666,15 @@ class X509View final {
16321666
const X509Name getSubjectName() const;
16331667
const X509Name getIssuerName() const;
16341668
BIOPointer getSubject() const;
1635-
BIOPointer getSubjectAltName() const;
1669+
BIOPointer getSubjectAltName(bool* has_extension = nullptr) const;
16361670
BIOPointer getIssuer() const;
16371671
BIOPointer getInfoAccess() const;
16381672
BIOPointer getValidFrom() const;
16391673
BIOPointer getValidTo() const;
16401674
std::optional<std::string_view> getSignatureAlgorithm() const;
16411675
std::optional<std::string> getSignatureAlgorithmOID() const;
1642-
int64_t getValidFromTime() const;
1643-
int64_t getValidToTime() const;
1676+
std::optional<int64_t> getValidFromTime() const;
1677+
std::optional<int64_t> getValidToTime() const;
16441678
DataPointer getSerialNumber() const;
16451679
Result<EVPKeyPointer, int> getPublicKey() const;
16461680
StackOfASN1 getKeyUsage() const;
@@ -1664,7 +1698,7 @@ class X509View final {
16641698
int flags,
16651699
DataPointer* peerName = nullptr) const;
16661700
CheckMatch checkEmail(std::string_view email, int flags) const;
1667-
CheckMatch checkIp(std::string_view ip, int flags) const;
1701+
CheckMatch checkIp(const Buffer<const unsigned char>& ip, int flags) const;
16681702

16691703
using UsageCallback = std::function<void(const char*)>;
16701704
bool enumUsages(UsageCallback callback) const;

‎src/crypto/crypto_x509.cc‎

Lines changed: 121 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,13 @@
88
#include "ncrypto.h"
99
#include "node_errors.h"
1010
#include "util-inl.h"
11+
#include "uv.h"
1112
#include "v8.h"
1213

14+
#include <array>
15+
#include <limits>
1316
#include <string>
17+
#include <string_view>
1418
#include <vector>
1519

1620
namespace node {
@@ -20,6 +24,7 @@ using ncrypto::BIOPointer;
2024
using ncrypto::ClearErrorOnReturn;
2125
using ncrypto::DataPointer;
2226
using ncrypto::Digest;
27+
using ncrypto::MarkPopErrorOnReturn;
2328
using ncrypto::SSLPointer;
2429
using ncrypto::X509Name;
2530
using ncrypto::X509Pointer;
@@ -65,12 +70,20 @@ ManagedX509& ManagedX509::operator=(const ManagedX509& that) {
6570

6671
void ManagedX509::MemoryInfo(MemoryTracker* tracker) const {
6772
if (!cert_) return;
73+
MarkPopErrorOnReturn mark_pop_error_on_return;
6874
// This is an approximation based on the der encoding size.
69-
int size = i2d_X509(cert_.get(), nullptr);
70-
tracker->TrackFieldWithSize("cert", size);
75+
const int size = i2d_X509(cert_.get(), nullptr);
76+
if (size > 0) tracker->TrackFieldWithSize("cert", size);
7177
}
7278

7379
namespace {
80+
enum class MalformedSubjectAltNameMode {
81+
kThrow,
82+
kReturnLegacySentinel,
83+
};
84+
85+
constexpr std::string_view kMalformedSubjectAltName = "DNS:";
86+
7487
MaybeLocal<Value> GetFingerprintDigest(Environment* env,
7588
const Digest& method,
7689
const X509View& cert) {
@@ -167,12 +180,29 @@ MaybeLocal<Value> GetDer(Environment* env, const X509View& view) {
167180
return ret;
168181
}
169182

170-
MaybeLocal<Value> GetSubjectAltNameString(Environment* env,
171-
const X509View& view) {
183+
MaybeLocal<Value> GetSubjectAltNameString(
184+
Environment* env,
185+
const X509View& view,
186+
MalformedSubjectAltNameMode malformed_name_mode) {
172187
Local<Value> ret;
173-
auto bio = view.getSubjectAltName();
174-
if (!bio) [[unlikely]]
175-
return Undefined(env->isolate());
188+
bool has_extension = false;
189+
auto bio = view.getSubjectAltName(&has_extension);
190+
if (!bio) [[unlikely]] {
191+
// An absent SAN maps to undefined. A malformed, present SAN throws for
192+
// direct X509Certificate inspection, while the legacy TLS object uses a
193+
// deliberately nonmatching value to prevent commonName fallback.
194+
if (!has_extension) return Undefined(env->isolate());
195+
if (malformed_name_mode ==
196+
MalformedSubjectAltNameMode::kReturnLegacySentinel) {
197+
// The legacy object is used by TLS hostname verification. Return an
198+
// empty DNS name so verification cannot fall back to the common name,
199+
// while keeping getPeerCertificate() usable for malformed certificates.
200+
return OneByteString(env->isolate(), kMalformedSubjectAltName);
201+
}
202+
THROW_ERR_CRYPTO_OPERATION_FAILED(
203+
env, "Failed to format subject alternative name");
204+
return {};
205+
}
176206
if (!ToV8Value(env->context(), bio).ToLocal(&ret)) return {};
177207
return ret;
178208
}
@@ -211,13 +241,19 @@ MaybeLocal<Value> GetValidTo(Environment* env, const X509View& view) {
211241
}
212242

213243
MaybeLocal<Value> GetValidFromDate(Environment* env, const X509View& view) {
214-
int64_t validFromTime = view.getValidFromTime();
215-
return Date::New(env->context(), validFromTime * 1000.);
244+
const auto valid_from_time = view.getValidFromTime();
245+
const double milliseconds = valid_from_time.has_value()
246+
? valid_from_time.value() * 1000.
247+
: std::numeric_limits<double>::quiet_NaN();
248+
return Date::New(env->context(), milliseconds);
216249
}
217250

218251
MaybeLocal<Value> GetValidToDate(Environment* env, const X509View& view) {
219-
int64_t validToTime = view.getValidToTime();
220-
return Date::New(env->context(), validToTime * 1000.);
252+
const auto valid_to_time = view.getValidToTime();
253+
const double milliseconds = valid_to_time.has_value()
254+
? valid_to_time.value() * 1000.
255+
: std::numeric_limits<double>::quiet_NaN();
256+
return Date::New(env->context(), milliseconds);
221257
}
222258

223259
MaybeLocal<Value> GetSignatureAlgorithm(Environment* env,
@@ -296,7 +332,9 @@ void SubjectAltName(const FunctionCallbackInfo<Value>& args) {
296332
X509Certificate* cert;
297333
ASSIGN_OR_RETURN_UNWRAP(&cert, args.This());
298334
Local<Value> ret;
299-
if (GetSubjectAltNameString(env, cert->view()).ToLocal(&ret)) {
335+
if (GetSubjectAltNameString(
336+
env, cert->view(), MalformedSubjectAltNameMode::kThrow)
337+
.ToLocal(&ret)) {
300338
args.GetReturnValue().Set(ret);
301339
}
302340
}
@@ -517,11 +555,29 @@ void CheckEmail(const FunctionCallbackInfo<Value>& args) {
517555
}
518556

519557
void CheckIP(const FunctionCallbackInfo<Value>& args) {
520-
CheckX509Subject(args,
521-
[](X509View& cert,
522-
std::string_view subject,
523-
uint32_t flags,
524-
DataPointer*) { return cert.checkIp(subject, flags); });
558+
CheckX509Subject(
559+
args,
560+
[](X509View& cert,
561+
std::string_view subject,
562+
uint32_t flags,
563+
DataPointer*) {
564+
if (subject.find('\0') != std::string_view::npos ||
565+
subject.find('%') != std::string_view::npos) {
566+
return X509View::CheckMatch::INVALID_NAME;
567+
}
568+
569+
const std::string input(subject);
570+
std::array<unsigned char, 16> address{};
571+
size_t length = 0;
572+
if (uv_inet_pton(AF_INET, input.c_str(), address.data()) == 0) {
573+
length = 4;
574+
} else if (uv_inet_pton(AF_INET6, input.c_str(), address.data()) == 0) {
575+
length = 16;
576+
} else {
577+
return X509View::CheckMatch::INVALID_NAME;
578+
}
579+
return cert.checkIp({.data = address.data(), .len = length}, flags);
580+
});
525581
}
526582

527583
void GetIssuerCert(const FunctionCallbackInfo<Value>& args) {
@@ -606,9 +662,20 @@ static MaybeLocal<Value> GetX509NameObject(Environment* env,
606662
Object::New(env->isolate(), Null(env->isolate()), nullptr, nullptr, 0);
607663
if (result.IsEmpty()) return {};
608664

609-
for (auto i : name) {
610-
if (!ToV8Value(env, i.first).ToLocal(&v8_name) ||
611-
!ToV8Value(env, i.second).ToLocal(&v8_value)) {
665+
for (auto entry : name) {
666+
if (!entry.has_value()) [[unlikely]] {
667+
THROW_ERR_CRYPTO_OPERATION_FAILED(env,
668+
"Failed to convert X509 name to UTF-8");
669+
return {};
670+
}
671+
672+
const auto& [key, value] = *entry;
673+
// The legacy object can only represent string values. Preserve valid
674+
// sibling attributes and omit values that cannot be converted to UTF-8.
675+
if (!value.has_value()) continue;
676+
677+
if (!ToV8Value(env, key).ToLocal(&v8_name) ||
678+
!ToV8Value(env, *value).ToLocal(&v8_value)) {
612679
return {};
613680
}
614681

@@ -669,11 +736,13 @@ MaybeLocal<Value> GetModulusString(Environment* env, const BIGNUM* n) {
669736

670737
MaybeLocal<Value> GetExponentString(Environment* env, const BIGNUM* e) {
671738
if (e == nullptr) return Null(env->isolate());
739+
MarkPopErrorOnReturn mark_pop_error_on_return;
672740
auto bio = BIOPointer::NewMem();
673741
if (!bio) [[unlikely]]
674742
return {};
675-
BIO_puts(bio.get(), "0x");
676-
BN_print(bio.get(), e);
743+
if (!BIOPointer::WriteAll(bio, "0x") || BN_print(bio.get(), e) != 1) {
744+
return {};
745+
}
677746
return ToV8Value(env->context(), bio);
678747
}
679748

@@ -699,7 +768,9 @@ MaybeLocal<Value> GetECGroupBits(Environment* env, const EC_GROUP* group) {
699768

700769
template <const char* (*nid2string)(int nid)>
701770
MaybeLocal<Value> GetCurveName(Environment* env, const int nid) {
702-
std::string_view name = nid2string(nid);
771+
const char* const curve_name = nid2string(nid);
772+
if (curve_name == nullptr) return Undefined(env->isolate());
773+
const std::string_view name = curve_name;
703774
return name.size() ? MaybeLocal<Value>(OneByteString(env->isolate(), name))
704775
: MaybeLocal<Value>(Undefined(env->isolate()));
705776
}
@@ -734,10 +805,21 @@ MaybeLocal<Object> X509ToObject(Environment* env, const X509View& cert) {
734805
env->set_x509_dictionary_template(tmpl);
735806
}
736807

808+
Local<Value> subject;
809+
if (!GetX509NameObject(env, cert.getSubjectName()).ToLocal(&subject)) {
810+
return {};
811+
}
812+
813+
Local<Value> issuer;
814+
if (!GetX509NameObject(env, cert.getIssuerName()).ToLocal(&issuer)) {
815+
return {};
816+
}
817+
737818
MaybeLocal<Value> values[] = {
738-
GetX509NameObject(env, cert.getSubjectName()),
739-
GetX509NameObject(env, cert.getIssuerName()),
740-
GetSubjectAltNameString(env, cert),
819+
subject,
820+
issuer,
821+
GetSubjectAltNameString(
822+
env, cert, MalformedSubjectAltNameMode::kReturnLegacySentinel),
741823
GetInfoAccessString(env, cert),
742824
Boolean::New(env->isolate(), cert.isCA()),
743825
Undefined(env->isolate()), // modulus
@@ -756,6 +838,19 @@ MaybeLocal<Object> X509ToObject(Environment* env, const X509View& cert) {
756838
Undefined(env->isolate()), // nistcurve
757839
};
758840

841+
// X509_get0_pubkey() fails when the linked OpenSSL cannot decode the
842+
// certificate's public key (e.g. an unsupported curve in FIPS builds).
843+
// ifRsa()/ifEc() assume a decodable key, so leave the key-specific
844+
// fields undefined in that case.
845+
const bool has_public_key = [&] {
846+
MarkPopErrorOnReturn mark_pop_error_on_return;
847+
return X509_get0_pubkey(cert.get()) != nullptr;
848+
}();
849+
if (!has_public_key) {
850+
return scope.EscapeMaybe(
851+
NewDictionaryInstance(env->context(), tmpl, values));
852+
}
853+
759854
cert.ifRsa([&](const ncrypto::Rsa& rsa) {
760855
auto pub_key = rsa.getPublicKey();
761856
values[5] = GetModulusString(env, pub_key.n); // modulus

0 commit comments

Comments
 (0)