Fix SSL_peek and SSL_pending.

CHANGES

@@ -33,11 +33,19 @@
      result of the server certificate verification.)
      [Lutz Jaenicke]
 
-  *) Disable ssl2_peek and ssl3_peek (i.e., both implementations
+  *) Fix ssl3_pending: If the record in s->s3->rrec is not of type
-     of SSL_peek) because they both are completely broken.
+     SSL3_RT_APPLICATION_DATA, return 0.
-     For fixing this, the internal read functions now have an additional
+     [Bodo Moeller]
-     'peek' parameter, but the actual peek functionality has not
+
-     yet been implemented.
+  *) 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]
 
   *) Increase BN_CTX_NUM (the number of BIGNUMs in a BN_CTX) to 16.

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<ssl>.  The value returned by that
+SSL_read(), SSL_peek(), or SSL_write() on B<ssl>.  The value returned by
-TLS/SSL I/O function must be passed to SSL_get_error() in parameter
+that TLS/SSL I/O function must be passed to SSL_get_error() in parameter
 B<ret>.
 
 In addition to B<ssl> and B<ret>, SSL_get_error() inspects the
@@ -48,16 +48,26 @@ has been closed.
 =item SSL_ERROR_WANT_READ, SSL_ERROR_WANT_WRITE
 
 The operation did not complete; the same TLS/SSL I/O function should be
-called again later.  There will be protocol progress if, by then, the
+called again later.  If, by then, the underlying B<BIO> has data
-underlying B<BIO> has data available for reading (if the result code is
+available for reading (if the result code is B<SSL_ERROR_WANT_READ>)
-B<SSL_ERROR_WANT_READ>) or allows writing data (B<SSL_ERROR_WANT_WRITE>). 
+or allows writing data (B<SSL_ERROR_WANT_WRITE>), then some TLS/SSL
-For socket B<BIO>s (e.g. when SSL_set_fd() was used) this means that
+protocol progress will take place, i.e. at least part of an TLS/SSL
-select() or poll() on the underlying socket can be used to find out
+record will be read or written.  Note that the retry may again lead to
-when the TLS/SSL I/O function should be retried.
+a B<SSL_ERROR_WANT_READ> or B<SSL_ERROR_WANT_WRITE> condition.
+There is no fixed upper limit for the number of iterations that
+may be necessary until progress becomes visible at application
+protocol level.
+
+For socket B<BIO>s (e.g. when SSL_set_fd() was used), select() or
+poll() on the underlying socket can be used to find out when the
+TLS/SSL I/O function should be retried.
 
 Caveat: Any TLS/SSL I/O function can lead to either of
-B<SSL_ERROR_WANT_READ> and B<SSL_ERROR_WANT_WRITE>, i.e. SSL_read()
+B<SSL_ERROR_WANT_READ> and B<SSL_ERROR_WANT_WRITE>.  In particular,
-may want to write data and SSL_write() may want to read data.
+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
 

doc/ssl/SSL_pending.pod

@@ -33,8 +33,8 @@ I<read_ahead> 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
+Up to OpenSSL 0.9.6, SSL_pending() does not check if the record type
-application data.
+of pending data is application data.
 
 =head1 SEE ALSO
 

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);
+		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,17 +275,17 @@ ssl2_read_again:
 		INC32(s->s2->read_sequence); /* expect next number */
 		/* s->s2->ract_data is now available for processing */
 
-#if 1
+		/* Possibly the packet that we just read had 0 actual data bytes.
-		/* How should we react when a packet containing 0
+		 * (SSLeay/OpenSSL itself never sends such packets; see ssl2_write.)
-		 * bytes is received?  (Note that SSLeay/OpenSSL itself
+		 * In this case, returning 0 would be interpreted by the caller
-		 * never sends such packets; see ssl2_write.)
+		 * as indicating EOF, so it's not a good idea.  Instead, we just
-		 * Returning 0 would be interpreted by the caller as
+		 * continue reading; thus ssl2_read_internal may have to process
-		 * indicating EOF, so it's not a good idea.
+		 * multiple packets before it can return.
-		 * Instead, we just continue reading.  Note that using
+		 *
-		 * select() for blocking sockets *never* guarantees
+		 * [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
+		 * data may contain incomplete packets, and except for SSL 2,
-		 * renegotiation can confuse things even more. */
+		 * renegotiation can confuse things even more.] */
 
 		goto ssl2_read_again; /* This should really be
 		                       * "return ssl2_read(s,buf,len)",
@@ -284,15 +293,6 @@ ssl2_read_again:
 		                       * 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
 		}
 	else
 		{

ssl/s3_lib.c

@@ -691,10 +691,9 @@ SSL_CIPHER *ssl3_get_cipher(unsigned int u)
 		return(NULL);
 	}
 
-/* The problem is that it may not be the correct record type */
 int ssl3_pending(SSL *s)
 	{
-	return(s->s3->rrec.length);
+	return (s->s3->rrec.type == SSL3_RT_APPLICATION_DATA) ? s->s3->rrec.length : 0;
 	}
 
 int ssl3_new(SSL *s)

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,6 +813,8 @@ start:
 			n = (unsigned int)len;
 
 		memcpy(buf,&(rr->data[rr->off]),n);
+		if (!peek)
+			{
 			rr->length-=n;
 			rr->off+=n;
 			if (rr->length == 0)
@@ -823,6 +822,7 @@ start:
 				s->rstate=SSL_ST_READ_HEADER;
 				rr->off=0;
 				}
+			}
 		return(n);
 		}