<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3653/">https://reviewboard.asterisk.org/r/3653/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 27th, 2014, 2:42 p.m. UTC, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3653/diff/2/?file=59976#file59976line2755" style="color: black; font-weight: bold; text-decoration: underline;">trunk/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int sip_check_authtimeout(time_t start)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2755</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>int res, content_length, after_poll = <span class="hl">1</span>, need_poll = <span class="hl">1</span>;</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2755</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>int res, content_length, after_poll = <span class="hl">0</span>, need_poll = <span class="hl">0</span>;</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So, I had to go back and do some research to see if this solution was correct - and I think it is a step in the right direction, but may still be missing some key pieces to properly handling large messages (or multiple messages) received over a TLS socket.
Examining _sip_tcp_helper_thread, a socket event may not always have tripped the file descriptor when sip_tls_read has been called. Another way in which the method can be called is when tcptls_session->overflow_buf has data within it. This buffer is used to store the remaining bytes when multiple SIP messages are read in a single read from a socket; the buffer holds the bytes that were read from the previous read operation. When that occurs, a previous ast_poll will not necessarily have been called, and it is up to the sip_*_read function to use the contents of the buffer as the beginning of the next message.
However, this clearly does not occur with sip_tls_read. The sip_tcp_read is the only function that handles this case; sip_tls_read does not append data to or otherwise use tcptls_session->overflow_buf.
The question then becomes: should sip_tls_read be cognizant of overflow? If so, it should be appending data into the overflow buffer, in which case this patch's assumption that ast_poll has already been called and a fd has been tripped is wrong.
If, however, the messages are guaranteed to be framed, then the current implementation of not bothering with the overflow buffer is correct, and you can assume that ast_poll already detected a tripped fd prior to sip_tls_read.
I think, unfortunately, that you cannot expect that the latter is the case. This is for two reasons:
(1) Looking at the documentation for SSL_Read (https://www.openssl.org/docs/ssl/SSL_read.html):
"SSL_read() works based on the SSL/TLS records. The data are received in records (with a maximum record size of 16kB for SSLv3/TLSv1). Only when a record has been completely received, it can be processed (decryption and check of integrity). Therefore data that was not retrieved at the last call of SSL_read() can still be buffered inside the SSL layer and will be retrieved on the next call to SSL_read(). If num is higher than the number of bytes buffered, SSL_read() will return with the bytes buffered. If no more bytes are in the buffer, SSL_read() will trigger the processing of the next record. Only when the record has been received and processed completely, SSL_read() will return reporting success. At most the contents of the record will be returned. As the size of an SSL/TLS record may exceed the maximum packet size of the underlying transport (e.g. TCP), it may be necessary to read several packets from the transport layer before the record is complete and SSL_read() can succeed."
Ideally, the tcptls code already handles that for us (although, in point #2, we'll see that it doesn't). However, the fact that a single read operation is not sufficient to request all data (which is part of the poll + try again code in sip_tls_read) also would make me think that - for sufficiently fast sending of data - you will get more than a single message when you complete the reading of the first message. This would imply that sip_tls_read *should* be making use of the overflow buffer in the same fashion as sip_tcp_read, and thus you cannot assume directly that ast_poll has already been called when this function is called. That changes the patch substantially.
(2) Worse, you cannot assume that SSL_Read has finished reading the buffer. When tcptls_stream_read is called (as a result of the fgets call), SSL_Read may return SSL_ERROR_WANT_READ. Due to the 'shared' nature of the the stream, chan_sip has to set stream->exclusive_input to 0, which means we'll return EAGAIN as opposed to calling SSL_Read a subsequent time. sip_tls_read currently will not handle this situation correctly; I think your change may help slightly in that it will force a second poll, but that is still no guarantee that all data will be read. This does mean however that we are not necessarily sure that we have read an entire message in sip_tls_read, and hence a 'retry once' poll may not be sufficient regardless to read all of the data from the socket.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> This would imply that sip_tls_read *should* be making use of the overflow buffer in
> the same fashion as sip_tcp_read, and thus you cannot assume directly that ast_poll
> has already been called when this function is called.
Pointing to: https://reviewboard.asterisk.org/r/2123/
> [...] The patch specifically targets TCP connections and not TLS. TLS connections
> were not reported as having the issue, plus changing TLS would be a much more
> invasive operation.
>
> In my opinion, we should remove the use of FILE handles altogether in the TCP/TLS
> code [...]
Yes. There still is fgets() in chan_sip.c, it should be killed.</pre>
<br />
<p>- wdoekes</p>
<br />
<p>On June 20th, 2014, 2:06 p.m. UTC, Alexander Traud wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Alexander Traud.</div>
<p style="color: grey;"><i>Updated June 20, 2014, 2:06 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-18345">ASTERISK-18345</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">With some large SDP, a *second* poll is required on the first part of a TLS message.
The current code did not poll a second time because the variable need_poll was inited with yes (1). That poll was a no-operation because there was a socket event already (which mandates fgets without poll). In the current code, poll returned immediately, fgets returned NULL, after_poll was yes (1), sip_tls_read returned failed (-1), _sip_tcp_helper_thread went to cleanup, called ast_tcptls_close_session_file, which closed the TLS connection.
The proposed patch, reads the gets the first message. If that failed, it does poll. This fixed all large SDP issues with SIP over TLS which I faced.
I am aware there were changes committed to tcptls.c just recently (revision 415907). Anyway, let us fix this bug as well.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Asterisk 12.3</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/channels/chan_sip.c <span style="color: grey">(416319)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3653/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>