Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129753002:
Prevent stack-buffer-overflow with address sanitizer (Closed)
|
Created:
4 years, 5 months ago by joachim Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/deps/libsrtp@master Target Ref:
refs/heads/master Project:
chromium_deps Visibility:
Public. |
DescriptionPrevent stack-buffer-overflow with address sanitizer
This is a backport of https://github.com/cisco/libsrtp/commit/da34bb7139e1ab7437e90afa039bde26d1e8d3cd
Original description:
-----
The value passed to srtp_aes_gcm_openssl_set_aad is not guarantied
to be >= c->tag_len. Since the call to EVP_CIPHER_CTX_ctrl is a
dummy call pass it a dummy tag that is large enough.
-----
This was also found by "asan" trybots in https://codereview.webrtc.org/1528843005/
BUG=webrtc:5222
R=mattdr@webrtc.org, tommi@webrtc.org
Committed: https://chromium.googlesource.com/chromium/deps/libsrtp/+/48bdd208dcdbb018c4a154cf260414dbdfabb86d
Patch Set 1 #
Total comments: 3
Messages
Total messages: 18 (6 generated)
jbauch@webrtc.org changed reviewers: + mattdr@webrtc.org
Ptal, this fixes the asan errors from https://codereview.webrtc.org/1528843005/ Is there a chance you can also integrate this in your internal libsrtp - if not it will block landing of the CL above (again) :-/
On 2016/07/06 20:02:17, joachim wrote: > Ptal, this fixes the asan errors from https://codereview.webrtc.org/1528843005/ > > Is there a chance you can also integrate this in your internal libsrtp - if not > it will block landing of the CL above (again) :-/ Yikes. Upstream's making some questionable decisions here... "OpenSSL reasonably wants a tag before decryption starts, but instead of reordering our calls we'll lie to it and assume it won't remember". I guess it's still better than passing the AAD as the expected tag value, which was indefensibly terrible. This isn't bad enough to diverge from upstream, though, so "lgtm". Once this is submitted here I'll integrate it into our internal libsrtp. Peter is *also* on vacation, so you may want to ping someone else for OWNERS if you want this in this week.
Description was changed from ========== Prevent stack-buffer-overflow with address sanitizer This is a backport of https://github.com/cisco/libsrtp/commit/da34bb7139e1ab7437e90afa039bde26d1e8d3cd Original description: ----- The value passed to srtp_aes_gcm_openssl_set_aad is not guarantied to be >= c->tag_len. Since the call to EVP_CIPHER_CTX_ctrl is a dummy call pass it a dummy tag that is large enough. ----- This was also found by "asan" trybots in https://codereview.webrtc.org/1528843005/ BUG=webrtc:5222 ========== to ========== Prevent stack-buffer-overflow with address sanitizer This is a backport of https://github.com/cisco/libsrtp/commit/da34bb7139e1ab7437e90afa039bde26d1e8d3cd Original description: ----- The value passed to srtp_aes_gcm_openssl_set_aad is not guarantied to be >= c->tag_len. Since the call to EVP_CIPHER_CTX_ctrl is a dummy call pass it a dummy tag that is large enough. ----- This was also found by "asan" trybots in https://codereview.webrtc.org/1528843005/ BUG=webrtc:5222 ==========
jbauch@webrtc.org changed reviewers: + jiayl@google.com
On 2016/07/06 21:42:37, mattdr-at-webrtc.org wrote: > On 2016/07/06 20:02:17, joachim wrote: > > Ptal, this fixes the asan errors from > https://codereview.webrtc.org/1528843005/ > > > > Is there a chance you can also integrate this in your internal libsrtp - if > not > > it will block landing of the CL above (again) :-/ > > Yikes. Upstream's making some questionable decisions here... "OpenSSL reasonably > wants a tag before decryption starts, but instead of reordering our calls we'll > lie to it and assume it won't remember". I guess it's still better than passing > the AAD as the expected tag value, which was indefensibly terrible. > > This isn't bad enough to diverge from upstream, though, so "lgtm". Once this is > submitted here I'll integrate it into our internal libsrtp. Perfect, thanks. > Peter is *also* on vacation, so you may want to ping someone else for OWNERS if > you want this in this week. I'm on vacation next week, so I'll try to get this CL to land and roll into Chromium before that - and then land the WebRTC GCM CL when I'm back (depending on how fast everything rolls up to WebRTC). +jiayl for owner review, could you please take a final look and land this?
On 2016/07/06 21:56:22, joachim wrote: > On 2016/07/06 21:42:37, http://mattdr-at-webrtc.org wrote: > > On 2016/07/06 20:02:17, joachim wrote: > > > Ptal, this fixes the asan errors from > > https://codereview.webrtc.org/1528843005/ > > > > > > Is there a chance you can also integrate this in your internal libsrtp - if > > not > > > it will block landing of the CL above (again) :-/ > > > > Yikes. Upstream's making some questionable decisions here... "OpenSSL > reasonably > > wants a tag before decryption starts, but instead of reordering our calls > we'll > > lie to it and assume it won't remember". I guess it's still better than > passing > > the AAD as the expected tag value, which was indefensibly terrible. > > > > This isn't bad enough to diverge from upstream, though, so "lgtm". Once this > is > > submitted here I'll integrate it into our internal libsrtp. > > Perfect, thanks. > > > Peter is *also* on vacation, so you may want to ping someone else for OWNERS > if > > you want this in this week. > > I'm on vacation next week, so I'll try to get this CL to land and roll into > Chromium before that - and then land the WebRTC GCM CL when I'm back (depending > on how fast everything rolls up to WebRTC). > > +jiayl for owner review, could you please take a final look and land this? Ping?
jiayl@google.com changed reviewers: + tommi@chromium.org
On 2016/07/18 13:44:02, jiayl1 wrote: Tommi: friendly ping
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... File srtp/crypto/cipher/aes_gcm_ossl.c (right): https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... srtp/crypto/cipher/aes_gcm_ossl.c:272: EVP_CIPHER_CTX_ctrl(&c->ctx, EVP_CTRL_GCM_SET_TAG, c->tag_len, &dummy_tag); does sizeof(dummy_tag) have to match c->tag_len?
https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... File srtp/crypto/cipher/aes_gcm_ossl.c (right): https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... srtp/crypto/cipher/aes_gcm_ossl.c:272: EVP_CIPHER_CTX_ctrl(&c->ctx, EVP_CTRL_GCM_SET_TAG, c->tag_len, &dummy_tag); On 2016/07/26 08:47:14, tommi-webrtc wrote: > does sizeof(dummy_tag) have to match c->tag_len? Yes, but it has to be between 1 and 16 bytes (see https://cs.chromium.org/chromium/src/third_party/boringssl/src/crypto/cipher/...), so always passing a buffer with "GCM_AUTH_TAG_LEN" (i.e. 16) bytes will work in any case.
lgtm https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... File srtp/crypto/cipher/aes_gcm_ossl.c (right): https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... srtp/crypto/cipher/aes_gcm_ossl.c:272: EVP_CIPHER_CTX_ctrl(&c->ctx, EVP_CTRL_GCM_SET_TAG, c->tag_len, &dummy_tag); On 2016/07/26 09:08:38, joachim wrote: > On 2016/07/26 08:47:14, tommi-webrtc wrote: > > does sizeof(dummy_tag) have to match c->tag_len? > > Yes, but it has to be between 1 and 16 bytes (see > https://cs.chromium.org/chromium/src/third_party/boringssl/src/crypto/cipher/...), > so always passing a buffer with "GCM_AUTH_TAG_LEN" (i.e. 16) bytes will work in > any case. Acknowledged.
On 2016/07/26 09:45:09, tommi-webrtc wrote: > lgtm Tommi, do you have the necessary permissions to land this? If not, Jiayl, please land. Thanks! > https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... > File srtp/crypto/cipher/aes_gcm_ossl.c (right): > > https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... > srtp/crypto/cipher/aes_gcm_ossl.c:272: EVP_CIPHER_CTX_ctrl(&c->ctx, > EVP_CTRL_GCM_SET_TAG, c->tag_len, &dummy_tag); > On 2016/07/26 09:08:38, joachim wrote: > > On 2016/07/26 08:47:14, tommi-webrtc wrote: > > > does sizeof(dummy_tag) have to match c->tag_len? > > > > Yes, but it has to be between 1 and 16 bytes (see > > > https://cs.chromium.org/chromium/src/third_party/boringssl/src/crypto/cipher/...), > > so always passing a buffer with "GCM_AUTH_TAG_LEN" (i.e. 16) bytes will work > in > > any case. > > Acknowledged.
On 2016/07/26 11:01:19, joachim wrote: > On 2016/07/26 09:45:09, tommi-webrtc wrote: > > lgtm > > Tommi, do you have the necessary permissions to land this? > If not, Jiayl, please land. > > Thanks! > > > > https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... > > File srtp/crypto/cipher/aes_gcm_ossl.c (right): > > > > > https://codereview.webrtc.org/2129753002/diff/1/srtp/crypto/cipher/aes_gcm_os... > > srtp/crypto/cipher/aes_gcm_ossl.c:272: EVP_CIPHER_CTX_ctrl(&c->ctx, > > EVP_CTRL_GCM_SET_TAG, c->tag_len, &dummy_tag); > > On 2016/07/26 09:08:38, joachim wrote: > > > On 2016/07/26 08:47:14, tommi-webrtc wrote: > > > > does sizeof(dummy_tag) have to match c->tag_len? > > > > > > Yes, but it has to be between 1 and 16 bytes (see > > > > > > https://cs.chromium.org/chromium/src/third_party/boringssl/src/crypto/cipher/...), > > > so always passing a buffer with "GCM_AUTH_TAG_LEN" (i.e. 16) bytes will work > > in > > > any case. > > > > Acknowledged. jiayl, friendly ping
Description was changed from ========== Prevent stack-buffer-overflow with address sanitizer This is a backport of https://github.com/cisco/libsrtp/commit/da34bb7139e1ab7437e90afa039bde26d1e8d3cd Original description: ----- The value passed to srtp_aes_gcm_openssl_set_aad is not guarantied to be >= c->tag_len. Since the call to EVP_CIPHER_CTX_ctrl is a dummy call pass it a dummy tag that is large enough. ----- This was also found by "asan" trybots in https://codereview.webrtc.org/1528843005/ BUG=webrtc:5222 ========== to ========== Prevent stack-buffer-overflow with address sanitizer This is a backport of https://github.com/cisco/libsrtp/commit/da34bb7139e1ab7437e90afa039bde26d1e8d3cd Original description: ----- The value passed to srtp_aes_gcm_openssl_set_aad is not guarantied to be >= c->tag_len. Since the call to EVP_CIPHER_CTX_ctrl is a dummy call pass it a dummy tag that is large enough. ----- This was also found by "asan" trybots in https://codereview.webrtc.org/1528843005/ BUG=webrtc:5222 R=mattdr@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/chromium/deps/libsrtp/+/48bdd208dcdbb018c4a... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 48bdd208dcdbb018c4a154cf260414dbdfabb86d (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
