On 29/11/2018 13:27, Rémy Maucherat wrote: > On Sun, Nov 25, 2018 at 10:42 AM Rainer Jung <rainer.j...@kippdata.de> > wrote: > >> In our Java code, what happens is a call to unwrap() in OpenSSLEngine. >> This call writes I think 146 bytes, then checks >> pendingReadableBytesInSSL(). That call in turn calls SSL.readFromSSL() >> and gets back "0" (from SSL_read()). Up in unwrap() we then skip the >> while loop and finally return with BUFFER_UNDERFLOW. Then we hang, >> probably because the data was read by OpenSSL and no more socket event >> happens. If I artificially add another call to >> pendingReadableBytesInSSL() which triggers another SSL_read(), the hang >> does not occur. >> >> IMHO TLS 1.0 is not such a big problem, but we should at least document >> it when we do a new release. >> > Last time, Mark fixed pendingReadableBytesInSSL (= > SSL.pendingReadableBytesInSSL) not working by using a fake read > (SSL.readFromSSL(ssl, EMPTY_ADDR, 0)) to start a new record. So then we > actually need to make *two* fake reads (if the result of the first is zero) > and we would be fine ? This OpenSSL API sounds ridiculously bad ... (IMO)
My research is leading me in that direction. So far, it looks like TLS 1.0 is sending two application data packets (I assume this is mitigation for BEAST) whereas TLS 1.1 and later only send 1. It appears those two packets each need a priming read. Not sure why. I'd expect to be able to read a single character after the first packet but the API indicates 0 characters are available. I have been trying to find some API to call that reliably indicates that another priming read is required but I haven't found it yet. I am currently looking at triggering a second priming read if: - TLS 1.0 is being used - a priming read returns 0 - SSL_get_error returns 5 - pendingReadableBytesInSSL returns 0 I also want to think about trying to reduce the number of times we call into native code. I'm not sure how practical this is and - since it only affects TLS 1.0 - I'm not overly concerned since folks should really be on TLS 1.2 / 1.3 by now. > So you made it sound like it would work: > Index: java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java > =================================================================== > --- java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java (revision > 1847712) > +++ java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java (working > copy) > @@ -635,6 +635,9 @@ > // See https://www.openssl.org/docs/manmaster/ssl/SSL_pending.html > clearLastError(); > int lastPrimingReadResult = SSL.readFromSSL(ssl, EMPTY_ADDR, 0); > // priming read > + if (lastPrimingReadResult == 0) { > + lastPrimingReadResult = SSL.readFromSSL(ssl, EMPTY_ADDR, 0); > + } > // check if SSL_read returned <= 0. In this case we need to check > the error and see if it was something > // fatal. > if (lastPrimingReadResult <= 0) { Yep. That is a much cleaner version of what I have currently working. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
