From a0aae68cf6f3383f248c0e1991973224f2e4498f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bodo=20M=C3=B6ller?= Date: Mon, 25 Dec 2000 18:40:46 +0000 Subject: [PATCH] Fix SSL_peek and SSL_pending. --- CHANGES | 18 ++++++++---- doc/ssl/SSL_get_error.pod | 12 ++++---- doc/ssl/SSL_pending.pod | 4 +-- ssl/s2_lib.c | 3 -- ssl/s2_pkt.c | 58 +++++++++++++++++++-------------------- ssl/s3_lib.c | 3 +- ssl/s3_pkt.c | 28 +++++++++---------- 7 files changed, 65 insertions(+), 61 deletions(-) diff --git a/CHANGES b/CHANGES index 167048fa6a..ccb179ed83 100644 --- a/CHANGES +++ b/CHANGES @@ -144,11 +144,19 @@ 512 bits], about 30% for larger ones [1024 or 2048 bits].) [Bodo Moeller] - *) Disable ssl2_peek and ssl3_peek (i.e., both implementations - of SSL_peek) because they both are completely broken. - For fixing this, the internal read functions now have an additional - 'peek' parameter, but the actual peek functionality has not - yet been implemented. + *) Fix ssl3_pending: If the record in s->s3->rrec is not of type + SSL3_RT_APPLICATION_DATA, return 0. + [Bodo Moeller] + + *) Fix SSL_peek: + Both ssl2_peek and ssl3_peek, which were totally broken in earlier + releases, have been re-implemented by renaming the previous + implementations of ssl2_read and ssl3_read to ssl2_read_internal + and ssl3_read_internal, respectively, and adding 'peek' parameters + to them. The new ssl[23]_{read,peek} functions are calls to + ssl[23]_read_internal with the 'peek' flag set appropriately. + A 'peek' parameter has also been added to ssl3_read_bytes, which + does the actual work for ssl3_read_internal. [Bodo Moeller] *) New function BN_kronecker. diff --git a/doc/ssl/SSL_get_error.pod b/doc/ssl/SSL_get_error.pod index bd6872f61f..fefaf61936 100644 --- a/doc/ssl/SSL_get_error.pod +++ b/doc/ssl/SSL_get_error.pod @@ -14,8 +14,8 @@ SSL_get_error - obtain result code for TLS/SSL I/O operation SSL_get_error() returns a result code (suitable for the C "switch" statement) for a preceding call to SSL_connect(), SSL_accept(), -SSL_read(), or SSL_write() on B. The value returned by that -TLS/SSL I/O function must be passed to SSL_get_error() in parameter +SSL_read(), SSL_peek(), or SSL_write() on B. The value returned by +that TLS/SSL I/O function must be passed to SSL_get_error() in parameter B. In addition to B and B, SSL_get_error() inspects the @@ -64,10 +64,10 @@ TLS/SSL I/O function should be retried. Caveat: Any TLS/SSL I/O function can lead to either of B and B. In particular, -SSL_read() may want to write data and SSL_write() may want to read -data. This is mainly because TLS/SSL handshakes may occur at any time -during the protocol (initiated by either the client or the server); -SSL_read() and SSL_write() will handle any pending handshakes. +SSL_read() or SSL_peek() may want to write data and SSL_write() may want +to read data. This is mainly because TLS/SSL handshakes may occur at any +time during the protocol (initiated by either the client or the server); +SSL_read(), SSL_peek(), and SSL_write() will handle any pending handshakes. =item SSL_ERROR_WANT_X509_LOOKUP diff --git a/doc/ssl/SSL_pending.pod b/doc/ssl/SSL_pending.pod index 6c03b14caf..b4c48598b2 100644 --- a/doc/ssl/SSL_pending.pod +++ b/doc/ssl/SSL_pending.pod @@ -33,8 +33,8 @@ I flag is set, additional protocol bytes may have been read containing more TLS/SSL records; these are ignored by SSL_pending(). -SSL_pending() does not check if the record type of pending data is -application data. +Up to OpenSSL 0.9.6, SSL_pending() does not check if the record type +of pending data is application data. =head1 SEE ALSO diff --git a/ssl/s2_lib.c b/ssl/s2_lib.c index 52a2b27963..a89958607c 100644 --- a/ssl/s2_lib.c +++ b/ssl/s2_lib.c @@ -260,9 +260,6 @@ SSL_CIPHER *ssl2_get_cipher(unsigned int u) int ssl2_pending(SSL *s) { - /* Unlike ssl2_pending, this one probably works (if read-ahead - * is disabled), but it should be examined - * XXX */ return(s->s2->ract_data_length); } diff --git a/ssl/s2_pkt.c b/ssl/s2_pkt.c index 2866d61fa4..0ec9ee3393 100644 --- a/ssl/s2_pkt.c +++ b/ssl/s2_pkt.c @@ -138,7 +138,7 @@ static int ssl2_read_internal(SSL *s, void *buf, int len, int peek) return -1; } -ssl2_read_again: + ssl2_read_again: if (SSL_in_init(s) && !s->in_handshake) { n=s->handshake_func(s); @@ -162,13 +162,22 @@ ssl2_read_again: n=len; memcpy(buf,s->s2->ract_data,(unsigned int)n); - s->s2->ract_data_length-=n; - s->s2->ract_data+=n; - if (s->s2->ract_data_length == 0) - s->rstate=SSL_ST_READ_HEADER; + if (!peek) + { + s->s2->ract_data_length-=n; + s->s2->ract_data+=n; + if (s->s2->ract_data_length == 0) + s->rstate=SSL_ST_READ_HEADER; + } + return(n); } + /* s->s2->ract_data_length == 0 + * + * Fill the buffer, then goto ssl2_read_again. + */ + if (s->rstate == SSL_ST_READ_HEADER) { if (s->first_packet) @@ -266,33 +275,24 @@ ssl2_read_again: INC32(s->s2->read_sequence); /* expect next number */ /* s->s2->ract_data is now available for processing */ -#if 1 - /* How should we react when a packet containing 0 - * bytes is received? (Note that SSLeay/OpenSSL itself - * never sends such packets; see ssl2_write.) - * Returning 0 would be interpreted by the caller as - * indicating EOF, so it's not a good idea. - * Instead, we just continue reading. Note that using - * select() for blocking sockets *never* guarantees + /* Possibly the packet that we just read had 0 actual data bytes. + * (SSLeay/OpenSSL itself never sends such packets; see ssl2_write.) + * In this case, returning 0 would be interpreted by the caller + * as indicating EOF, so it's not a good idea. Instead, we just + * continue reading; thus ssl2_read_internal may have to process + * multiple packets before it can return. + * + * [Note that using select() for blocking sockets *never* guarantees * that the next SSL_read will not block -- the available - * data may contain incomplete packets, and except for SSL 2 - * renegotiation can confuse things even more. */ + * data may contain incomplete packets, and except for SSL 2, + * renegotiation can confuse things even more.] */ goto ssl2_read_again; /* This should really be - * "return ssl2_read(s,buf,len)", - * but that would allow for - * denial-of-service attacks if a - * C compiler is used that does not - * recognize end-recursion. */ -#else - /* If a 0 byte packet was sent, return 0, otherwise - * we play havoc with people using select with - * blocking sockets. Let them handle a packet at a time, - * they should really be using non-blocking sockets. */ - if (s->s2->ract_data_length == 0) - return(0); - return(ssl2_read(s,buf,len)); -#endif + * "return ssl2_read(s,buf,len)", + * but that would allow for + * denial-of-service attacks if a + * C compiler is used that does not + * recognize end-recursion. */ } else { diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index f2bfb15a5a..47768cc281 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -782,8 +782,7 @@ SSL_CIPHER *ssl3_get_cipher(unsigned int u) int ssl3_pending(SSL *s) { - /* The problem is that it may not be the correct record type */ - return(s->s3->rrec.length); /* FIXME */ + return (s->s3->rrec.type == SSL3_RT_APPLICATION_DATA) ? s->s3->rrec.length : 0; } int ssl3_new(SSL *s) diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index 92d9a4ab1b..9ab76604a6 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -711,17 +711,12 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) SSL3_RECORD *rr; void (*cb)()=NULL; - if (peek) - { - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_FIXME); /* proper implementation not yet completed */ - return -1; - } - if (s->s3->rbuf.buf == NULL) /* Not initialized yet */ if (!ssl3_setup_buffers(s)) return(-1); - if ((type != SSL3_RT_APPLICATION_DATA) && (type != SSL3_RT_HANDSHAKE) && type) + if ((type && (type != SSL3_RT_APPLICATION_DATA) && (type != SSL3_RT_HANDSHAKE) && type) || + (peek && (type != SSL3_RT_APPLICATION_DATA))) { SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_INTERNAL_ERROR); return -1; @@ -734,6 +729,7 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) unsigned char *dst = buf; unsigned int k; + /* peek == 0 */ n = 0; while ((len > 0) && (s->s3->handshake_fragment_len > 0)) { @@ -769,7 +765,7 @@ start: * s->s3->rrec.length, - number of bytes. */ rr = &(s->s3->rrec); - /* get new packet */ + /* get new packet if necessary */ if ((rr->length == 0) || (s->rstate == SSL_ST_READ_BODY)) { ret=ssl3_get_record(s); @@ -787,7 +783,8 @@ start: goto err; } - /* If the other end has shutdown, throw anything we read away */ + /* If the other end has shut down, throw anything we read away + * (even in 'peek' mode) */ if (s->shutdown & SSL_RECEIVED_SHUTDOWN) { rr->length=0; @@ -816,12 +813,15 @@ start: n = (unsigned int)len; memcpy(buf,&(rr->data[rr->off]),n); - rr->length-=n; - rr->off+=n; - if (rr->length == 0) + if (!peek) { - s->rstate=SSL_ST_READ_HEADER; - rr->off=0; + rr->length-=n; + rr->off+=n; + if (rr->length == 0) + { + s->rstate=SSL_ST_READ_HEADER; + rr->off=0; + } } return(n); }