Feature openssl #34

Merged
merged 22 commits into from Mar 2, 2014

Conversation

Projects
None yet
9 participants
@jfigus
Contributor

jfigus commented Oct 10, 2013

The feature-openssl branch is now feature complete. Support for AES-NI through libcrypto.so is now provided. Support for AES-GCM mode is provided. These features are enabled using the --enable-openssl configuration option. Omitting this option results in the legacy ciphers being compiled into libsrtp with no link dependency on libcrypto.so.

@fluffy

This comment has been minimized.

Show comment
Hide comment
@fluffy

fluffy May 9, 2013

Member

might be nice to add the --enable-openssl to readme

when I compiled on mac, I got
gcc -DHAVE_CONFIG_H -Icrypto/include -I./include -I./crypto/include -Wall -O4 -fexpensive-optimizations -funroll-loops -L. crypto/test/cipher_driver.c -o crypto/test/cipher_driver -lsrtp -lcrypto
Undefined symbols for architecture x86_64:
"_EVP_aes_128_ctr", referenced from:
_aes_icm_openssl_set_iv in libsrtp.a(aes_icm_ossl.o)
"_EVP_aes_192_ctr", referenced from:
_aes_icm_openssl_set_iv in libsrtp.a(aes_icm_ossl.o)
"_EVP_aes_256_ctr", referenced from:
_aes_icm_openssl_set_iv in libsrtp.a(aes_icm_ossl.o)
ld: symbol(s) not found for architecture x86_64

I did not dig into what went wrong and I know I should but ...

Member

fluffy commented on c7e66e3 May 9, 2013

might be nice to add the --enable-openssl to readme

when I compiled on mac, I got
gcc -DHAVE_CONFIG_H -Icrypto/include -I./include -I./crypto/include -Wall -O4 -fexpensive-optimizations -funroll-loops -L. crypto/test/cipher_driver.c -o crypto/test/cipher_driver -lsrtp -lcrypto
Undefined symbols for architecture x86_64:
"_EVP_aes_128_ctr", referenced from:
_aes_icm_openssl_set_iv in libsrtp.a(aes_icm_ossl.o)
"_EVP_aes_192_ctr", referenced from:
_aes_icm_openssl_set_iv in libsrtp.a(aes_icm_ossl.o)
"_EVP_aes_256_ctr", referenced from:
_aes_icm_openssl_set_iv in libsrtp.a(aes_icm_ossl.o)
ld: symbol(s) not found for architecture x86_64

I did not dig into what went wrong and I know I should but ...

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus May 9, 2013

Contributor
Contributor

jfigus replied May 9, 2013

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus May 9, 2013

Contributor
Contributor

jfigus replied May 9, 2013

This comment has been minimized.

Show comment
Hide comment
@tvsriram

tvsriram May 9, 2013

Contributor
Contributor

tvsriram replied May 9, 2013

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus May 9, 2013

Contributor
Contributor

jfigus replied May 9, 2013

This comment has been minimized.

Show comment
Hide comment
@tvsriram

tvsriram May 9, 2013

Contributor
Contributor

tvsriram replied May 9, 2013

This comment has been minimized.

Show comment
Hide comment
@fluffy

fluffy May 10, 2013

Member
Member

fluffy replied May 10, 2013

This comment has been minimized.

Show comment
Hide comment
@tvsriram

tvsriram May 10, 2013

Contributor
Contributor

tvsriram replied May 10, 2013

This comment has been minimized.

Show comment
Hide comment
@fluffy

fluffy May 10, 2013

Member
Member

fluffy replied May 10, 2013

This comment has been minimized.

Show comment
Hide comment
@tvsriram

tvsriram May 13, 2013

Contributor
Contributor

tvsriram replied May 13, 2013

@fluffy

This comment has been minimized.

Show comment
Hide comment
@fluffy

fluffy May 28, 2013

Member

Fix looks fine to me. I will get someone from Mozilla to review.

Member

fluffy commented on dfe68ea May 28, 2013

Fix looks fine to me. I will get someone from Mozilla to review.

This comment has been minimized.

Show comment
Hide comment
@jesup

jesup May 28, 2013

Contributor

That looks fine, but I'd also update the srtp_protect_rtcp() docs to indicate that it requires a buffer of SRTP_MAX_TRAILER_LEN+4 (Auth tag plus 4 bytes for 'E' + SRTCP index (see RFC 3711)). You might want to add an SRTCP_MAX_MAX_TRAILER_LEN define.

Contributor

jesup replied May 28, 2013

