Code review request, CR 7180038 regression test failure, SSLEngineBadBufferArrayAccess.java
Xuelei Fan
xuelei.fan at oracle.com
Mon Jul 9 02:30:05 UTC 2012
On 7/7/2012 10:26 AM, Brad Wetmore wrote:
>> It was the abbreviated handshaking. I guess that the previous client
>> has not closed the socket completely, so for *this* handshaking, the
>> abbreviated handshaking rather than the full handshaking is used.
>
> The issue is not due to closing the previous connections completely. The
> two close messages are racing and the messages appeared out of the
> "normal" ordering in this one case when the thread did a simple session
> resume.
>
I think there may be no two racing close messages. It is just a session
resuming that cause abbreviated handshaking.
>> I tied several different approaches within this test, but failed to
>> reproduce the abbreviated handshaking. It is not easy to hack the
>> test without significant changes.
>
> The original testcase was for the bad_record_mac issue, and testing
> whether or not ByteBuffers with valid hasArray() was working correctly.
> I wasn't intending to test renegotiation/abbreviated handshakes.
I see. But you don't mind if the handshaking is abbreviated handshakes,
right? I think renegotiation/abbreviated handshakes also expose the the
bad_record_mac issue.
> I
> obviously didn't completely step through the code when I added the
> second runTest(). :( That explains the race condition on close
> ordering, and the issue is still there.
>
> A better fix would be the following so that the test is actually
> starting from scratch each time (no session resumes):
>
The following fix only disables abbreviate handshakes. I don't think it
addresses the issue properly.
There is a assumption in the test that after the final wrap() in
runTest(), when server send out its application data, the server should
have already received application data from client. That's not true,
the next call after the final wrap() may need a unwrap() to parser
application data from client. Please see my evaluation in the bug or my
previous mails in the thread. Abbreviated handshaking just expose the
issue. Abbreviated handshaking is not the real issue.
Thanks for the comments.
Xuelei
> diff --git
> a/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
> b/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
>
> ---
> a/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
>
> +++
> b/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
>
> @@ -146,14 +146,13 @@
> "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2" };
>
> for (String protocol : protocols) {
> - log("Testing " + protocol);
> /*
> * Run the tests with direct and indirect buffers.
> */
> - SSLEngineBadBufferArrayAccess test =
> - new SSLEngineBadBufferArrayAccess(protocol);
> - test.runTest(true);
> - test.runTest(false);
> + log("Testing " + protocol + ":true");
> + new SSLEngineBadBufferArrayAccess(protocol).runTest(true);
> + log("Testing " + protocol + ":false");
> + new SSLEngineBadBufferArrayAccess(protocol).runTest(false);
> }
>
> System.out.println("Test Passed.");
>
> Please consider opening a new bug and changing.
>
> Brad
>
>
>
>
> On 7/2/2012 8:15 PM, Xuelei Fan wrote:
>> On 7/3/2012 11:09 AM, Weijun Wang wrote:
>>> Your fix looks fine.
>>>
>> Thanks!
>>
>>> IMHO, the remind is not really useful unless you dump more info, say,
>>> the value of serverIn.remaining().
>> We can get the value from analysis of the log. The remind is only used
>> for the case that we do not really fix the issue with this update.
>>
>> Xuelei
>>
>>> QE would report the failure to "THE
>>> SECURITY TEAM" anyway.
>>>
>>> -Max
>>>
>>> On 07/03/2012 11:00 AM, Xuelei Fan wrote:
>>>> On 7/3/2012 10:40 AM, Weijun Wang wrote:
>>>>> No new test needed. I only think that you might be able to hack the
>>>>> current test a little to reproduce this and see if the failure is the
>>>>> same and if your code change can fix it. There is no need to keep this
>>>>> hack in your final changeset.
>>>>>
>>>> I tied several different approaches within this test, but failed to
>>>> reproduce the abbreviated handshaking. ;-) It is not easy to hack the
>>>> test without significant changes.
>>>>
>>>> Xuelei
>>>>
>>>>> -Max
>>>>>
>>>>>
>>>>> On 07/03/2012 10:37 AM, Xuelei Fan wrote:
>>>>>> On 7/3/2012 10:02 AM, Weijun Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 07/03/2012 09:48 AM, Xuelei Fan wrote:
>>>>>>>> On 7/2/2012 4:35 PM, Weijun Wang wrote:
>>>>>>>>> I take a look at the test output. When the last handshake starts:
>>>>>>>>>
>>>>>>>>> ================
>>>>>>>>> server unwrap: OK/NEED_TASK, 230/0 bytes
>>>>>>>>> running delegated task...
>>>>>>>>> new HandshakeStatus: NEED_WRAP
>>>>>>>>> ----
>>>>>>>>> server wrap: OK/NEED_WRAP, 0/86 bytes
>>>>>>>>> ================
>>>>>>>>>
>>>>>>>>> Here the first wrap only generates 86 bytes, I guess that's the
>>>>>>>>> ServerHello message? It keeps the state at NEED_WRAP but then
>>>>>>>>> never
>>>>>>>>> really generates the Certificate message. What might be the
>>>>>>>>> problem?
>>>>>>>>>
>>>>>>>> Good catch!
>>>>>>>>
>>>>>>>> It was the abbreviated handshaking. I guess that the previous
>>>>>>>> client
>>>>>>>> has
>>>>>>>> not closed the socket completely, so for *this* handshaking, the
>>>>>>>> abbreviated handshaking rather than the full handshaking is used.
>>>>>>>>
>>>>>>>> For full handshaking, it is the client sending the "Finished"
>>>>>>>> message at
>>>>>>>> first. However, for abbreviated handshaking, the server send the
>>>>>>>> "Finished" message at first.
>>>>>>>>
>>>>>>>> In the current scenarios, it is expected that the client sends its
>>>>>>>> application data (26 bytes), and then the server sends its
>>>>>>>> application
>>>>>>>> data (29 bytes). However, the abbreviated handshaking disorder the
>>>>>>>> sequence in that it is the sever sends it application data (29
>>>>>>>> bytes)
>>>>>>>> before client. In such cases, the following logic does not stand
>>>>>>>> any
>>>>>>>> more:
>>>>>>>> if (!closed&& (serverOut.remaining() == 0)) {
>>>>>>>> closed = true;
>>>>>>>> ...
>>>>>>>> if (serverIn.remaining() != clientMsg.length) {
>>>>>>>>
>>>>>>>> throw new Exception("Client: Data length error");
>>>>>>>> }
>>>>>>>> ...
>>>>>>>> }
>>>>>>>>
>>>>>>>> Because the server has not receive the client message when the
>>>>>>>> server
>>>>>>>> sends its application data.
>>>>>>>>
>>>>>>>> I think it is a test issue, the current fix should has already
>>>>>>>> addressed
>>>>>>>> the issue.
>>>>>>>
>>>>>>> That's great.
>>>>>>>
>>>>>>> Since the reason is clear, can you reproduce this failure and then
>>>>>>> confirm the current fix does solve the problem?
>>>>>>>
>>>>>> It is possible to reproduce this failure with a new test case. But
>>>>>> it is
>>>>>> pretty hard to reproduce it within this test. I was wondering as
>>>>>> it is
>>>>>> test bug, so we may not want a extra test case to prove that this
>>>>>> test
>>>>>> is correct.
>>>>>>
>>>>>> We also have a nested remind that, "IF THIS FAILS, PLEASE REPORT
>>>>>> THIS TO
>>>>>> THE SECURITY TEAM. WE HAVE BEEN UNABLE TO RELIABLY DUPLICATE." I
>>>>>> think
>>>>>> it might be OK that we do not reproduce this issue at present.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>>
>>>>>>> Thanks
>>>>>>> Max
>>>>>>>
>>>>>>>>
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Max
>>>>>>>>>
>>>>>>>>> On 07/02/2012 10:39 AM, Xuelei Fan wrote:
>>>>>>>>>> Hi Weijun,
>>>>>>>>>>
>>>>>>>>>> Would you please review the test update for CR 7180038?
>>>>>>>>>> http://cr.openjdk.java.net./~xuelei/7180038/webrev.00/
>>>>>>>>>>
>>>>>>>>>> We cannot reproduce the issue. However, from the test log, there
>>>>>>>>>> is two
>>>>>>>>>> possible issues exposed by this CR.
>>>>>>>>>> 1. the improper test case senarios of un/wrap()
>>>>>>>>>> In the test case, the scenarios is
>>>>>>>>>> unwrap()->wrap()->serverOut.remaining()->"serverIn.remaining() !=
>>>>>>>>>> clientMsg.length". After the wrap(), the next operation may need
>>>>>>>>>> to be
>>>>>>>>>> unwrap() to get more incoming data before comparing serverIn
>>>>>>>>>> buffer
>>>>>>>>>> with
>>>>>>>>>> the expected client message.
>>>>>>>>>>
>>>>>>>>>> This fix is trying to do the comparing after the
>>>>>>>>>> engine has
>>>>>>>>>> closed.
>>>>>>>>>>
>>>>>>>>>> 2. From the log, the engine status and handshaking status move
>>>>>>>>>> from
>>>>>>>>>> CLOSED/NOT_HANDSHAKING to OK/FINISHED. FINISHED means the TLS
>>>>>>>>>> handshaking just finished. As the handshaking should have
>>>>>>>>>> completed for
>>>>>>>>>> a while, it does not sound like a correct status change.
>>>>>>>>>>
>>>>>>>>>> However, I did not find why this happens. Need more
>>>>>>>>>> info. So I
>>>>>>>>>> added
>>>>>>>>>> a line of log (suggested by Brad Wetmore) to collect the next
>>>>>>>>>> failure:
>>>>>>>>>>
>>>>>>>>>> IF THIS FAILS, PLEASE REPORT THIS TO THE SECURITY
>>>>>>>>>> TEAM. WE
>>>>>>>>>> HAVE
>>>>>>>>>> BEEN UNABLE TO RELIABLY DUPLICATE.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
More information about the security-dev
mailing list