Skip to content

Commit 2ff593a

Browse files
ryry
committed
TLS: better error reporting at binding layer
Closes nodejsGH-612.
1 parent ff7fc09 commit 2ff593a

File tree

3 files changed

+114
-82
lines changed

3 files changed

+114
-82
lines changed

‎lib/tls.js‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -192,11 +192,11 @@ CryptoStream.prototype._blow = function() {
192192
var pool = new Buffer(4096); // alloc every time?
193193

194194
do {
195-
try {
196-
chunkBytes = this._blower(pool, bytesRead, pool.length - bytesRead);
197-
} catch (e) {
195+
chunkBytes = this._blower(pool, bytesRead, pool.length - bytesRead);
196+
197+
if (this.pair._ssl && this.pair._ssl.error) {
198198
if (this.pair._secureEstablished) {
199-
this.pair._error(e);
199+
this.pair._error();
200200
} else {
201201
this.pair._destroy();
202202
}
@@ -244,7 +244,6 @@ CryptoStream.prototype._blow = function() {
244244
// });
245245
//
246246
CryptoStream.prototype._suck = function() {
247-
var rv;
248247
var havePending = this._pending.length > 0;
249248

250249
while (this._pending.length > 0) {
@@ -253,18 +252,18 @@ CryptoStream.prototype._suck = function() {
253252

254253
assert(this._pending.length === this._pendingCallbacks.length);
255254

256-
try {
257-
rv = this._sucker(tmp);
258-
} catch (e) {
255+
var rv = this._sucker(tmp);
256+
257+
if (this.pair._ssl && this.pair._ssl.error) {
259258
if (this.pair._secureEstablished) {
260-
this.pair._error(e);
259+
this.pair._error();
261260
} else {
262261
this.pair._destroy();
263262
}
264263
return;
265264
}
266265

267-
if (rv === 0) {
266+
if (rv === 0 || rv < 0) {
268267
this._pending.unshift(tmp);
269268
this._pendingCallbacks.unshift(cb);
270269
break;
@@ -453,9 +452,10 @@ SecurePair.prototype._cycle = function() {
453452
};
454453

455454

456-
SecurePair.prototype._destroy = function(err) {
455+
SecurePair.prototype._destroy = function() {
457456
if (!this._done) {
458457
this._done = true;
458+
this._ssl.error = null;
459459
this._ssl.close();
460460
this._ssl = null;
461461

@@ -469,7 +469,10 @@ SecurePair.prototype._destroy = function(err) {
469469
};
470470

471471

472-
SecurePair.prototype._error = function(err) {
472+
SecurePair.prototype._error = function() {
473+
var err = this._ssl.error;
474+
this._ssl.error = null;
475+
473476
if (this._isServer &&
474477
this._rejectUnauthorized &&
475478
/peer did not return a certificate/.test(err.message)) {

‎src/node_crypto.cc‎

Lines changed: 90 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -287,28 +287,55 @@ Handle<Value> SecureContext::Close(const Arguments& args) {
287287
return False();
288288
}
289289

290-
char ssl_error_buf[512];
290+
#ifdef SSL_PRINT_DEBUG
291+
# define DEBUG_PRINT(...) fprintf (stderr, __VA_ARGS__)
292+
#else
293+
# define DEBUG_PRINT(...)
294+
#endif
291295

292-
static int serr(SSL *ssl, const char* func, int rv) {
293-
if (rv >= 0) {
294-
return rv;
295-
}
296296

297-
int err = SSL_get_error(ssl, rv);
298-
if (err != SSL_ERROR_WANT_WRITE &&
299-
err != SSL_ERROR_WANT_READ) {
300-
ERR_error_string_n(ERR_get_error(), &ssl_error_buf[0], sizeof(ssl_error_buf));
301-
/* fprintf(stderr, "[%p] SSL: %s failed: (%d:%d) %s\n", ssl, func, err, rv, buf); */
302-
return rv;
303-
} else if (err == SSL_ERROR_WANT_WRITE) {
304-
/* fprintf(stderr, "[%p] SSL: %s want write\n", ssl, func); */
297+
int Connection::HandleError(const char* func, int rv, bool ignore_error) {
298+
if (rv >= 0) return rv;
299+
300+
int err = SSL_get_error(ssl_, rv);
301+
302+
if (err == SSL_ERROR_WANT_WRITE) {
303+
DEBUG_PRINT("[%p] SSL: %s want write\n", ssl_, func);
304+
return 0;
305+
305306
} else if (err == SSL_ERROR_WANT_READ) {
306-
/* fprintf(stderr, "[%p] SSL: %s want read\n", ssl, func); */
307+
DEBUG_PRINT("[%p] SSL: %s want read\n", ssl_, func);
308+
return 0;
309+
310+
} else {
311+
static char ssl_error_buf[512];
312+
ERR_error_string_n(err, ssl_error_buf, sizeof(ssl_error_buf));
313+
314+
if (!ignore_error) {
315+
HandleScope scope;
316+
Local<Value> e = Exception::Error(String::New(ssl_error_buf));
317+
handle_->Set(String::New("error"), e);
318+
}
319+
320+
DEBUG_PRINT("[%p] SSL: %s failed: (%d:%d) %s\n", ssl_, func, err, rv, ssl_error_buf);
321+
322+
return rv;
307323
}
308324

309325
return 0;
310326
}
311327

328+
329+
void Connection::ClearError() {
330+
#ifndef NDEBUG
331+
HandleScope scope;
332+
333+
// We should clear the error in JS-land
334+
assert(handle_->Get(String::New("error"))->BooleanValue() == false);
335+
#endif // NDEBUG
336+
}
337+
338+
312339
void Connection::Initialize(Handle<Object> target) {
313340
HandleScope scope;
314341

@@ -440,7 +467,7 @@ Handle<Value> Connection::New(const Arguments& args) {
440467
Handle<Value> Connection::EncIn(const Arguments& args) {
441468
HandleScope scope;
442469

443-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
470+
Connection *ss = Connection::Unwrap(args);
444471

445472
if (args.Length() < 3) {
446473
return ThrowException(Exception::TypeError(
@@ -468,12 +495,9 @@ Handle<Value> Connection::EncIn(const Arguments& args) {
468495
String::New("Length is extends beyond buffer")));
469496
}
470497

471-
int bytes_written = serr(ss->ssl_, "BIO_write", BIO_write(ss->bio_read_, (char*)buffer_data + off, len));
498+
int bytes_written = BIO_write(ss->bio_read_, (char*)buffer_data + off, len);
472499

473-
if (bytes_written < 0) {
474-
if (errno == EAGAIN || errno == EINTR) return Null();
475-
return ThrowException(ErrnoException(errno, "read"));
476-
}
500+
ss->HandleError("BIO_write", bytes_written);
477501

478502
return scope.Close(Integer::New(bytes_written));
479503
}
@@ -482,7 +506,7 @@ Handle<Value> Connection::EncIn(const Arguments& args) {
482506
Handle<Value> Connection::ClearOut(const Arguments& args) {
483507
HandleScope scope;
484508

485-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
509+
Connection *ss = Connection::Unwrap(args);
486510

487511
if (args.Length() < 3) {
488512
return ThrowException(Exception::TypeError(
@@ -510,33 +534,33 @@ Handle<Value> Connection::ClearOut(const Arguments& args) {
510534
String::New("Length is extends beyond buffer")));
511535
}
512536

513-
int bytes_read;
514537

515538
if (!SSL_is_init_finished(ss->ssl_)) {
539+
int rv;
540+
516541
if (ss->is_server_) {
517-
bytes_read = serr(ss->ssl_, "SSL_accept:ClearOut", SSL_accept(ss->ssl_));
542+
rv = SSL_accept(ss->ssl_);
543+
ss->HandleError("SSL_accept:ClearOut", rv);
518544
} else {
519-
bytes_read = serr(ss->ssl_, "SSL_connect:ClearOut", SSL_connect(ss->ssl_));
520-
}
521-
if (bytes_read < 0) {
522-
return ThrowException(Exception::Error(v8::String::New(ssl_error_buf)));
545+
rv = SSL_connect(ss->ssl_);
546+
ss->HandleError("SSL_connect:ClearOut", rv);
523547
}
524-
return scope.Close(Integer::New(0));
525-
}
526548

527-
bytes_read = serr(ss->ssl_, "SSL_read:ClearOut", SSL_read(ss->ssl_, (char*)buffer_data + off, len));
528-
if (bytes_read < 0) {
529-
return ThrowException(Exception::Error(v8::String::New(ssl_error_buf)));
549+
if (rv < 0) return scope.Close(Integer::New(rv));
530550
}
531551

552+
int bytes_read = SSL_read(ss->ssl_, (char*)buffer_data + off, len);
553+
ss->HandleError("SSL_read:ClearOut", bytes_read);
554+
532555
return scope.Close(Integer::New(bytes_read));
533556
}
534557

535558

536559
Handle<Value> Connection::ClearPending(const Arguments& args) {
537560
HandleScope scope;
538561

539-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
562+
Connection *ss = Connection::Unwrap(args);
563+
540564
int bytes_pending = BIO_pending(ss->bio_read_);
541565
return scope.Close(Integer::New(bytes_pending));
542566
}
@@ -545,7 +569,8 @@ Handle<Value> Connection::ClearPending(const Arguments& args) {
545569
Handle<Value> Connection::EncPending(const Arguments& args) {
546570
HandleScope scope;
547571

548-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
572+
Connection *ss = Connection::Unwrap(args);
573+
549574
int bytes_pending = BIO_pending(ss->bio_write_);
550575
return scope.Close(Integer::New(bytes_pending));
551576
}
@@ -554,7 +579,7 @@ Handle<Value> Connection::EncPending(const Arguments& args) {
554579
Handle<Value> Connection::EncOut(const Arguments& args) {
555580
HandleScope scope;
556581

557-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
582+
Connection *ss = Connection::Unwrap(args);
558583

559584
if (args.Length() < 3) {
560585
return ThrowException(Exception::TypeError(
@@ -582,7 +607,9 @@ Handle<Value> Connection::EncOut(const Arguments& args) {
582607
String::New("Length is extends beyond buffer")));
583608
}
584609

585-
int bytes_read = serr(ss->ssl_, "BIO_read:EncOut", BIO_read(ss->bio_write_, (char*)buffer_data + off, len));
610+
int bytes_read = BIO_read(ss->bio_write_, (char*)buffer_data + off, len);
611+
612+
ss->HandleError("BIO_read:EncOut", bytes_read, true);
586613

587614
return scope.Close(Integer::New(bytes_read));
588615
}
@@ -591,7 +618,7 @@ Handle<Value> Connection::EncOut(const Arguments& args) {
591618
Handle<Value> Connection::ClearIn(const Arguments& args) {
592619
HandleScope scope;
593620

594-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
621+
Connection *ss = Connection::Unwrap(args);
595622

596623
if (args.Length() < 3) {
597624
return ThrowException(Exception::TypeError(
@@ -620,25 +647,21 @@ Handle<Value> Connection::ClearIn(const Arguments& args) {
620647
}
621648

622649
if (!SSL_is_init_finished(ss->ssl_)) {
623-
int s;
650+
int rv;
624651
if (ss->is_server_) {
625-
s = serr(ss->ssl_, "SSL_accept:ClearIn", SSL_accept(ss->ssl_));
652+
rv = SSL_accept(ss->ssl_);
653+
ss->HandleError("SSL_accept:ClearIn", rv);
626654
} else {
627-
s = serr(ss->ssl_, "SSL_connect:ClearIn", SSL_connect(ss->ssl_));
628-
}
629-
630-
if (s < 0) {
631-
return ThrowException(Exception::Error(v8::String::New(ssl_error_buf)));
655+
rv = SSL_connect(ss->ssl_);
656+
ss->HandleError("SSL_connect:ClearIn", rv);
632657
}
633658

634-
return scope.Close(Integer::New(0));
659+
if (rv < 0) return scope.Close(Integer::New(rv));
635660
}
636661

637-
int bytes_written = serr(ss->ssl_, "SSL_write:ClearIn", SSL_write(ss->ssl_, (char*)buffer_data + off, len));
662+
int bytes_written = SSL_write(ss->ssl_, (char*)buffer_data + off, len);
638663

639-
if (bytes_written < 0) {
640-
return ThrowException(Exception::Error(v8::String::New(ssl_error_buf)));
641-
}
664+
ss->HandleError("SSL_write:ClearIn", bytes_written);
642665

643666
return scope.Close(Integer::New(bytes_written));
644667
}
@@ -647,7 +670,7 @@ Handle<Value> Connection::ClearIn(const Arguments& args) {
647670
Handle<Value> Connection::GetPeerCertificate(const Arguments& args) {
648671
HandleScope scope;
649672

650-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
673+
Connection *ss = Connection::Unwrap(args);
651674

652675
if (ss->ssl_ == NULL) return Undefined();
653676
Local<Object> info = Object::New();
@@ -719,36 +742,30 @@ Handle<Value> Connection::GetPeerCertificate(const Arguments& args) {
719742

720743
Handle<Value> Connection::Start(const Arguments& args) {
721744
HandleScope scope;
722-
int rv;
723745

724-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
746+
Connection *ss = Connection::Unwrap(args);
725747

726748
if (!SSL_is_init_finished(ss->ssl_)) {
749+
int rv;
727750
if (ss->is_server_) {
728-
rv = serr(ss->ssl_, "SSL_accept:Start", SSL_accept(ss->ssl_));
751+
rv = SSL_accept(ss->ssl_);
752+
ss->HandleError("SSL_accept:Start", rv);
729753
} else {
730-
rv = serr(ss->ssl_, "SSL_connect:Start", SSL_connect(ss->ssl_));
731-
}
732-
733-
if (rv < 0) {
734-
return ThrowException(Exception::Error(v8::String::New(ssl_error_buf)));
754+
rv = SSL_connect(ss->ssl_);
755+
ss->HandleError("SSL_connect:Start", rv);
735756
}
736757

737-
if (rv == 1) {
738-
return True();
739-
} else {
740-
return False();
741-
}
758+
return scope.Close(Integer::New(rv));
742759
}
743760

744-
return True();
761+
return scope.Close(Integer::New(0));
745762
}
746763

747764

748765
Handle<Value> Connection::Shutdown(const Arguments& args) {
749766
HandleScope scope;
750767

751-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
768+
Connection *ss = Connection::Unwrap(args);
752769

753770
if (ss->ssl_ == NULL) return False();
754771
int r = SSL_shutdown(ss->ssl_);
@@ -760,7 +777,7 @@ Handle<Value> Connection::Shutdown(const Arguments& args) {
760777
Handle<Value> Connection::ReceivedShutdown(const Arguments& args) {
761778
HandleScope scope;
762779

763-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
780+
Connection *ss = Connection::Unwrap(args);
764781

765782
if (ss->ssl_ == NULL) return False();
766783
int r = SSL_get_shutdown(ss->ssl_);
@@ -773,7 +790,9 @@ Handle<Value> Connection::ReceivedShutdown(const Arguments& args) {
773790

774791
Handle<Value> Connection::IsInitFinished(const Arguments& args) {
775792
HandleScope scope;
776-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
793+
794+
Connection *ss = Connection::Unwrap(args);
795+
777796
if (ss->ssl_ == NULL) return False();
778797
return SSL_is_init_finished(ss->ssl_) ? True() : False();
779798
}
@@ -782,7 +801,7 @@ Handle<Value> Connection::IsInitFinished(const Arguments& args) {
782801
Handle<Value> Connection::VerifyError(const Arguments& args) {
783802
HandleScope scope;
784803

785-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
804+
Connection *ss = Connection::Unwrap(args);
786805

787806
if (ss->ssl_ == NULL) return Null();
788807

@@ -926,7 +945,8 @@ Handle<Value> Connection::VerifyError(const Arguments& args) {
926945
Handle<Value> Connection::GetCurrentCipher(const Arguments& args) {
927946
HandleScope scope;
928947

929-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
948+
Connection *ss = Connection::Unwrap(args);
949+
930950
OPENSSL_CONST SSL_CIPHER *c;
931951

932952
if ( ss->ssl_ == NULL ) return Undefined();
@@ -943,7 +963,7 @@ Handle<Value> Connection::GetCurrentCipher(const Arguments& args) {
943963
Handle<Value> Connection::Close(const Arguments& args) {
944964
HandleScope scope;
945965

946-
Connection *ss = ObjectWrap::Unwrap<Connection>(args.Holder());
966+
Connection *ss = Connection::Unwrap(args);
947967

948968
if (ss->ssl_ != NULL) {
949969
SSL_free(ss->ssl_);

0 commit comments

Comments
 (0)