That looks fine, but I'd also update the srtp_protect_rtcp() docs to indicate that it requires a buffer of SRTP_MAX_TRAILER_LEN+4 (Auth tag plus 4 bytes for 'E' + SRTCP index (see RFC 3711)). You might want to add an SRTCP_MAX_MAX_TRAILER_LEN define.

This comment has been minimized.

Show comment
Hide comment
@davidmcgrew

davidmcgrew May 29, 2013

@krisk84

This comment has been minimized.

Show comment
Hide comment
@krisk84

krisk84 Oct 30, 2013

Contributor

Is OpenSSL (and AES-NI) support only offered with AES-GCM mode?

Contributor

krisk84 commented Oct 30, 2013

Is OpenSSL (and AES-NI) support only offered with AES-GCM mode?

@jfigus

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus Oct 30, 2013

Contributor

No, the legacy AES-CTR mode will utilize AES-NI as well (when the
hardware supports it).

On 10/30/2013 04:11 PM, Kristian Kielhofner wrote:

Is OpenSSL (and AES-NI) support only offered with AES-GCM mode?


Reply to this email directly or view it on GitHub
#34 (comment).

Contributor

jfigus commented Oct 30, 2013

No, the legacy AES-CTR mode will utilize AES-NI as well (when the
hardware supports it).

On 10/30/2013 04:11 PM, Kristian Kielhofner wrote:

Is OpenSSL (and AES-NI) support only offered with AES-GCM mode?


Reply to this email directly or view it on GitHub
#34 (comment).

@krisk84

This comment has been minimized.

Show comment
Hide comment
@krisk84

krisk84 Oct 30, 2013

Contributor

Excellent, thanks for the clarification!

Contributor

krisk84 commented Oct 30, 2013

Excellent, thanks for the clarification!

@ukreator

This comment has been minimized.

Show comment
Hide comment
@ukreator

ukreator Nov 7, 2013

Contributor

I checked our code (which uses openssl-based libsrtp) with address-sanitizer from clang, and it reported buffer overrun when calling v128_copy_octet_string() from aes_icm_openssl_context_init(). There are FIXME warnings, are there plans to fix this?

Contributor

ukreator commented Nov 7, 2013

I checked our code (which uses openssl-based libsrtp) with address-sanitizer from clang, and it reported buffer overrun when calling v128_copy_octet_string() from aes_icm_openssl_context_init(). There are FIXME warnings, are there plans to fix this?

@jfigus

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus Nov 7, 2013

Contributor

Which one of the calls to v128_copy_octet_string() is triggering the
warning?

The 'FIX' comments appear to be stale and should be removed. The
destination buffer for the two calls to v128_copy_octet_string() below
the 'FIX' comments are defined appropriately.

On 11/07/2013 07:46 AM, Dmitry Sobinov wrote:

I checked our code (which uses openssl-based libsrtp) with
address-sanitizer from clang, and it reported buffer overrun when
calling v128_copy_octet_string() from aes_icm_openssl_context_init().
There are FIXME warnings, are there plans to fix this?


Reply to this email directly or view it on GitHub
#34 (comment).

Contributor

jfigus commented Nov 7, 2013

Which one of the calls to v128_copy_octet_string() is triggering the
warning?

The 'FIX' comments appear to be stale and should be removed. The
destination buffer for the two calls to v128_copy_octet_string() below
the 'FIX' comments are defined appropriately.

On 11/07/2013 07:46 AM, Dmitry Sobinov wrote:

I checked our code (which uses openssl-based libsrtp) with
address-sanitizer from clang, and it reported buffer overrun when
calling v128_copy_octet_string() from aes_icm_openssl_context_init().
There are FIXME warnings, are there plans to fix this?


Reply to this email directly or view it on GitHub
#34 (comment).

@ukreator

This comment has been minimized.

Show comment
Hide comment
@ukreator

ukreator Nov 7, 2013

Contributor

The first call. To reproduce, I've just modified Makefile generated by './configure' to use clang with address sanitizer. Then, starting 'make runtest' (or smth like this). Output:

==2726==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000069a5be at pc 0x47e1cf bp 0x7fffa5ac6b20 sp 0x7fffa5ac6b18
READ of size 1 at 0x00000069a5be thread T0
#0 0x47e1ce in v128_copy_octet_string datatypes.c:247
#1 0x47c379 in aes_icm_openssl_context_init aes_icm_ossl.c:225
#2 0x478824 in cipher_type_test cipher.c:121
#3 0x4770f4 in cipher_driver_self_test cipher_driver.c:337
#4 0x476504 in main cipher_driver.c:206
#5 0x2b62af56bea4 in __libc_start_main libc-start.c:260
#6 0x47610c in _start ??:?

