Details

    • Type: Improvement Improvement
    • Status: Closed
    • Severity: Minor Minor
    • Resolution: Done
    • Affects Version/s: SVN
    • Target Release Version/s: None
    • Component/s: Channels/chan_sip/SRTP
    • Security Level: None
    • Labels:
      None
    • Environment:
      Linux x86_64
    • SVN Revision Number:
      402525
    • Regression:
      No

      Description

      There is a version of libsrtp that supports AES-NI and AES-GCM mode:

      https://github.com/cisco/libsrtp/pull/34

      More on AES-GCM mode:

      http://tools.ietf.org/html/draft-ietf-avtcore-srtp-aes-gcm-10

      https://crypto.stanford.edu/RealWorldCrypto/slides/gueron.pdf

      AES-GCM mode improves the performance of SRTP on systems with and without support for the AES-NI instruction set.

      Performance test results pending.

      1. asterisk_gcm.patch
        3 kB
        Kristian Kielhofner
      2. asterisk_gcm_draft10.patch
        3 kB
        Kristian Kielhofner
      3. asterisk-1.8-srtp-crypto_kernel-include.patch
        0.3 kB
        abelbeck

        Issue Links

          Activity

          Hide
          Kristian Kielhofner added a comment - - edited

          This patch supports AEAD_AES_GCM_128_8 with Asterisk trunk. Tested with pjsip 2.1 with AES-GCM patches applied.

          pjsip source here:

          https://github.com/krisk84/PJSIP

          Show
          Kristian Kielhofner added a comment - - edited This patch supports AEAD_AES_GCM_128_8 with Asterisk trunk. Tested with pjsip 2.1 with AES-GCM patches applied. pjsip source here: https://github.com/krisk84/PJSIP
          Hide
          Matt Jordan added a comment -

          Well, that's a fairly straight forward patch. I might recommend re-ordering the ast_srtp_suite enumeration to line up with the #defined constants, but that's just a nit-pick. Thanks for the contribution!

          Show
          Matt Jordan added a comment - Well, that's a fairly straight forward patch. I might recommend re-ordering the ast_srtp_suite enumeration to line up with the #defined constants, but that's just a nit-pick. Thanks for the contribution!
          Hide
          Rusty Newton added a comment -

          Kristian, you'll want to go ahead and get your patch on reviewboard to get more eyeballs on it.

          Here is the Code Review process on the wiki which describe the general process and how to use reviewboard.

          Show
          Rusty Newton added a comment - Kristian, you'll want to go ahead and get your patch on reviewboard to get more eyeballs on it. Here is the Code Review process on the wiki which describe the general process and how to use reviewboard.
          Hide
          Kristian Kielhofner added a comment -

          Update crypto suite names to latest draft (10).

          Show
          Kristian Kielhofner added a comment - Update crypto suite names to latest draft (10).
          Hide
          Kristian Kielhofner added a comment -

          I also created a ReviewBoard entry with the latest patch:

          https://reviewboard.asterisk.org/r/3350/

          Show
          Kristian Kielhofner added a comment - I also created a ReviewBoard entry with the latest patch: https://reviewboard.asterisk.org/r/3350/
          Hide
          abelbeck added a comment -

          Adds srtp/crypto_kernel.h include to res/res_srtp.c for libsrtp 1.5.0

          Show
          abelbeck added a comment - Adds srtp/crypto_kernel.h include to res/res_srtp.c for libsrtp 1.5.0
          Hide
          abelbeck added a comment -

          Attached is trivial patch to res/res_srtp.c to also include srtp/crypto_kernel.h

          This is related to Kristian's issue here as the libsrtp version 1.5.0 ( https://github.com/cisco/libsrtp ) has rearranged some of the header files, and crypto_kernel.h is not longer included in srtp.h. Without the patch you see:

            [CC] res_srtp.c -> res_srtp.o
          res_srtp.c: In function 'ast_srtp_get_random':
          res_srtp.c:307: warning: implicit declaration of function 'crypto_get_random'
            [LD] res_srtp.o -> res_srtp.so
          

          The included patch should be backward compatible, and should apply to all versions of Asterisk.

          Using libsrtp version 1.5.0 offers OpenSSL support and it's AES optimizations.

          Show
          abelbeck added a comment - Attached is trivial patch to res/res_srtp.c to also include srtp/crypto_kernel.h This is related to Kristian's issue here as the libsrtp version 1.5.0 ( https://github.com/cisco/libsrtp ) has rearranged some of the header files, and crypto_kernel.h is not longer included in srtp.h. Without the patch you see: [CC] res_srtp.c -> res_srtp.o res_srtp.c: In function 'ast_srtp_get_random': res_srtp.c:307: warning: implicit declaration of function 'crypto_get_random' [LD] res_srtp.o -> res_srtp.so The included patch should be backward compatible, and should apply to all versions of Asterisk. Using libsrtp version 1.5.0 offers OpenSSL support and it's AES optimizations.
          Hide
          Alexander Traud added a comment -

          abelbeck, thank you for reporting that issue with crypto_get_random. It got its own issue report. Therefore, please, continue with ASTERISK-24436.

          A. Sresnewsky, the attached patch targeted the master branch at that time, which created Asterisk 13. If you are looking for a backport of AES-GCM for Asterisk 11, please, do give a rationale why you cannot update to Asterisk 13 and why you want AES-GCM. That raises motivation and might help to find an Asterisk team member or a community member, to create such a patch.

          @all
          In December 2015, that draft matured to RFC 7714. However in June 2014 with draft revision 13, the crypto suite AEAD_AES_128_GCM_8 got dropped. Furthermore even back than, there was a bug in libSRTP because the key length (actually the master salt) was too long. This patch here relied on the fact that the key length was the same as for AES_CM_128_HMAC_SHA1_80. This is not the case anymore. Therefore, please, let us continue with ASTERISK-26190.

          Show
          Alexander Traud added a comment - abelbeck , thank you for reporting that issue with crypto_get_random . It got its own issue report. Therefore, please, continue with ASTERISK-24436 . A. Sresnewsky , the attached patch targeted the master branch at that time, which created Asterisk 13. If you are looking for a backport of AES-GCM for Asterisk 11, please, do give a rationale why you cannot update to Asterisk 13 and why you want AES-GCM. That raises motivation and might help to find an Asterisk team member or a community member, to create such a patch. @all In December 2015, that draft matured to RFC 7714. However in June 2014 with draft revision 13, the crypto suite AEAD_AES_128_GCM_8 got dropped. Furthermore even back than, there was a bug in libSRTP because the key length (actually the master salt) was too long. This patch here relied on the fact that the key length was the same as for AES_CM_128_HMAC_SHA1_80 . This is not the case anymore. Therefore, please, let us continue with ASTERISK-26190 .
          Hide
          Alexander Traud added a comment -

          Kristian, I am closing this report because sRTP supports AES-GCM since Asterisk 14. Your patch was a helpful starting point.

          If you want to see this variant of AES-GCM (discussed here in this report) to be implemented as well, please, re-open this issue report. In that case, please, specify which products still require/rely on this AES-GCM draft.

          Show
          Alexander Traud added a comment - Kristian, I am closing this report because sRTP supports AES-GCM since Asterisk 14. Your patch was a helpful starting point. If you want to see this variant of AES-GCM (discussed here in this report) to be implemented as well, please, re-open this issue report. In that case, please, specify which products still require/rely on this AES-GCM draft.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development