<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/4419/">https://reviewboard.asterisk.org/r/4419/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 18th, 2015, 1:57 p.m. CST, <b>Mark Michelson</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/4419/diff/1/?file=71388#file71388line304" style="color: black; font-weight: bold; text-decoration: underline;">/branches/11/channels/sip/sdp_crypto.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">int sdp_crypto_process(struct sdp_crypto *p, const char *attr, struct ast_rtp_instance *rtp, struct sip_srtp *srtp)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">296</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">sdes_lifetime</span> <span class="o">=</span> <span class="p">(</span><span class="kt">unsigned</span> <span class="kt">long</span><span class="p">)</span><span class="n">pow</span><span class="p">(</span><span class="mi">2</span><span class="p">,</span> <span class="n">n_lifetime</span><span class="p">);</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;">Since we're only concerned about an integer value here, would the following work:
sdes_lifetime = 2UL << n_lifetime;
?
pow() probably has a built-in optimization for raising 2 to integer powers, but the fact that it returns a double means that there is still the issue of type conversion when using it.
Also I have a question here: since n_lifetime can be as large as 48, does this have the possibility of overflow on 32-bit architectures? The double returned by pow() can hold the value just fine, but when casting to unsigned long, what happens?</pre>
</blockquote>
<p>On February 18th, 2015, 2:30 p.m. CST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think that would be:
sdes_lifetime = 1UL << n_lifetime;
:)</pre>
</blockquote>
<p>On February 18th, 2015, 3:06 p.m. CST, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah yes, my mistake.</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;">Unfortunately, the default recommended max lifetime for SRTP is 2^48, which means we probably should just use a double for storage and comparisons.
Tha also means using pow, and not using a bit shift.</pre>
<br />
<p>- Matt</p>
<br />
<p>On February 13th, 2015, 9:26 p.m. CST, Matt Jordan 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 and Olle E Johansson.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Feb. 13, 2015, 9:26 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-17721">ASTERISK-17721</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-17899">ASTERISK-17899</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22748">ASTERISK-22748</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;">Note that this patch is a forward port of oej's lingon-srtp-key-lifetime-1.8 branch, with a few small modifications to reduce block indentation and a few improvements made to surrounding existing code.
To quote Olle from ASTERISK-17721:
{quote}
The Lingon branch now handle lifetime and MKI parameters.
We only accept lifetimes up to max for the crypto and higher than 10 hours for packetization of 20 ms (50 pps).
We only handle MKI with index 1.
We do not really bother with counting packets and reinviting at end of lifetime, so the min of 10 hours kind of takes care of most calls. If there are longer ones, we rely on the other side for re-invites.
It's still not perfect, but I personally think this is an improvement. A configuration option for minimum lifetime accepted could be added.
{quote}
I personally don't think the minimum lifetime option is needed, as in practice, every instance of an SDP offer for SDES-SRTP with lifetime I've seen offers a lifetime of 2^32 - but it could be added in the future if necessary.
Note that since the sdp_crypto code was moved to the core in 12+, a separate review (r4418) has been made for the Asterisk 13 variant. It is essentially identical to this review.</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;">Tests were added for chan_sip and updated for chan_pjsip - see https://reviewboard.asterisk.org/r/4420 . This includes both nominal and off-nominal offers negotiating SDES-SRTP.</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>/branches/11/channels/sip/sdp_crypto.c <span style="color: grey">(431750)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4419/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>