0x00000069a5be is located 34 bytes to the left of global variable 'aes_icm_test_case_0_ciphertext' from 'crypto/cipher/aes_icm_ossl.c' (0x69a5e0) of size 32
0x00000069a5be is located 0 bytes to the right of global variable 'aes_icm_test_case_0_key' from 'crypto/cipher/aes_icm_ossl.c' (0x69a5a0) of size 30

Contributor

ukreator commented Nov 7, 2013

The first call. To reproduce, I've just modified Makefile generated by './configure' to use clang with address sanitizer. Then, starting 'make runtest' (or smth like this). Output:

==2726==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000069a5be at pc 0x47e1cf bp 0x7fffa5ac6b20 sp 0x7fffa5ac6b18
READ of size 1 at 0x00000069a5be thread T0
#0 0x47e1ce in v128_copy_octet_string datatypes.c:247
#1 0x47c379 in aes_icm_openssl_context_init aes_icm_ossl.c:225
#2 0x478824 in cipher_type_test cipher.c:121
#3 0x4770f4 in cipher_driver_self_test cipher_driver.c:337
#4 0x476504 in main cipher_driver.c:206
#5 0x2b62af56bea4 in __libc_start_main libc-start.c:260
#6 0x47610c in _start ??:?

0x00000069a5be is located 34 bytes to the left of global variable 'aes_icm_test_case_0_ciphertext' from 'crypto/cipher/aes_icm_ossl.c' (0x69a5e0) of size 32
0x00000069a5be is located 0 bytes to the right of global variable 'aes_icm_test_case_0_key' from 'crypto/cipher/aes_icm_ossl.c' (0x69a5a0) of size 30

@jfigus

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus Nov 7, 2013

Contributor

Thanks for the information, this is helpful. The problem is the legacy counter mode AES self-test uses a single data structure for both the 128-bit and 256-bit self test. The new openssl implementation uses separate data structures for each key size, but the driver code that invokes the self tests is using the wrong data set for the 256 bit key size. This is a bug in the cipher driver code.

Sorry, but I don't have any experience with clang. I've installed it and modified the Makefile to use it. It looks like clang doesn't honor the gcc options. Can you provide the details of your Makefile modifications? It would be good if I can recreate the problem using clang to verify the fix before committing. Thanks.

Contributor

jfigus commented Nov 7, 2013

Thanks for the information, this is helpful. The problem is the legacy counter mode AES self-test uses a single data structure for both the 128-bit and 256-bit self test. The new openssl implementation uses separate data structures for each key size, but the driver code that invokes the self tests is using the wrong data set for the 256 bit key size. This is a bug in the cipher driver code.

Sorry, but I don't have any experience with clang. I've installed it and modified the Makefile to use it. It looks like clang doesn't honor the gcc options. Can you provide the details of your Makefile modifications? It would be good if I can recreate the problem using clang to verify the fix before committing. Thanks.

krisk84 and others added some commits Nov 7, 2013

add -fPIC to CFLAGS by default, use pkg-config to get LDFLAGS and CFL…
…AGS for openssl, output enable_openssl status
jfigus
Correct issue in cipher_driver unit test facility where the wrong cip…
…her instance was being used for AES-256 when configured for OpenSSL.
Merge pull request #35 from krisk84/feature-openssl
add -fPIC to CFLAGS by default, use pkg-config to get LDFLAGS and CFLAGS...
@ukreator

This comment has been minimized.

Show comment
Hide comment
@ukreator

ukreator Nov 11, 2013

Contributor

Sorry for late response. I've tested with clang-3.4 from trunk (built manually) and official 3.3 release from http://llvm.org/releases/download.html . The reason for using these builds is that some distributives like Fedora contain clang without sanitizers.

Makefile changes:

  • CC = /path/to/clang/binary
  • CFLAGS = -Wall -O1 -g -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize=address -I/usr/local/ssl/include
  • LDFLAGS = -L/usr/local/ssl/lib -L. -fsanitize=address

Then, run

$ make runtest 2> log.txt

Resulting file will contain warning with description. To get proper call stack with symbols (not just call addresses), get asan symbolize.py from https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/asan/scripts/asan_symbolize.py and run

$ python asan_symbolize.py / < log.txt

Also, crypto/include/sha1.h functions should be fixed not to have 'inline' keyword in their declarations (otherwise linker error). More details here: http://clang.llvm.org/compatibility.html#inline

Contributor

ukreator commented Nov 11, 2013

Sorry for late response. I've tested with clang-3.4 from trunk (built manually) and official 3.3 release from http://llvm.org/releases/download.html . The reason for using these builds is that some distributives like Fedora contain clang without sanitizers.

Makefile changes:

  • CC = /path/to/clang/binary
  • CFLAGS = -Wall -O1 -g -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize=address -I/usr/local/ssl/include
  • LDFLAGS = -L/usr/local/ssl/lib -L. -fsanitize=address

Then, run

$ make runtest 2> log.txt

Resulting file will contain warning with description. To get proper call stack with symbols (not just call addresses), get asan symbolize.py from https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/asan/scripts/asan_symbolize.py and run

$ python asan_symbolize.py / < log.txt

Also, crypto/include/sha1.h functions should be fixed not to have 'inline' keyword in their declarations (otherwise linker error). More details here: http://clang.llvm.org/compatibility.html#inline

@ukreator

This comment has been minimized.

Show comment
Hide comment
@ukreator

ukreator Nov 11, 2013

Contributor

Looks like original problem is fixed by your recent commits. However 'make runtest' gives another warning:

==2123==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff0db735c8 at pc 0x446cf8 bp 0x7fff0db73380 sp 0x7fff0db73378
READ of size 1 at 0x7fff0db735c8 thread T0
    #0 0x446cf7 in v128_copy_octet_string datatypes.c:241
    #1 0x440acb in srtp_calc_aead_iv srtp.c:791
    #2 0x4393b5 in srtp_protect_aead srtp.c:909
    #3 0x43803b in srtp_protect srtp.c:1184
    #4 0x430922 in srtp_test srtp_driver.c:666
    #5 0x42feb9 in main srtp_driver.c:233
    #6 0x7eff6f629b44 in __libc_start_main ??:?
    #7 0x42fb8c in _start ??:?
Address 0x7fff0db735c8 is located in stack of thread T0 at offset 104 in frame
    #0 0x438e9f in srtp_protect_aead srtp.c:832
  This frame has 6 object(s):
    [32, 36) 'enc_octet_len'
    [96, 104) 'est'
    [160, 164) 'tag_len'
    [224, 240) 'iv'
    [288, 312) 'data'
    [352, 376) 'data1'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Contributor

ukreator commented Nov 11, 2013

Looks like original problem is fixed by your recent commits. However 'make runtest' gives another warning:

==2123==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff0db735c8 at pc 0x446cf8 bp 0x7fff0db73380 sp 0x7fff0db73378
READ of size 1 at 0x7fff0db735c8 thread T0
    #0 0x446cf7 in v128_copy_octet_string datatypes.c:241
    #1 0x440acb in srtp_calc_aead_iv srtp.c:791
    #2 0x4393b5 in srtp_protect_aead srtp.c:909
    #3 0x43803b in srtp_protect srtp.c:1184
    #4 0x430922 in srtp_test srtp_driver.c:666
    #5 0x42feb9 in main srtp_driver.c:233
    #6 0x7eff6f629b44 in __libc_start_main ??:?
    #7 0x42fb8c in _start ??:?
Address 0x7fff0db735c8 is located in stack of thread T0 at offset 104 in frame
    #0 0x438e9f in srtp_protect_aead srtp.c:832
  This frame has 6 object(s):
    [32, 36) 'enc_octet_len'
    [96, 104) 'est'
    [160, 164) 'tag_len'
    [224, 240) 'iv'
    [288, 312) 'data'
    [352, 376) 'data1'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
@armandogmendoza

This comment has been minimized.

Show comment
Hide comment
@armandogmendoza

armandogmendoza Feb 13, 2014

what's the status of this pull request? is is going to be merged soon?
Thanks!

what's the status of this pull request? is is going to be merged soon?
Thanks!

@jfigus

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus Feb 14, 2014

Contributor

We need approval to commit from the other code maintainers. There were some comments from the initial review last May, which I fixed and subsequently submitted a new pull request. If the other maintainers don't object, I can go ahead and do the commit.

Contributor

jfigus commented Feb 14, 2014

We need approval to commit from the other code maintainers. There were some comments from the initial review last May, which I fixed and subsequently submitted a new pull request. If the other maintainers don't object, I can go ahead and do the commit.

@tvsriram

This comment has been minimized.

Show comment
Hide comment
@tvsriram

tvsriram Feb 18, 2014

Contributor

Looks good to me.

On Fri, Feb 14, 2014 at 7:44 AM, John Foley notifications@github.comwrote:

We need approval to commit from the other code maintainers. There were
some comments from the initial review last May, which I fixed and
subsequently submitted a new pull request. If the other maintainers don't
object, I can go ahead and do the commit.

Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/pull/34#issuecomment-35094765
.

Contributor

tvsriram commented Feb 18, 2014

Looks good to me.

On Fri, Feb 14, 2014 at 7:44 AM, John Foley notifications@github.comwrote:

We need approval to commit from the other code maintainers. There were
some comments from the initial review last May, which I fixed and
subsequently submitted a new pull request. If the other maintainers don't
object, I can go ahead and do the commit.

Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/pull/34#issuecomment-35094765
.

@jfigus

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus Feb 21, 2014

Contributor

Thanks. I'll wait another week and commit the code if there are no
objections.

On 02/18/2014 05:20 PM, tvsriram wrote:

Looks good to me.

On Fri, Feb 14, 2014 at 7:44 AM, John Foley
notifications@github.comwrote:

We need approval to commit from the other code maintainers. There were
some comments from the initial review last May, which I fixed and
subsequently submitted a new pull request. If the other maintainers
don't
object, I can go ahead and do the commit.

Reply to this email directly or view it on
GitHubhttps://github.com/cisco/libsrtp/pull/34#issuecomment-35094765
.


Reply to this email directly or view it on GitHub
#34 (comment).

Contributor

jfigus commented Feb 21, 2014

Thanks. I'll wait another week and commit the code if there are no
objections.

On 02/18/2014 05:20 PM, tvsriram wrote:

Looks good to me.

On Fri, Feb 14, 2014 at 7:44 AM, John Foley
notifications@github.comwrote:

We need approval to commit from the other code maintainers. There were
some comments from the initial review last May, which I fixed and
subsequently submitted a new pull request. If the other maintainers
don't
object, I can go ahead and do the commit.

Reply to this email directly or view it on
GitHubhttps://github.com/cisco/libsrtp/pull/34#issuecomment-35094765
.


Reply to this email directly or view it on GitHub
#34 (comment).

@krisk84

This comment has been minimized.

Show comment
Hide comment
@krisk84

krisk84 Feb 26, 2014

Contributor

Update: as of yesterday this is now included (and supported) in FreeSWITCH:

http://jira.freeswitch.org/browse/FS-5937

Contributor

krisk84 commented Feb 26, 2014

Update: as of yesterday this is now included (and supported) in FreeSWITCH:

http://jira.freeswitch.org/browse/FS-5937

@jfigus

This comment has been minimized.

Show comment
Hide comment
@jfigus

jfigus Feb 27, 2014

Contributor

Thanks for the update. I saw the email thread on this yesterday. It looks like I goofed on the SDES values in PJSIP. I’ll update these in my PJSIP fork to avoid any further confusion.

From: Kristian Kielhofner [mailto:notifications@github.com]
Sent: Wednesday, February 26, 2014 3:07 PM
To: cisco/libsrtp
Cc: John Foley (foleyj)
Subject: Re: [libsrtp] Feature openssl (#34)

Update: as of yesterday this is now included (and supported) in FreeSWITCH:

http://jira.freeswitch.org/browse/FS-5937


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/pull/34#issuecomment-36170375.

Contributor

jfigus commented Feb 27, 2014

Thanks for the update. I saw the email thread on this yesterday. It looks like I goofed on the SDES values in PJSIP. I’ll update these in my PJSIP fork to avoid any further confusion.

From: Kristian Kielhofner [mailto:notifications@github.com]
Sent: Wednesday, February 26, 2014 3:07 PM
To: cisco/libsrtp
Cc: John Foley (foleyj)
Subject: Re: [libsrtp] Feature openssl (#34)

Update: as of yesterday this is now included (and supported) in FreeSWITCH:

http://jira.freeswitch.org/browse/FS-5937


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/pull/34#issuecomment-36170375.

@krisk84

This comment has been minimized.

Show comment
Hide comment
@krisk84

krisk84 Feb 27, 2014

Contributor

No problem, those SDES values are a bit wonky. Thanks again for all of your help on this!

Contributor

krisk84 commented Feb 27, 2014

No problem, those SDES values are a bit wonky. Thanks again for all of your help on this!

jfigus added a commit that referenced this pull request Mar 2, 2014

Merge pull request #34 from cisco/feature-openssl
Merge feature openssl branch

@jfigus jfigus merged commit f34baf3 into master Mar 2, 2014

@jfigus jfigus deleted the feature-openssl branch Oct 31, 2014

This was referenced Jun 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment